[PATCH] Rewrite gdb_mpz::export_bits

[Tom Tromey <tromey@adacore.com>]

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
---
 gdb/gmp-utils.c                     | 38 ++++++++++-------------------
 gdb/unittests/gmp-utils-selftests.c |  6 +++++
 2 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/gdb/gmp-utils.c b/gdb/gmp-utils.c
index b7fed9a82d1..949d75d21bd 100644
--- a/gdb/gmp-utils.c
+++ b/gdb/gmp-utils.c
@@ -128,35 +128,22 @@ 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)
+  gdb_mpz masked = *this;
+  masked.mask (buf.size () * HOST_CHAR_BIT);
+
+  if (sign < 0 && masked.sgn () != 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.  */
+	 would be the same as our negative value.  However, if masking
+	 left us with 0, we don't need to do anything else.  */
       gdb_mpz neg_offset = gdb_mpz::pow (2, buf.size () * HOST_CHAR_BIT);
-      un_signed = *exported_val + neg_offset;
-      exported_val = &un_signed;
-    }
-
-  /* If the value is too large, truncate it.  */
-  if (!safe
-      && mpz_sizeinbase (exported_val->m_val, 2) > buf.size () * HOST_CHAR_BIT)
-    {
-      /* 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);
+      masked += neg_offset;
     }
 
-  /* It's possible that one of the above results in zero, which has to
-     be handled specially.  */
-  if (exported_val->sgn () == 0)
+  /* It's possible that the above results in zero, which has to be
+     handled specially.  */
+  if (masked.sgn () == 0)
     {
       memset (buf.data (), 0, buf.size ());
       return;
@@ -174,8 +161,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 */,
+		 masked.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

base-commit: 2ece447a4f3dcf1d96bcaca644dbf895e42194b8
-- 
2.55.0