Re: [PATCH 05/43] Move compilation unit info to dwarf_expr_context

Simon Marchi via Gdb-patches
2021-04-27T02:58:16Z

On 2021-03-01 9:45 a.m., Zoran Zaric via Gdb-patches wrote:
> From: Zoran Zaric <zoran.zaric@amd.com>
>
> This patch moves the compilation unit context information and support
> from dwarf_expr_executor and dwarf_evaluate_loc_desc to
> dwarf_expr_context evaluator. The idea is to report an error when a
> given operation requires a compilation unit information to be resolved,
> which is not available.
>
> gdb/ChangeLog:
>
> * dwarf2/expr.c (ensure_have_per_cu): New function.
> (dwarf_expr_context::dwarf_expr_context): Add compilation unit
> context information.
> (dwarf_expr_context::get_base_type): Move from
> dwarf_evaluate_loc_desc.
> (dwarf_expr_context::get_addr_index): Remove method.
> (dwarf_expr_context::dwarf_variable_value): Remove method.
> (dwarf_expr_context::execute_stack_op): Call compilation unit
> context info check. Inline get_addr_index and
> dwarf_variable_value methods.
> * dwarf2/expr.h (struct dwarf_expr_context): Add compilation
> context info. Remove get_addr_index and dwarf_variable_value.
> * dwarf2/frame.c (dwarf_expr_executor::get_addr_index): Remove
> method.
> (dwarf_expr_executor::dwarf_variable_value): Remove method.
> * dwarf2/loc.c (sect_variable_value): Expose function.
> (dwarf_evaluate_loc_desc::get_addr_index): Remove method.
> (dwarf_evaluate_loc_desc::dwarf_variable_value): Remove method.
> (class dwarf_evaluate_loc_desc): Move compilation unit context
> information to dwarf_expr_context class.
> * dwarf2/loc.h (sect_variable_value): Expose function.
> ---
> gdb/dwarf2/expr.c | 55 +++++++++++++++++++++++++++++++++++++++-------
> gdb/dwarf2/expr.h | 26 ++++++++--------------
> gdb/dwarf2/frame.c | 10 ---------
> gdb/dwarf2/loc.c | 35 +++--------------------------
> gdb/dwarf2/loc.h | 8 +++++++
> 5 files changed, 67 insertions(+), 67 deletions(-)
>
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index 6b5cba6b7b4..5a0a4a4299c 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -72,6 +72,20 @@ ensure_have_frame (struct frame_info *frame, const char *op_name)
> _("%s evaluation requires a frame."), op_name);
> }
>
> +/* Ensure that a PER_CU is defined and throw an exception otherwise.
> +
> + Throwing NOT_AVAILABLE_ERROR error so that a client can chose
> + to react differently if the evaluation ended because there
> + was a missing context information. */
> +
> +static void
> +ensure_have_per_cu (struct dwarf2_per_cu_data *per_cu, const char* op_name)
> +{
> + if (per_cu == nullptr)
> + throw_error (NOT_AVAILABLE_ERROR,
> + _("%s evaluation requires a compilation unit."), op_name);
> +}

Same comment as earlier about NOT_AVAILABLE_ERROR, and about wrapping
per_cu to force accessing it through a getter that throws if it is
nullptr (whatever we end up doing for the frame should be done here).

> +
> /* See expr.h. */
>
> CORE_ADDR
> @@ -201,6 +215,25 @@ dwarf_expr_context::get_frame_base (const gdb_byte **start,
> start, length);
> }
>
> +/* See expr.h. */
> +
> +struct type *
> +dwarf_expr_context::get_base_type (cu_offset die_cu_off, int size)
> +{
> + if (per_cu == nullptr)
> + return builtin_type (this->gdbarch)->builtin_int;

Ok, so this seems to be a case where we need to check if per_cu is
nullptr or not. The getter that throws won't work here, but there could
be an `has_per_cu ()` method on the side.

> +
> + struct type *result = dwarf2_get_die_type (die_cu_off, per_cu, per_objfile);
> +
> + if (result == NULL)
> + error (_("Could not find type for DW_OP_const_type"));

Might as well change this NULL for nullptr.

> +
> + if (size != 0 && TYPE_LENGTH (result) != size)
> + error (_("DW_OP_const_type has different sizes for type and data"));
> +
> + return result;
> +}

I find get_base_type a bit awkward. Do you think you could split it in
two:

- get_base_type
- get_base_type_check_size

... where get_base_type_check_size uses get_base_type? The callers that
don't need the size checking wouldn't need to pass 0.

I also find it awkward that get_base_type hardcodes DW_OP_const_type in
its error message, given that it's used in the context of many other
operators. In the end we understand that DW_OP_const_type is the only
case that passes size != 0, so it's the only case where that error can
possibly be thrown. But if you could pass the op name as a parameter,
like you did for the ensure_have_per_cu function, I think it would make
things better.

Or, since that size checking is only used when handling
DW_OP_const_type, we could in-line that code in the caller and not
bother with having get_base_type_check_size.

> @@ -834,10 +873,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
> case DW_OP_GNU_implicit_pointer:
> {
> int64_t len;
> -
> - if (this->ref_addr_size == -1)
> - error (_("DWARF-2 expression error: DW_OP_implicit_pointer "
> - "is not allowed in frame context"));
> + ensure_have_per_cu (per_cu, "DW_OP_implicit_pointer");
>
> /* The referred-to DIE of sect_offset kind. */
> this->len = extract_unsigned_integer (op_ptr, this->ref_addr_size,

This case would be a bit weird with my suggestion of per_cu getter that
throws if per_cu is nullptr, because nothing actually uses per_cu here.
But, I think that in the end we could get rid of the
`this->ref_addr_size` field, and use `this->per_cu ()->ref_addr_size
()`.

Simon