[4/8] Add value_as_mpz and value_from_mpz

  This adds the two new functions, value_as_mpz and value_from_mpz,
useful for manipulation values via gdb_mpz.
---
 gdb/value.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/value.h |   7 ++++
 2 files changed, 113 insertions(+)
  

Hi,

On Fri, Mar 03, 2023 at 02:12:03PM -0700, Tom Tromey via Gdb-patches wrote:
> This adds the two new functions, value_as_mpz and value_from_mpz,
> useful for manipulation values via gdb_mpz.
> ---
>  gdb/value.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/value.h |   7 ++++
>  2 files changed, 113 insertions(+)
> 
> diff --git a/gdb/value.c b/gdb/value.c
> index e9afa89c243..19da9e1a00b 100644
> --- a/gdb/value.c
> +++ b/gdb/value.c
> @@ -2551,6 +2551,76 @@ value_as_long (struct value *val)
>    return unpack_long (val->type (), val->contents ().data ());
>  }
>  
> +/* See value.h.  */
> +
> +gdb_mpz
> +value_as_mpz (struct value *val)
> +{
> +  val = coerce_array (val);
> +  struct type *type = check_typedef (val->type ());
> +
> +  switch (type->code ())
> +    {
> +    case TYPE_CODE_ENUM:
> +    case TYPE_CODE_BOOL:
> +    case TYPE_CODE_INT:
> +    case TYPE_CODE_CHAR:
> +    case TYPE_CODE_RANGE:
> +      break;
> +
> +    default:
> +      return gdb_mpz (value_as_long (val));
> +    }
> +
> +  gdb_mpz result;
> +
> +  gdb::array_view<const gdb_byte> valbytes = val->contents ();
> +  enum bfd_endian byte_order = type_byte_order (type);
> +
> +  /* Handle integers that are either not a multiple of the word size,
> +     or that are stored at some bit offset.  */
> +  unsigned bit_off = 0, bit_size = 0;
> +  if (type->bit_size_differs_p ())
> +    {
> +      bit_size = type->bit_size ();
> +      if (bit_size == 0)
> +	{
> +	  /* We can just handle this immediately.  */
> +	  return result;
> +	}
> +
> +      bit_off = type->bit_offset ();
> +
> +      unsigned n_bytes = ((bit_off % 8) + bit_size + 7) / 8;

This is a bit out of the scope of this patch as gdbvalue.h says that
bit_off / bit_size are 8 based, but should they be HOST_CHAR_BIT based
instead?  The n_bytes is used to to access a
"gdb::array_view<const gdb_byte>".

> +      valbytes = valbytes.slice (bit_off / 8, n_bytes);
> +
> +      if (byte_order == BFD_ENDIAN_BIG)
> +	bit_off = (n_bytes * 8 - bit_off % 8 - bit_size);
> +      else
> +	bit_off %= 8;
> +    }
> +
> +  result.read (val->contents (), byte_order, type->is_unsigned ());
> +
> +  /* Shift off any low bits, if needed.  */
> +  if (bit_off != 0)
> +    result >>= bit_off;
> +
> +  /* Mask off any high bits, if needed.  */
> +  if (bit_size)
> +    result.mask (bit_size);
> +
> +  /* Now handle any range bias.  */
> +  if (type->code () == TYPE_CODE_RANGE && type->bounds ()->bias != 0)
> +    {
> +      /* Unfortunately we have to box here, because LONGEST is
> +	 probably wider than long.  */
> +      result += gdb_mpz (type->bounds ()->bias);
> +    }
> +
> +  return result;
> +}
> +
>  /* Extract a value as a C pointer.  Does not deallocate the value.
>     Note that val's type may not actually be a pointer; value_as_long
>     handles all the cases.  */
> @@ -3378,6 +3448,42 @@ value_from_ulongest (struct type *type, ULONGEST num)
>    return val;
>  }
>  
> +/* See value.h.  */
> +
> +struct value *
> +value_from_mpz (struct type *type, const gdb_mpz &v)
> +{
> +  struct type *real_type = check_typedef (type);
> +
> +  const gdb_mpz *val = &v;
> +  gdb_mpz storage;
> +  if (real_type->code () == TYPE_CODE_RANGE && type->bounds ()->bias != 0)
> +    {
> +      storage = *val;
> +      val = &storage;
> +      storage -= type->bounds ()->bias;
> +    }
> +
> +  if (type->bit_size_differs_p ())
> +    {
> +      unsigned bit_off = type->bit_offset ();
> +      unsigned bit_size = type->bit_size ();
> +
> +      if (val != &storage)
> +	{
> +	  storage = *val;
> +	  val = &storage;
> +	}
> +
> +      storage.mask ((ULONGEST) 1 << bit_size);

I do not think you should have the "(ULONGEST) 1 <<" part here.
gdb_mpz::mask does mask N bits (N being its arg).

> +      storage <<= bit_off;
> +    }

Best,
Lancelot.
  

>> +      unsigned n_bytes = ((bit_off % 8) + bit_size + 7) / 8;

Lancelot> This is a bit out of the scope of this patch as gdbvalue.h says that
Lancelot> bit_off / bit_size are 8 based, but should they be HOST_CHAR_BIT based
Lancelot> instead?  The n_bytes is used to to access a
Lancelot> "gdb::array_view<const gdb_byte>".

Yeah, I do not know.  I am not sure I've ever understood how gdb intends
to handle these scenarios, if it even really does.  I do see uses of
HOST_CHAR_BIT, but also plain '8'.

Also there's this weirdness:

    #if defined (CHAR_BIT)
    #define HOST_CHAR_BIT CHAR_BIT
    #else
    #define HOST_CHAR_BIT TARGET_CHAR_BIT
    #endif

It's hard to understand how this can ever make sense, even despite the
comment (which may be obsolete).

TARGET_CHAR_BIT is even worse because surely it cannot be a constant.

Anyway, this code in question in this patch is just copied from
elsewhere.

>> +      storage.mask ((ULONGEST) 1 << bit_size);

Lancelot> I do not think you should have the "(ULONGEST) 1 <<" part here.
Lancelot> gdb_mpz::mask does mask N bits (N being its arg).

Thank you.

Tom