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

[Lancelot SIX <lsix@lancelotsix.com>]

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?

Best,
Lancelot.

> +  gdb::byte_vector temp_buf;
> +
> +  *optimized = 0;
> +  total_bits_to_skip += m_bit_suboffset;
> +
> +  if (total_bits_to_skip % HOST_CHAR_BIT == 0
> +      && bit_size % HOST_CHAR_BIT == 0
> +      && buf_bit_offset % HOST_CHAR_BIT == 0)
> +    {
> +      /* Everything is byte-aligned, no buffer needed.  */
> +      read_from_memory (start_address,
> +			buf + buf_bit_offset / HOST_CHAR_BIT,
> +			bit_size / HOST_CHAR_BIT, m_stack, unavailable);
> +    }
> +  else
> +    {
> +      LONGEST this_size = bits_to_bytes (total_bits_to_skip, bit_size);
> +      temp_buf.resize (this_size);
> +
> +      /* Can only read from memory on byte granularity so an
> +	 additional buffer is required.  */
> +      read_from_memory (start_address, temp_buf.data (), this_size,
> +			m_stack, unavailable);
> +
> +      if (!*unavailable)
> +	copy_bitwise (buf, buf_bit_offset, temp_buf.data (),
> +		      total_bits_to_skip % HOST_CHAR_BIT,
> +		      bit_size, big_endian);
> +    }
> +}
> +
>  /* Register location description entry.  */
>  
>  class dwarf_register final : public dwarf_location
> @@ -477,11 +581,56 @@ class dwarf_register final : public dwarf_location
>      : dwarf_location (arch, offset), m_regnum (regnum)
>    {}
>  
> +  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:
>    /* DWARF register number.  */
>    unsigned int m_regnum;
>  };
>  
> +void
> +dwarf_register::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;
> +  size_t read_bit_limit = location_bit_limit;
> +  gdbarch *frame_arch = get_frame_arch (frame);
> +  int reg = dwarf_reg_to_regnum_or_error (frame_arch, m_regnum);
> +  ULONGEST reg_bits = HOST_CHAR_BIT * register_size (frame_arch, reg);
> +  gdb::byte_vector temp_buf;
> +
> +  if (big_endian)
> +    {
> +      if (!read_bit_limit || reg_bits <= read_bit_limit)
> +	read_bit_limit = bit_size;
> +
> +      total_bits_to_skip += reg_bits - (m_offset * HOST_CHAR_BIT
> +					+ m_bit_suboffset + read_bit_limit);
> +    }
> +  else
> +    total_bits_to_skip += m_offset * HOST_CHAR_BIT + m_bit_suboffset;
> +
> +  LONGEST this_size = bits_to_bytes (total_bits_to_skip, bit_size);
> +  temp_buf.resize (this_size);
> +
> +  if (frame == NULL)
> +    internal_error (__FILE__, __LINE__, _("invalid frame information"));
> +
> +  /* Can only read from a register on byte granularity so an
> +     additional buffer is required.  */
> +  read_from_register (frame, reg, total_bits_to_skip / HOST_CHAR_BIT,
> +		      temp_buf, optimized, unavailable);
> +
> +  /* Only copy data if valid.  */
> +  if (!*optimized && !*unavailable)
> +    copy_bitwise (buf, buf_bit_offset, temp_buf.data (),
> +		  total_bits_to_skip % HOST_CHAR_BIT, bit_size, big_endian);
> +}
> +
>  /* Implicit location description entry.  Describes a location
>     description not found on the target but instead saved in a
>     gdb-allocated buffer.  */
> @@ -497,6 +646,10 @@ class dwarf_implicit final : public dwarf_location
>        m_byte_order (byte_order)
>    {}
>  
> +  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:
>    /* Implicit location contents as a stream of bytes in target byte-order.  */
>    gdb::byte_vector m_contents;
> @@ -505,6 +658,45 @@ class dwarf_implicit final : public dwarf_location
>    bfd_endian m_byte_order;
>  };
>  
> +void
> +dwarf_implicit::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
> +{
> +  ULONGEST implicit_bit_size = HOST_CHAR_BIT * m_contents.size ();
> +  LONGEST total_bits_to_skip = bits_to_skip;
> +  size_t read_bit_limit = location_bit_limit;
> +
> +  *optimized = 0;
> +  *unavailable = 0;
> +
> +  /* Cut off at the end of the implicit value.  */
> +  if (m_byte_order == BFD_ENDIAN_BIG)
> +    {
> +      if (!read_bit_limit || read_bit_limit > implicit_bit_size)
> +	read_bit_limit = bit_size;
> +
> +      total_bits_to_skip
> +	+= implicit_bit_size - (m_offset * HOST_CHAR_BIT
> +			       + m_bit_suboffset + read_bit_limit);
> +    }
> +  else
> +    total_bits_to_skip += m_offset * HOST_CHAR_BIT + m_bit_suboffset;
> +
> +  if (total_bits_to_skip >= implicit_bit_size)
> +    {
> +      *unavailable = 1;
> +      return;
> +    }
> +
> +  if (bit_size > implicit_bit_size - total_bits_to_skip)
> +    bit_size = implicit_bit_size - total_bits_to_skip;
> +
> +  copy_bitwise (buf, buf_bit_offset, m_contents.data (),
> +		total_bits_to_skip, bit_size, big_endian);
> +}
> +
>  /* Implicit pointer location description entry.  */
>  
>  class dwarf_implicit_pointer final : public dwarf_location
> @@ -520,6 +712,10 @@ class dwarf_implicit_pointer final : public dwarf_location
>        m_addr_size (addr_size), m_die_offset (die_offset)
>    {}
>  
> +  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:
>    /* Per object file data of the implicit pointer.  */
>    dwarf2_per_objfile *m_per_objfile;
> @@ -534,6 +730,44 @@ class dwarf_implicit_pointer final : public dwarf_location
>    sect_offset m_die_offset;
>  };
>  
> +void
> +dwarf_implicit_pointer::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
> +{
> +  frame_info *actual_frame = frame;
> +  LONGEST total_bits_to_skip = bits_to_skip + m_bit_suboffset;
> +
> +  if (actual_frame == nullptr)
> +    actual_frame = get_selected_frame (_("No frame selected."));
> +
> +  struct type *type
> +    = address_type (get_frame_arch (actual_frame), m_addr_size);
> +
> +  struct value *value
> +    = indirect_synthetic_pointer (m_die_offset, m_offset, m_per_cu,
> +				  m_per_objfile, actual_frame, type);
> +
> +  gdb_byte *value_contents
> +    = value_contents_raw (value) + total_bits_to_skip / HOST_CHAR_BIT;
> +
> +  if (total_bits_to_skip % HOST_CHAR_BIT == 0
> +      && bit_size % HOST_CHAR_BIT == 0
> +      && buf_bit_offset % HOST_CHAR_BIT == 0)
> +    {
> +      memcpy (buf + buf_bit_offset / HOST_CHAR_BIT,
> +	      value_contents, bit_size / HOST_CHAR_BIT);
> +    }
> +  else
> +    {
> +      copy_bitwise (buf, buf_bit_offset, value_contents,
> +		    total_bits_to_skip % HOST_CHAR_BIT,
> +		    bit_size, big_endian);
> +    }
> +}
> +
>  /* Composite location description entry.  */
>  
>  class dwarf_composite final : public dwarf_location
> @@ -549,6 +783,10 @@ class dwarf_composite final : public dwarf_location
>      m_pieces.emplace_back (std::move (location), bit_size);
>    }
>  
> +  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:
>    /* Composite piece that contains a piece location
>       description and it's size.  */
> @@ -570,6 +808,50 @@ class dwarf_composite final : public dwarf_location
>    std::vector<piece> m_pieces;
>  };
>  
> +void
> +dwarf_composite::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
> +{
> +  unsigned int pieces_num = m_pieces.size ();
> +  LONGEST total_bits_to_skip = bits_to_skip;
> +  unsigned int i;
> +
> +  total_bits_to_skip += m_offset * HOST_CHAR_BIT + m_bit_suboffset;
> +
> +  /* Skip pieces covered by the read offset.  */
> +  for (i = 0; i < pieces_num; i++)
> +    {
> +      LONGEST piece_bit_size = m_pieces[i].size;
> +
> +      if (total_bits_to_skip < piece_bit_size)
> +        break;
> +
> +      total_bits_to_skip -= piece_bit_size;
> +    }
> +
> +  for (; i < pieces_num; i++)
> +    {
> +      LONGEST piece_bit_size = m_pieces[i].size;
> +      LONGEST actual_bit_size = piece_bit_size;
> +
> +      if (actual_bit_size > bit_size)
> +        actual_bit_size = bit_size;
> +
> +      m_pieces[i].location->read (frame, buf, buf_bit_offset,
> +				  actual_bit_size, total_bits_to_skip,
> +				  piece_bit_size, big_endian,
> +				  optimized, unavailable);
> +
> +      if (bit_size == actual_bit_size || *optimized || *unavailable)
> +	break;
> +
> +      buf_bit_offset += actual_bit_size;
> +      bit_size -= actual_bit_size;
> +    }
> +}
> +
>  struct piece_closure
>  {
>    /* Reference count.  */
> @@ -1185,27 +1467,8 @@ sect_variable_value (sect_offset sect_off,
>  struct type *
>  dwarf_expr_context::address_type () const
>  {
> -  gdbarch *arch = this->m_per_objfile->objfile->arch ();
> -  dwarf_gdbarch_types *types
> -    = (dwarf_gdbarch_types *) gdbarch_data (arch, dwarf_arch_cookie);
> -  int ndx;
> -
> -  if (this->m_addr_size == 2)
> -    ndx = 0;
> -  else if (this->m_addr_size == 4)
> -    ndx = 1;
> -  else if (this->m_addr_size == 8)
> -    ndx = 2;
> -  else
> -    error (_("Unsupported address size in DWARF expressions: %d bits"),
> -	   8 * this->m_addr_size);
> -
> -  if (types->dw_types[ndx] == NULL)
> -    types->dw_types[ndx]
> -      = arch_integer_type (arch, 8 * this->m_addr_size,
> -			   0, "<signed DWARF address type>");
> -
> -  return types->dw_types[ndx];
> +  return ::address_type (this->m_per_objfile->objfile->arch (),
> +			 this->m_addr_size);
>  }
>  
>  /* Create a new context for the expression evaluator.  */
> -- 
> 2.17.1
>