[v3,05/17] Move compilation unit info to dwarf_expr_context

Message ID 20210528154648.60881-6-zoran.zaric@amd.com
State New
Headers show
Series
  • DWARF expression evaluator design cleanup
Related show

Commit Message

Luis Machado via Gdb-patches May 28, 2021, 3:46 p.m.
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.

With this change, it also makes sense to always acquire ref_addr_size
information from the compilation unit context, considering that all
DWARF operations that refer to that information require a compilation
unit context to be present during their evaluation.

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.
        (dwarf_expr_context::get_addr_index): Remove method.
        (dwarf_expr_context::dwarf_variable_value): Remove method.
        (dwarf_expr_context::ref_addr_size): Remove member.
	* 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  | 76 ++++++++++++++++++++++++++++++++++++----------
 gdb/dwarf2/expr.h  | 29 +++++-------------
 gdb/dwarf2/frame.c | 11 -------
 gdb/dwarf2/loc.c   | 37 ++--------------------
 gdb/dwarf2/loc.h   |  8 +++++
 5 files changed, 79 insertions(+), 82 deletions(-)

-- 
2.17.1

Comments

Luis Machado via Gdb-patches June 9, 2021, 12:50 a.m. | #1
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c

> index ced78034fdf..d70f92f27bc 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.  */


This comment is also stale.

> +

> +static void

> +ensure_have_per_cu (struct dwarf2_per_cu_data *per_cu, const char* op_name)

> +{

> +  if (per_cu == nullptr)

> +    throw_error (GENERIC_ERROR,

> +		 _("%s evaluation requires a compilation unit."), op_name);

> +}

> +

>  /* See expr.h.  */

>  

>  CORE_ADDR

> @@ -201,6 +215,23 @@ 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)

> +{

> +  if (per_cu == nullptr)

> +    return builtin_type (this->gdbarch)->builtin_int;

> +

> +  struct type *result = dwarf2_get_die_type (die_cu_off, this->per_cu,

> +					     this->per_objfile);

> +

> +  if (result == nullptr)

> +    error (_("Could not find type for operation"));

> +

> +  return result;

> +}


The size check disappeared between the two versions, from you reply I
expected that it would stay the same.  Why was it removed?

Simon
Luis Machado via Gdb-patches June 11, 2021, 5:19 p.m. | #2
On 6/9/21 1:50 AM, Simon Marchi wrote:
> [CAUTION: External Email]

> 

>> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c

>> index ced78034fdf..d70f92f27bc 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.  */

> 

> This comment is also stale.


Thank you. This will be fixed in the next iteration.

> 

>> +

>> +static void

>> +ensure_have_per_cu (struct dwarf2_per_cu_data *per_cu, const char* op_name)

>> +{

>> +  if (per_cu == nullptr)

>> +    throw_error (GENERIC_ERROR,

>> +              _("%s evaluation requires a compilation unit."), op_name);

>> +}

>> +

>>   /* See expr.h.  */

>>

>>   CORE_ADDR

>> @@ -201,6 +215,23 @@ 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)

>> +{

>> +  if (per_cu == nullptr)

>> +    return builtin_type (this->gdbarch)->builtin_int;

>> +

>> +  struct type *result = dwarf2_get_die_type (die_cu_off, this->per_cu,

>> +                                          this->per_objfile);

>> +

>> +  if (result == nullptr)

>> +    error (_("Could not find type for operation"));

>> +

>> +  return result;

>> +}

> 

> The size check disappeared between the two versions, from you reply I

> expected that it would stay the same.  Why was it removed?

> 

> Simon

> 


As soon as the code that handles DW_OP_const_type operation specifics is 
moved to where that operation is evaluated, the size check also gets 
localized to that part of the code, so the 0 size case doesn't have 
meaning anymore.

Zoran

Patch

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index ced78034fdf..d70f92f27bc 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 (GENERIC_ERROR,
+		 _("%s evaluation requires a compilation unit."), op_name);
+}
+
 /* See expr.h.  */
 
 CORE_ADDR
