Re: [PATCH] Rewrite gdb_mpz::export_bits

[Tom de Vries <tdevries@suse.de>]

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