Re: [PATCH] Rewrite gdb_mpz::export_bits

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

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