@@ -201,6 +215,23 @@  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)
+{
+  if (per_cu == nullptr)
+    return builtin_type (this->gdbarch)->builtin_int;
+
+  struct type *result = dwarf2_get_die_type (die_cu_off, this->per_cu,
+					     this->per_objfile);
+
+  if (result == nullptr)
+    error (_("Could not find type for operation"));
+
+  return result;
+}
+
 /* Require that TYPE be an integral type; throw an exception if not.  */
 
 static void
@@ -697,14 +728,20 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 
 	case DW_OP_addrx:
 	case DW_OP_GNU_addr_index:
+	  ensure_have_per_cu (this->per_cu, "DW_OP_addrx");
+
 	  op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
-	  result = this->get_addr_index (uoffset);
+	  result = dwarf2_read_addr_index (this->per_cu, this->per_objfile,
+					   uoffset);
 	  result += this->per_objfile->objfile->text_section_offset ();
 	  result_val = value_from_ulongest (address_type, result);
 	  break;
 	case DW_OP_GNU_const_index:
+	  ensure_have_per_cu (this->per_cu, "DW_OP_GNU_const_index");
+
 	  op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
-	  result = this->get_addr_index (uoffset);
+	  result = dwarf2_read_addr_index (this->per_cu, this->per_objfile,
+					   uoffset);
 	  result_val = value_from_ulongest (address_type, result);
 	  break;
 
@@ -834,15 +871,14 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	case DW_OP_GNU_implicit_pointer:
 	  {
 	    int64_t len;
+	    ensure_have_per_cu (this->per_cu, "DW_OP_implicit_pointer");
 
-	    if (this->ref_addr_size == -1)
-	      error (_("DWARF-2 expression error: DW_OP_implicit_pointer "
-		       "is not allowed in frame context"));
+	    int ref_addr_size = this->per_cu->ref_addr_size ();
 
 	    /* The referred-to DIE of sect_offset kind.  */
-	    this->len = extract_unsigned_integer (op_ptr, this->ref_addr_size,
-						 byte_order);
-	    op_ptr += this->ref_addr_size;
+	    this->len = extract_unsigned_integer (op_ptr, ref_addr_size,
+						  byte_order);
+	    op_ptr += ref_addr_size;
 
 	    /* The byte offset into the data.  */
 	    op_ptr = safe_read_sleb128 (op_ptr, op_end, &len);
@@ -1008,7 +1044,7 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	      {
 		op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
 		cu_offset type_die_cu_off = (cu_offset) uoffset;
-		type = get_base_type (type_die_cu_off, 0);
+		type = get_base_type (type_die_cu_off);
 	      }
 	    else
 	      type = address_type;
@@ -1331,13 +1367,17 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 
 	case DW_OP_GNU_variable_value:
 	  {
+	    ensure_have_per_cu (this->per_cu, "DW_OP_GNU_variable_value");
+	    int ref_addr_size = this->per_cu->ref_addr_size ();
+
 	    sect_offset sect_off
 	      = (sect_offset) extract_unsigned_integer (op_ptr,
-							this->ref_addr_size,
+							ref_addr_size,
 							byte_order);
-	    op_ptr += this->ref_addr_size;
-	    result_val = value_cast (address_type,
-				     this->dwarf_variable_value (sect_off));
+	    op_ptr += ref_addr_size;
+	    result_val = sect_variable_value (sect_off, this->per_cu,
+					      this->per_objfile);
+	    result_val = value_cast (address_type, result_val);
 	  }
 	  break;
 	
@@ -1407,7 +1447,11 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	    data = op_ptr;
 	    op_ptr += n;
 
-	    type = get_base_type (type_die_cu_off, n);
+	    type = get_base_type (type_die_cu_off);
+
+	    if (TYPE_LENGTH (type) != n)
+	      error (_("DW_OP_const_type has different sizes for type and data"));
+
 	    result_val = value_from_contents (type, data);
 	  }
 	  break;
@@ -1421,7 +1465,7 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &uoffset);
 	    cu_offset type_die_cu_off = (cu_offset) uoffset;
 
-	    type = get_base_type (type_die_cu_off, 0);
+	    type = get_base_type (type_die_cu_off);
 	    result_val = this->get_reg_value (type, reg);
 	  }
 	  break;
@@ -1439,7 +1483,7 @@  dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	    if (to_underlying (type_die_cu_off) == 0)
 	      type = address_type;
 	    else
