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

[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>
> 
> 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