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

Simon Marchi via Gdb-patches
2021-04-28T02:45:32Z

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