-	      type = get_base_type (type_die_cu_off, 0);
+	      type = get_base_type (type_die_cu_off);
 
 	    result_val = fetch (0);
 	    pop ();
diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
index ba17dc40165..f14070de522 100644
--- a/gdb/dwarf2/expr.h
+++ b/gdb/dwarf2/expr.h
@@ -137,10 +137,6 @@  struct dwarf_expr_context
   /* Target address size in bytes.  */
   int addr_size = 0;
 
-  /* DW_FORM_ref_addr size in bytes.  If -1 DWARF is executed from a frame
-     context and operations depending on DW_FORM_ref_addr are not allowed.  */
-  int ref_addr_size = 0;
-
   /* The current depth of dwarf expression recursion, via DW_OP_call*,
      DW_OP_fbreg, DW_OP_push_object_address, etc., and the maximum
      depth we'll tolerate before raising an error.  */
@@ -189,6 +185,9 @@  struct dwarf_expr_context
   /* Frame information used for the evaluation.  */
   struct frame_info *frame = nullptr;
 
+  /* Compilation unit used for the evaluation.  */
+  struct dwarf2_per_cu_data *per_cu = nullptr;
+
   /* Read LENGTH bytes at ADDR into BUF.  */
   virtual void read_mem (gdb_byte *buf, CORE_ADDR addr, size_t length) = 0;
 
@@ -208,19 +207,6 @@  struct dwarf_expr_context
      subroutine.  */
   virtual void dwarf_call (cu_offset die_cu_off) = 0;
 
-  /* Execute "variable value" operation on the DIE at SECT_OFF.  */
-  virtual struct value *dwarf_variable_value (sect_offset sect_off) = 0;
-
-  /* Return the base type given by the indicated DIE at DIE_CU_OFF.
-     This can throw an exception if the DIE is invalid or does not
-     represent a base type.  SIZE is non-zero if this function should
-     verify that the resulting type has the correct size.  */
-  virtual struct type *get_base_type (cu_offset die_cu_off, int size)
-  {
-    /* Anything will do.  */
-    return builtin_type (this->gdbarch)->builtin_int;
-  }
-
   /* Push on DWARF stack an entry evaluated for DW_TAG_call_site's
      parameter matching KIND and KIND_U at the caller of specified BATON.
      If DEREF_SIZE is not -1 then use DW_AT_call_data_value instead of
@@ -229,10 +215,6 @@  struct dwarf_expr_context
 					   union call_site_parameter_u kind_u,
 					   int deref_size) = 0;
 
-  /* Return the address indexed by DW_OP_addrx or DW_OP_GNU_addr_index.
-     This can throw an exception if the index is out of range.  */
-  virtual CORE_ADDR get_addr_index (unsigned int index) = 0;
-
   /* Return the `object address' for DW_OP_push_object_address.  */
   virtual CORE_ADDR get_object_address () = 0;
 
@@ -255,6 +237,11 @@  struct dwarf_expr_context
      START and LENGTH.  The result must be live until the current
      expression evaluation is complete.  */
   void get_frame_base (const gdb_byte **start, size_t *length);
+
+  /* Return the base type given by the indicated DIE at DIE_CU_OFF.
+     This can throw an exception if the DIE is invalid or does not
+     represent a base type.  */
+  struct type *get_base_type (cu_offset die_cu_off);
 };
 
 /* Return the value of register number REG (a DWARF register number),
diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
index c543848cda6..492c1ec9d7c 100644
--- a/gdb/dwarf2/frame.c
+++ b/gdb/dwarf2/frame.c
@@ -259,16 +259,6 @@  class dwarf_expr_executor : public dwarf_expr_context
     invalid ("DW_OP_call*");
   }
 
-  struct value *dwarf_variable_value (sect_offset sect_off) override
-  {
-    invalid ("DW_OP_GNU_variable_value");
-  }
-
-  CORE_ADDR get_addr_index (unsigned int index) override
-  {
-    invalid ("DW_OP_addrx or DW_OP_GNU_addr_index");
-  }
-
  private:
 
   void invalid (const char *op) ATTRIBUTE_NORETURN
@@ -290,7 +280,6 @@  execute_stack_op (const gdb_byte *exp, ULONGEST len, int addr_size,
   ctx.frame = this_frame;
   ctx.gdbarch = get_frame_arch (this_frame);
   ctx.addr_size = addr_size;
-  ctx.ref_addr_size = -1;
 
   ctx.push_address (initial, initial_in_stack_memory);
   ctx.eval (exp, len);
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 7b9f06bd5a9..f5bd5c57c61 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -620,12 +620,10 @@  per_cu_dwarf_call (struct dwarf_expr_context *ctx, cu_offset die_offset,
   ctx->eval (block.data, block.size);
 }
 
-/* Given context CTX, section offset SECT_OFF, and compilation unit
-   data PER_CU, execute the "variable value" operation on the DIE
-   found at SECT_OFF.  */
+/* See loc.h.  */
 
