Re: [PATCH] Rewrite gdb_mpz::export_bits

[Tom Tromey <tromey@adacore.com>]

Tom> If so, I prefer "truncated" to "masked".  </bikeshedding>

I changed the name.

>> +  if (sign < 0 && masked.sgn () != 0)

Tom> I think it's clearer to do this, because there's less state to keep
Tom> going forward:
Tom> ...
Tom>   if (masked.sgn () == 0)
Tom>     {
Tom>       memset (buf.data (), 0, buf.size ());
Tom>       return;
Tom>     }

Tom>   if (sign < 0)
Tom> ...

Tom> This starts to get verbose, but after factoring out some lambda functions:

I don't like lambda functions in a situation like this.  IMO they often
just obfuscate the code.

Instead I rearranged the code a bit in v2.
This makes it a little simpler.

Tom

commit b107ae9e505d14a46211dd6832b69b8bd70fc16b
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Sep 9 11:22:04 2026 -0600

    Rewrite gdb_mpz::export_bits
    
    A couple of bugs point out that, when multiplication overflows, gdb
    does not compute the correct result.  This is caused by some bugs in
    gdb_mpz::export_bits.  This patch rewrites part of this function,
    hopefully now getting the correct answer.  I think the new code should
    be somewhat simpler to follow.
    
    A new selftest is added, derived from the code in the bug report.
    
    This rewrite doesn't try to minimize allocations, the way the previous
    one did.  I tend to doubt that matters, and this is one of the
    readability improvements IMO.
    
    Regression tested on x86-64 Fedora 43.
    
    I am not sure but it might be worth applying this to gdb 18; your
    thoughts appreciated.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=34601

diff --git a/gdb/gmp-utils.c b/gdb/gmp-utils.c
index b7fed9a82d1..21f474798eb 100644
--- a/gdb/gmp-utils.c
+++ b/gdb/gmp-utils.c
@@ -128,38 +128,24 @@ gdb_mpz::export_bits (gdb::array_view<gdb_byte> buf, int endian, bool unsigned_p
 	       hi.str ().c_str ());
     }
 
-  const gdb_mpz *exported_val = this;
-  gdb_mpz un_signed;
-  if (sign < 0)
-    {
-      /* mpz_export does not handle signed values, so create a positive
-	 value whose bit representation as an unsigned of the same length
-	 would be the same as our negative value.  */
-      gdb_mpz neg_offset = gdb_mpz::pow (2, buf.size () * HOST_CHAR_BIT);
-      un_signed = *exported_val + neg_offset;
-      exported_val = &un_signed;
-    }
+  gdb_mpz truncated = *this;
+  truncated.mask (buf.size () * HOST_CHAR_BIT);
 
-  /* If the value is too large, truncate it.  */
-  if (!safe
-      && mpz_sizeinbase (exported_val->m_val, 2) > buf.size () * HOST_CHAR_BIT)
+  /* It's possible that the above results in zero, which has to be
+     handled specially.  */
+  if (truncated.sgn () == 0)
     {
-      /* If we don't already have a copy, make it now.  */
-      if (exported_val != &un_signed)
-	{
-	  un_signed = *exported_val;
-	  exported_val = &un_signed;
-	}
-
-      un_signed.mask (buf.size () * HOST_CHAR_BIT);
+      memset (buf.data (), 0, buf.size ());
+      return;
     }
 
-  /* It's possible that one of the above results in zero, which has to
-     be handled specially.  */
-  if (exported_val->sgn () == 0)
+  if (sign < 0)
     {
-      memset (buf.data (), 0, buf.size ());
-      return;
+      /* mpz_export does not handle signed values, so create a
+	 positive value whose bit representation as an unsigned of the
+	 same length would be the same as our negative value.  */
+      gdb_mpz neg_offset = gdb_mpz::pow (2, buf.size () * HOST_CHAR_BIT);
+      truncated += neg_offset;
     }
 
   /* Do the export into a buffer allocated by GMP itself; that way,
@@ -174,8 +160,9 @@ gdb_mpz::export_bits (gdb::array_view<gdb_byte> buf, int endian, bool unsigned_p
 
   size_t word_countp;
   gdb::unique_xmalloc_ptr<void> exported
-    (mpz_export (NULL, &word_countp, -1 /* order */, buf.size () /* size */,
-		 endian, 0 /* nails */, exported_val->m_val));
+    (mpz_export (nullptr, &word_countp, -1 /* order */,
+		 buf.size () /* size */, endian, 0 /* nails */,
+		 truncated.m_val));
 
   gdb_assert (word_countp == 1);
 
diff --git a/gdb/unittests/gmp-utils-selftests.c b/gdb/unittests/gmp-utils-selftests.c
index 1912d346c17..417abae5463 100644
--- a/gdb/unittests/gmp-utils-selftests.c
+++ b/gdb/unittests/gmp-utils-selftests.c
@@ -80,6 +80,12 @@ gdb_mpz_as_integer ()
   v -= 1;
 
   SELF_CHECK (v.as_integer<ULONGEST> () == ul_expected);
+
+  /* This is from PR gdb/34601.  */
+  LONGEST neg = (LONGEST) 0x8000000000000001ull;
+  gdb_mpz a (neg);
+  gdb_mpz b (0x1234);
+  SELF_CHECK ((a * b).as_integer_truncate<int64_t> () == 0x1234);
 }
 
 /* A helper function which calls the given gdb_mpz object's as_integer