Re: [PATCH v3 08/28] Add deref method to location description classes

[Lancelot SIX <lsix@lancelotsix.com>]

Hi,

I include few comments in the patch bellow.

On Thu, Oct 14, 2021 at 10:32:15AM +0100, Zoran Zaric via Gdb-patches wrote:
> From: Zoran Zaric <Zoran.Zaric@amd.com>
> 
> Concept of reading from a location seems to be too low level for the
> DWARF standard. What the standard actually describes is a concept of
> dereferencing, where the type of the operation result can be
> specified in advance.
> 
> This can be seen in the definition of the DW_OP_derefX family of
> expression operations, but it is also happening implicitly in the case
> of DW_OP_fbreg, DW_OP_regval_type and DW_OP_bregX family of operations.
> 
> Currently, the DW_OP_derefX operations will take the value from the
> DWARF expression stack and implicitly convert it to a memory location
> description (in reality treat it as a memory address for a given
> target) and apply the dereference operation to it. When we allow any
> location description on a DWARF expression stack, these operations need
> to work in the same way.
> 
> The conclusion here is that we need an universal method that model the

*a* universal method that *models*

> dereference operation for any class derived from a location description
> class.
> 
> It is worth mentioning that because of how the passed in buffers are
> currently being implemented, we needed a specialisation for the deref
> method of the dwarf_memory class to support them.
> 
> gdb/ChangeLog:
> 
>         * dwarf2/expr.c (dwarf_location::deref): New method.
>         (dwarf_memory::deref): New method.
> ---
>  gdb/dwarf2/expr.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index 83a2db028ca..87caba5fd62 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -422,6 +422,18 @@ class dwarf_location : public dwarf_entry
>  		      bool big_endian, int *optimized,
>  		      int *unavailable) const = 0;
>  
> +  /* Apply dereference operation on the DWARF location description.
> +     Operation returns a DWARF value of a given TYPE type while FRAME
> +     contains a frame context information of the location.  ADDR_INFO
> +     (if present) describes a passed in memory buffer if a regular
> +     memory read is not desired for certain address range.  If the SIZE
> +     is specified, it must be equal or smaller then the TYPE type size.

then -> than ?

> +     If SIZE is smaller then the type size, the value will be zero

then -> than ?

> +     extended to the difference.  */
> +  virtual std::unique_ptr<dwarf_value> deref
> +    (frame_info *frame, const property_addr_info *addr_info,
> +     struct type *type, size_t size = 0) const;
> +
>  protected:
>    /* Architecture of the location.  */
>    gdbarch *m_arch;
> @@ -489,6 +501,43 @@ class dwarf_value : public dwarf_entry
>  
>  using dwarf_value_up = std::unique_ptr<dwarf_value>;
>  
> +std::unique_ptr<dwarf_value>
> +dwarf_location::deref (frame_info *frame, const property_addr_info *addr_info,
> +		       struct type *type, size_t size) const
> +{
> +  bool big_endian = type_byte_order (type) == BFD_ENDIAN_BIG;
> +  size_t actual_size = size != 0 ? size : TYPE_LENGTH (type);
> +
> +  if (actual_size > TYPE_LENGTH (type))
> +    ill_formed_expression ();
> +
> +  /* If the size of the object read from memory is different
> +     from the type length, we need to zero-extend it.  */
> +  gdb::byte_vector read_buf (TYPE_LENGTH (type), 0);
> +  gdb_byte *buf_ptr = read_buf.data ();
> +  int optimized, unavailable;
> +
> +  if (big_endian)
> +    buf_ptr += TYPE_LENGTH (type) - actual_size;
> +
> +  this->read (frame, buf_ptr, 0, actual_size * HOST_CHAR_BIT,
> +	      0, 0, big_endian, &optimized, &unavailable);
> +
> +  if (optimized)
> +    throw_error (OPTIMIZED_OUT_ERROR,
> +		 _("Can't do read-modify-write to "
> +		   "update bitfield; containing word "
> +		   "has been optimized out"));

The error message should be about dereferencing I guess.

> +  if (unavailable)
> +    throw_error (NOT_AVAILABLE_ERROR,
> +		 _("Can't dereference "
> +		   "update bitfield; containing word "
> +		   "is unavailable"));
> +
> +  return make_unique<dwarf_value>
> +    (gdb::array_view<const gdb_byte> (read_buf), type);
> +}
> +
>  /* Undefined location description entry.  This is a special location
>     description type that describes the location description that is
>     not known.  */
> @@ -541,6 +590,11 @@ class dwarf_memory final : public dwarf_location
>  	      size_t location_bit_limit, bool big_endian,
>  	      int *optimized, int *unavailable) const override;
>  
> +  std::unique_ptr<dwarf_value> deref (frame_info *frame,
> +				      const property_addr_info *addr_info,
> +				      struct type *type,
> +				      size_t size = 0) const override;
> +
>  private:
>    /* True if the location belongs to a stack memory region.  */
>    bool m_stack;
> @@ -663,6 +717,73 @@ dwarf_memory::write (frame_info *frame, const gdb_byte *buf,
>      }
>  }
>  
> +std::unique_ptr<dwarf_value>
> +dwarf_memory::deref (frame_info *frame, const property_addr_info *addr_info,
> +		     struct type *type, size_t size) const
> +{
> +  bool big_endian = type_byte_order (type) == BFD_ENDIAN_BIG;
> +  size_t actual_size = size != 0 ? size : TYPE_LENGTH (type);
> +
> +  if (actual_size > TYPE_LENGTH (type))
> +    ill_formed_expression ();
> +
> +  gdb::byte_vector read_buf (TYPE_LENGTH (type), 0);
> +  size_t size_in_bits = actual_size * HOST_CHAR_BIT;
> +  gdb_byte *buf_ptr = read_buf.data ();
> +  bool passed_in_buf = false;
> +
> +  if (big_endian)
> +    buf_ptr += TYPE_LENGTH (type) - actual_size;
> +
> +  /* Covers the case where we have a passed in memory that is not
> +     part of the target and requires for the location description
> +     to address it instead of addressing the actual target
> +     memory.  */
> +  LONGEST this_size = bits_to_bytes (m_bit_suboffset, size_in_bits);
> +
> +  /* We shouldn't have a case where we read from a passed in
> +     memory and the same memory being marked as stack. */
> +  if (!m_stack && this_size && addr_info != nullptr
> +      && addr_info->valaddr.data () != nullptr)
> +    {
> +      CORE_ADDR offset = (CORE_ADDR) m_offset - addr_info->addr;
> +      /* Using second buffer here because the copy_bitwise
> +	 doesn't support in place copy.  */
> +      gdb::byte_vector temp_buf (this_size);

I guess the temp_buf can be declared in the if bellow. If
addr_info->valaddr does not contain the data we are looking for, there
is no need to create a unused buffer on the stack.

> +
> +      if (offset < addr_info->valaddr.size ()
> +	  && offset + this_size <= addr_info->valaddr.size ())
> +	{
> +	  memcpy (temp_buf.data (), addr_info->valaddr.data (), this_size);

Isn't it addr_info->valaddr.data() + offset ?

> +	  copy_bitwise (buf_ptr, 0, temp_buf.data (),
> +			m_bit_suboffset, size_in_bits, big_endian);
> +	  passed_in_buf = true;
> +	}
> +    }
> +
> +  if (!passed_in_buf)
> +    {
> +      int optimized, unavailable;
> +
> +      this->read (frame, buf_ptr, 0, size_in_bits, 0, 0,
> +		  big_endian, &optimized, &unavailable);
> +
> +      if (optimized)
> +	throw_error (OPTIMIZED_OUT_ERROR,
> +		     _("Can't do read-modify-write to "
> +		     "update bitfield; containing word "
> +		     "has been optimized out"));

The message should also be about dereferencing I guess.

Best,
Lancelot.

> +      if (unavailable)
> +	throw_error (NOT_AVAILABLE_ERROR,
> +		     _("Can't dereference "
> +		     "update bitfield; containing word "
> +		     "is unavailable"));
> +    }
> +
> +  return make_unique<dwarf_value>
> +    (gdb::array_view<const gdb_byte> (read_buf), type);
> +}
> +
>  /* Register location description entry.  */
>  
>  class dwarf_register final : public dwarf_location
> -- 
> 2.17.1
>