-static struct value *
-sect_variable_value (struct dwarf_expr_context *ctx, sect_offset sect_off,
+struct value *
+sect_variable_value (sect_offset sect_off,
 		     dwarf2_per_cu_data *per_cu,
 		     dwarf2_per_objfile *per_objfile)
 {
@@ -654,7 +652,6 @@  class dwarf_evaluate_loc_desc : public dwarf_expr_context
     : dwarf_expr_context (per_objfile)
   {}
 
-  struct dwarf2_per_cu_data *per_cu;
   CORE_ADDR obj_address;
 
   /* Helper function for dwarf2_evaluate_loc_desc.  Computes the PC for
@@ -680,32 +677,6 @@  class dwarf_evaluate_loc_desc : public dwarf_expr_context
     per_cu_dwarf_call (this, die_offset, per_cu, per_objfile);
   }
 
-  /* Helper interface of sect_variable_value for
-     dwarf2_evaluate_loc_desc.  */
-
-  struct value *dwarf_variable_value (sect_offset sect_off) override
-  {
-    return sect_variable_value (this, sect_off, per_cu, per_objfile);
-  }
-
-  struct type *get_base_type (cu_offset die_offset, int size) override
-  {
-    struct type *result = dwarf2_get_die_type (die_offset, per_cu, per_objfile);
-    if (result == NULL)
-      error (_("Could not find type for DW_OP_const_type"));
-    if (size != 0 && TYPE_LENGTH (result) != size)
-      error (_("DW_OP_const_type has different sizes for type and data"));
-    return result;
-  }
-
-  /* Callback function for dwarf2_evaluate_loc_desc.
-     Fetch the address indexed by DW_OP_addrx or DW_OP_GNU_addr_index.  */
-
-  CORE_ADDR get_addr_index (unsigned int index) override
-  {
-    return dwarf2_read_addr_index (per_cu, per_objfile, index);
-  }
-
   /* Callback function for get_object_address. Return the address of the VLA
      object.  */
 
@@ -2195,7 +2166,6 @@  dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
 
   ctx.gdbarch = per_objfile->objfile->arch ();
   ctx.addr_size = per_cu->addr_size ();
-  ctx.ref_addr_size = per_cu->ref_addr_size ();
 
   try
     {
@@ -2462,7 +2432,6 @@  dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
 
   ctx.gdbarch = per_objfile->objfile->arch ();
   ctx.addr_size = dlbaton->per_cu->addr_size ();
-  ctx.ref_addr_size = dlbaton->per_cu->ref_addr_size ();
 
   if (push_initial_value)
     ctx.push_address (ctx.obj_address, false);
diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
index 2943baf91d2..a729b370e2e 100644
--- a/gdb/dwarf2/loc.h
+++ b/gdb/dwarf2/loc.h
@@ -53,6 +53,14 @@  extern void func_get_frame_base_dwarf_block (struct symbol *framefunc,
 					     const gdb_byte **start,
 					     size_t *length);
 
+/* Given section offset SECT_OFF, and compilation unit data
+   PER_CU, execute the "variable value" operation on the DIE
+   found at SECT_OFF.  */
+
+struct value *sect_variable_value (sect_offset sect_off,
+				   dwarf2_per_cu_data *per_cu,
+				   dwarf2_per_objfile *per_objfile);
+
 /* Evaluate a location description, starting at DATA and with length
    SIZE, to find the current location of variable of TYPE in the context
    of FRAME.  */