Re: [PATCH v3 06/28] Add read method to location description classes

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-10-25 14:33, Lancelot SIX via Gdb-patches wrote:
> Hi,
> 
> On Thu, Oct 14, 2021 at 10:32:13AM +0100, Zoran Zaric via Gdb-patches wrote:
>> From: Zoran Zaric <Zoran.Zaric@amd.com>
>>
>> After adding the interface for register and memory location access, a
>> new method for reading from any location derived from a dwarf_location
>> class, can now be defined.
>>
>> In the case of implicit pointer location description the existing
>> indirect_synthetic_pointer interface requiers a type of the pointer
>> to be specified, so for a generic read interface the only type that
>> makes sense is the DWARF generic type. This means that the existing
>> address_type method of a dwarf_expr_context class needs to be exposed
>> outside of that class.
>>
>> gdb/ChangeLog:
>>
>>         * dwarf2/expr.c (dwarf_location::read): New method.
>>         (dwarf_undefined::read): New method.
>>         (dwarf_memory::read): New method.
>>         (dwarf_register::read): New method.
>>         (dwarf_implicit::read): New method.
>>         (dwarf_implicit_pointer::read): New method.
>>         (dwarf_composite::read): New method.
>>         (dwarf_expr_context::address_type): Change to use the new
>>         address_type function.
>>         (address_type): New function.
>> ---
>>  gdb/dwarf2/expr.c | 305 ++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 284 insertions(+), 21 deletions(-)
>>
>> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
>> index 56f17f52756..3f3e09db7cc 100644
>> --- a/gdb/dwarf2/expr.c
>> +++ b/gdb/dwarf2/expr.c
>> @@ -289,6 +289,36 @@ write_to_memory (CORE_ADDR address, const gdb_byte *buffer,
>>  					 length, buffer);
>>  }
>>  
>> +/* Return the type used for DWARF operations where the type is
>> +   generic in the DWARF spec, the ARCH is a target architecture.
> 
> There is an extra '.' at the end of this line.
> 
>> +   of the type and ADDR_SIZE is expected size of the address.
>> +   Only certain sizes are supported.  */
>> +
>> +static type *
>> +address_type (gdbarch *arch, int addr_size)
>> +{
>> +  dwarf_gdbarch_types *types
>> +    = (dwarf_gdbarch_types *) gdbarch_data (arch, dwarf_arch_cookie);
>> +  int ndx;
>> +
>> +  if (addr_size == 2)
>> +    ndx = 0;
>> +  else if (addr_size == 4)
>> +    ndx = 1;
>> +  else if (addr_size == 8)
>> +    ndx = 2;
>> +  else
>> +    error (_("Unsupported address size in DWARF expressions: %d bits"),
>> +	   HOST_CHAR_BIT * addr_size);
>> +
>> +  if (types->dw_types[ndx] == NULL)
> 
> It is not a big deal, but I think nullptr is preferred over NULL for
> new/updated code (there are few across the series, I can point them out
> if you want).
> 
>> +    types->dw_types[ndx]
>> +      = arch_integer_type (arch, HOST_CHAR_BIT * addr_size,
>> +			   0, "<signed DWARF address type>");
>> +
>> +  return types->dw_types[ndx];
>> +}
>> +
>>  class dwarf_location;
>>  class dwarf_memory;
>>  class dwarf_value;
>> @@ -349,6 +379,27 @@ class dwarf_location : public dwarf_entry
>>      ill_formed_expression ();
>>    }
>>  
>> +  /* Read contents from the described location.
>> +
>> +     The read operation is performed in the context of a FRAME.
>> +     BIT_SIZE is the number of bits to read.  The data read is copied
>> +     to the caller-managed buffer BUF.  BIG_ENDIAN defines the
>> +     endianness of the target.  BITS_TO_SKIP is a bit offset into the
>> +     location and BUF_BIT_OFFSET is buffer BUF's bit offset.
>> +     LOCATION_BIT_LIMIT is a maximum number of bits that location can
>> +     hold, where value zero signifies that there is no such
>> +     restriction.
>> +
>> +     Note that some location types can be read without a FRAME context.
>> +
>> +     If the location is optimized out or unavailable, the OPTIMIZED and
>> +     UNAVAILABLE outputs are set accordingly.  */
>> +  virtual void read (frame_info *frame, gdb_byte *buf,
>> +		     int buf_bit_offset, size_t bit_size,
>> +		     LONGEST bits_to_skip, size_t location_bit_limit,
>> +		     bool big_endian, int *optimized,
>> +		     int *unavailable) const = 0;
>> +
>>  protected:
>>    /* Architecture of the location.  */
>>    gdbarch *m_arch;
>> @@ -426,6 +477,14 @@ class dwarf_undefined final : public dwarf_location
>>    dwarf_undefined (gdbarch *arch)
>>      : dwarf_location (arch, 0)
>>    {}
>> +
>> +  void read (frame_info *frame, gdb_byte *buf, int buf_bit_offset,
>> +	     size_t bit_size, LONGEST bits_to_skip, size_t location_bit_limit,
>> +	     bool big_endian, int *optimized, int *unavailable) const override
>> +  {
>> +    *unavailable = 0;
>> +    *optimized = 1;
>> +  }
>>  };
>>  
>>  class dwarf_memory final : public dwarf_location
>> @@ -442,6 +501,11 @@ class dwarf_memory final : public dwarf_location
>>  
>>    dwarf_value_up to_value (struct type *type) const override;
>>  
>> +  void read (frame_info *frame, gdb_byte *buf, int buf_bit_offset,
>> +	     size_t bit_size, LONGEST bits_to_skip,
>> +	     size_t location_bit_limit, bool big_endian,
>> +	     int *optimized, int *unavailable) const override;
>> +
>>  private:
>>    /* True if the location belongs to a stack memory region.  */
>>    bool m_stack;
>> @@ -468,6 +532,46 @@ dwarf_memory::to_value (struct type *type) const
>>    return make_unique<dwarf_value> (m_offset, type);
>>  }
>>  
>> +void
>> +dwarf_memory::read (frame_info *frame, gdb_byte *buf,
>> +		    int buf_bit_offset, size_t bit_size,
>> +		    LONGEST bits_to_skip, size_t location_bit_limit,
>> +		    bool big_endian, int *optimized, int *unavailable) const
>> +{
>> +  LONGEST total_bits_to_skip = bits_to_skip;
>> +  CORE_ADDR start_address
>> +    = m_offset + (m_bit_suboffset + total_bits_to_skip) / HOST_CHAR_BIT;
> 
> I am not entirely sure of what follows, so if a maintainer can step in,
> I would appreciate.
> 
> I am not sure HOST_CHAR_BIT is what should be used here.  This is meant
> to compute an address on the target.  The number of bits per byte might
> (in theory) be different on the host and the target.  From what I
> understand, this is where gdbarch_addressable_memory_unit_size could
> come into play (it gives the number of octets associated with a memory
> address for the given arch).
> 
> In practice however, I do not know what target has non 8-bit bytes.
> set_gdbarch_addressable_memory_unit_size seems to never be called in the
> codebase, so I guess if such target has gdb support, it is off-tree.
> 
> As I said before, I am not entirely sure to what extent such target
> are currently supported.  Is this something you have considered?

It's true that there isn't an architecture upstream which uses that
feature.  I was the one who introduced
gdbarch_addressable_memory_unit_size, while at my previous job, because
we had a platform with non-8-bit-bytes.  And this was merged, as it was
seen as an improvement over using TARGET_CHAR_BIT (TARGET_CHAR_BIT is a
compile-time constant, so if you are building a GDB that debugs
architectures with different byte sizes... that doesn't work).

So yeah, gdbarch_addressable_memory_unit_size looks like the right thing
to use here.  Note that this function returns 1 for x86, and 2 for an
architecture with 16-bit bytes.  So you would divide by:

  gdbarch_addressable_memory_unit_size (arch) * 8

I am fine with using the magical number 8 here, because
gdbarch_addressable_memory_unit_size is explicitly defined as "the size
in 8-bit bytes of...".  Using CHAR_BIT or something like that would
defeat the purpose because it could *in theory* be something else than
8.

So cases like this one, we do a best effort to write things in a way
that works for non-8-bit-bytes platform, but don't sweat it too much.
Users downstream who actually use that will fix it and provide patches
if needed.

Simon