[PATCH] Rewrite gdb_mpz::export_bits

[PATCH] Rewrite gdb_mpz::export_bits

[Tom Tromey]

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

Re: [PATCH] Rewrite gdb_mpz::export_bits

[Tom de Vries]

On 9/9/26 8:08 PM, Tom Tromey wrote:

Hi Tom,

thanks for fixing this.

This LGTM.  I have some comments below, but these are all nits, so feel 
free to ignore.

Approved-By: Tom de Vries <tdevries@suse.de>

> 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. 

There's more than one bug?  If not, then maybe use "a bug".

> This patch rewrites part of this function,
> hopefully now getting the correct answer.  I think the new code should
> be somewhat simpler to follow.
> 

Agreed, it is simpler.

> 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.
> 

Agreed.

I was not familiar with this code, so I ended up splitting the patch in 
three:
- drop allocation minimization
- fix
- rename exported_val to masked
to get a more minimal fix to look at.

<bikeshedding>  I could live without the third patch, or a more neutral 
name like res or tmp.  Another approach could be to use meaningful names 
like masked, but then do something like:
...
-      masked += neg_offset;
-    }
+      un_signed = masked + neg_offset
+    } else {
+      un_signed = masked;
+    }
...
If so, I prefer "truncated" to "masked".  </bikeshedding>

> Regression tested on x86-64 Fedora 43.
> 
> I am not sure but it might be worth applying this to gdb 18; your
> thoughts appreciated.
> 

I think it's worth it.  As mentioned, I'm not familiar with this code, 
so I can't asses the risk.

> 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)

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

   if (sign < 0)
...

This starts to get verbose, but after factoring out some lambda functions:
...
+  auto export_zero = [&] ()
+  {
+    memset (buf.data (), 0, buf.size ());
+  };
+
+  auto export_if_zero = [&] (gdb_mpz &val)
+  {
+    if (val.sgn () != 0)
+      return false;
+
+    export_zero ();
+    return true;
+  };
...
this is reduced from 5 to 2 lines:
...
   if (export_if_zero (masked))
     return;

   if (sign < 0)
...

Thanks,
- Tom

>       {
>         /* 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

Re: [PATCH] Rewrite gdb_mpz::export_bits

[Tom Tromey]

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

Re: [PATCH] Rewrite gdb_mpz::export_bits

[Tom de Vries]

On 9/10/26 9:01 PM, Tom Tromey wrote:
> 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.

I see.

LGTM.

Thanks,
- Tom


> 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

Re: [PATCH] Rewrite gdb_mpz::export_bits

[Tom Tromey]

Tom> LGTM.

Thanks, I'm going to check it in.

I think I will apply it to gdb-18 as well.

Tom