Re: [PATCH 16/43] Simplify dwarf_expr_context class interface

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 2021-03-01 9:45 a.m., Zoran Zaric via Gdb-patches wrote:
> From: Zoran Zaric <zoran.zaric@amd.com>
> 
> Idea of this patch is to get a clean and simple public interface for
> the dwarf_expr_context class, looking like:
> 
> - constructor,
> - destructor,
> - push_address method and
> - evaluate method.
> 
> Where constructor should only ever require a target architecture
> information. This information is held in per object file
> (dwarf2_per_objfile) structure, so it makes sense to keep that
> structure as a constructor argument. It also makes sense to get the
> address size from that structure, but unfortunately that interface
> doesn’t exist at the moment, so the dwarf_expr_context class user
> needs to provide that information.
> 
> The push_address method is used to push a CORE_ADDR as a value on
> top of the DWARF stack before the evaluation. This method can be
> later changed to push any struct value object on the stack.
> 
> The evaluate method is the method that evaluates a DWARF expression
> and provides the evaluation result, in a form of a single struct
> value object that describes a location. To do this, the method requires
> a context of the evaluation, as well as expected result type
> information. If the type information is not provided, the DWARF generic
> type will be used instead.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2/expr.c (dwarf_expr_context::dwarf_expr_context): Add
> 	address size argument.
> 	(dwarf_expr_context::read_mem): Change to use property_addr_info
> 	structure.
> 	(dwarf_expr_context::evaluate): New function.
> 	(dwarf_expr_context::execute_stack_op): Change to use
> 	property_addr_info structure.
> 	* dwarf2/expr.h (struct dwarf_expr_context): New evaluate
> 	declaration. Change eval and fetch_result method to private.
> 	* dwarf2/frame.c (execute_stack_op): Change to call evaluate
> 	method.
> 	* dwarf2/loc.c (dwarf2_evaluate_loc_desc_full): Change to call
> 	evaluate method.
> 	(dwarf2_locexpr_baton_eval): Change to call evaluate method.
> ---
>  gdb/dwarf2/expr.c  | 49 ++++++++++++++++++++++++++++++++++++----------
>  gdb/dwarf2/expr.h  | 44 +++++++++++++++++++++++++++--------------
>  gdb/dwarf2/frame.c | 18 ++++-------------
>  gdb/dwarf2/loc.c   | 41 ++++++++++++--------------------------
>  4 files changed, 85 insertions(+), 67 deletions(-)
> 
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index b2248681899..1645f477d74 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -699,8 +699,11 @@ dwarf_expr_context::address_type () const
>  
>  /* Create a new context for the expression evaluator.  */
>  
> -dwarf_expr_context::dwarf_expr_context (dwarf2_per_objfile *per_objfile)
> -: per_objfile (per_objfile)
> +dwarf_expr_context::dwarf_expr_context (dwarf2_per_objfile *per_objfile,
> +					int addr_size)
> +: gdbarch (per_objfile->objfile->arch ()),

If gdbarch is always obtained from the objfile, we could maybe avoid
storing it.

> +  addr_size (addr_size),
> +  per_objfile (per_objfile)
>  {
>  }
>  
> @@ -825,13 +828,17 @@ dwarf_expr_context::read_mem (gdb_byte *buf, CORE_ADDR addr,
>      return;
>  
>    /* Prefer the passed-in memory, if it exists.  */
> -  CORE_ADDR offset = addr - obj_address;
> +  if (addr_info != nullptr)
> +    {
> +      CORE_ADDR offset = addr - addr_info->addr;
>  
> -  if (offset < data_view.size () && offset + length <= data_view.size ())
> +      if (offset < addr_info->valaddr.size ()
> +	  && offset + length <= addr_info->valaddr.size ())
>      {
> -      memcpy (buf, data_view.data (), length);
> +      memcpy (buf, addr_info->valaddr.data (), length);

Not related to this patch, but: I find it odd that the field `valaddr`
contains the object's value.  Why is it called like that?

>        return;
>      }
> +    }

The indentation here is not right.

>  
>    read_memory (addr, buf, length);
>  }
> @@ -874,8 +881,8 @@ dwarf_expr_context::push_dwarf_reg_entry_value
>  						   caller_frame);
>    scoped_restore save_per_cu = make_scoped_restore (&this->per_cu,
>  						    caller_per_cu);
> -  scoped_restore save_obj_addr = make_scoped_restore (&this->obj_address,
> -						      (CORE_ADDR) 0);
> +  scoped_restore save_addr_info = make_scoped_restore (&this->addr_info,
> +						       nullptr);
>    scoped_restore save_per_objfile = make_scoped_restore (&this->per_objfile,
>  							 caller_per_objfile);
>  
> @@ -1043,6 +1050,28 @@ dwarf_expr_context::fetch_result (struct type *type,
>    return retval;
>  }
>  
> +/* See expr.h.  */
> +
> +struct value *
> +dwarf_expr_context::evaluate (const gdb_byte *addr, size_t len,
> +			      struct dwarf2_per_cu_data *per_cu,
> +			      struct frame_info *frame,
> +			      const struct property_addr_info *addr_info,
> +			      struct type *type,
> +			      struct type *subobj_type,
> +			      LONGEST subobj_offset)
> +{
> +  this->per_cu = per_cu;
> +  this->frame = frame;
> +  this->addr_info = addr_info;
> +
> +  if (per_cu != nullptr)
> +    this->ref_addr_size = per_cu->ref_addr_size ();

As mentioned in a previous message, I have the feeling that we could get
rid of the ref_addr_size field and just call per_cu->ref_addr_size when
we need it.  It could always be a follow-up cleanup.

> +
> +  eval (addr, len);
> +  return fetch_result (type, subobj_type, subobj_offset);

This is just me, not an officiel style rule, when I like to use `this->`
when accessing fields and methods, to make it clear what they are (not
global variables or free functions).  Although for fields prefixed with
`m_`, I don't, because it's already obvious by the naming.

> diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
> index a0ac21f2ed1..d1374068732 100644
> --- a/gdb/dwarf2/expr.h
> +++ b/gdb/dwarf2/expr.h
> @@ -119,19 +119,30 @@ struct dwarf_stack_value
>     its current state and its callbacks.  */
>  struct dwarf_expr_context
>  {
> -  dwarf_expr_context (dwarf2_per_objfile *per_objfile);
> +  /* We should ever only pass in the PER_OBJFILE, while the ADDR_SIZE
> +     information should be retrievable from there.  The PER_OBJFILE
> +     contains a pointer to the PER_BFD information anyway and the
> +     address size information must be the same for the whole BFD.  */
> +  dwarf_expr_context (struct dwarf2_per_objfile *per_objfile,
> +		      int addr_size);

I don't really understand the comment.  What do you mean by "We should
ever only pass..."?  In general, I don't find that the comment helps me
understand what the parameters are and do.  Try to change it to be more
concrete and straight to the point.

>    virtual ~dwarf_expr_context () = default;
>  
>    void push_address (CORE_ADDR value, bool in_stack_memory);
> -  void eval (const gdb_byte *addr, size_t len);
>  
> -  /* Fetch the result of the expression evaluation in a form of
> -     a struct value, where TYPE, SUBOBJ_TYPE and SUBOBJ_OFFSET
> -     describe the source level representation of that result.  */
> -  struct value *fetch_result (struct type *type = nullptr,
> -			      struct type *subobj_type = nullptr,
> -			      LONGEST subobj_offset = 0);
> +  /* Evaluate the expression at ADDR (LEN bytes long) in a given PER_CU
> +     FRAME context.  Where TYPE, SUBOBJ_TYPE and SUBOBJ_OFFSET describe
> +     expected struct value representation of the evaluation result.
> +     The ADDR_INFO property can be specified to override the range of
> +     memory addresses with the passed in buffer.  */

It sounds like you are missing a word between PER_CU and FRAME.

The sentence starting with "Where" sounds strange, I'd remove the
"Where" to make it a simple statement.

It sounds like you are missing a "the" between "describe" and
"expected".

> +  struct value *evaluate (const gdb_byte *addr, size_t len,
> +			  struct dwarf2_per_cu_data *per_cu,
> +			  struct frame_info *frame,
> +			  const struct property_addr_info *addr_info = nullptr,
> +			  struct type *type = nullptr,
> +			  struct type *subobj_type = nullptr,
> +			  LONGEST subobj_offset = 0);

Note to self that ADDR + LEN would be good candidates to become a
gdb::array_view later.

>  
> +private:
>    /* The stack of values.  */
>    std::vector<dwarf_stack_value> stack;

If all these fields become private, it would be good to rename them to
add the `m_` prefix.  It could be done at the end (if not already done
later in the series), to avoid the conflicts.

Simon