[PATCH] gdb: unify trivial_entry_value handling in DW_OP_entry_value

[Andrew Burgess <aburgess@redhat.com>]

This commit simplifies the DW_OP_entry_value handling in dwarf2/expr.c
for the case where trivial_entry_value is true.

Currently GDB can handle two different DW_OP_entry_value DWARF
expressions, DW_OP_reg or DW_OP_breg/DW_OP_deref, and the current
trivial_entry_value is also split in two to mirror these two cases.

The trivial_entry_value function returns true when the inferior is
stopped at the first instruction of a function.  This means that the
value of the DW_OP_entry_value expression is just the current value of
that DWARF expression.

This commit moves the trivial_entry_value handling to earlier in the
DW_OP_entry_value case statement block, and unifies the existing two
cases.

The new code performs a recursive evaluation of the DW_OP_entry_value
DWARF expression.

I took the m_stack save and restore idea from how DW_OP_fbreg is
handled, though I used SCOPE_EXIT for the restore so that the restore
happens even on the exception case.

Also, unlike the fbreg case I also save and restore m_location as
DW_OP_entry_value could be in the middle of a complex DWARF expression
which has already set m_location, and m_pieces as we could also be in
the middle of building a composite value.

Finally, the result is pulled from m_stack by a call to fetch_result.
The DWARF 5 spec says this about DW_OP_entry_value:

  The DW_OP_entry_value operation pushes the value that the described
  location held upon entering the current subprogram. It has two
  operands: an unsigned LEB128 length, followed by a block containing
  a DWARF expression or a register location description (see Section
  2.6.1.1.3 on page 39).  The length operand specifies the length in
  bytes of the block. If the block contains a DWARF expression, the
  DWARF expression is evaluated as if it had been evaluated upon
  entering the current subprogram. The DWARF expression assumes no
  values are present on the DWARF stack initially and results in
  exactly one value being pushed on the DWARF stack when completed. If
  the block contains a register location description,
  DW_OP_entry_value pushes the value that register had upon entering
  the current subprogram.

From this, and from how GDB currently handles DW_OP_breg/DW_OP_deref
within the DW_OP_entry_value case, I believe that this means there are
two possible results from a DW_OP_entry_value expression:

  (1) The result is left on the DWARF expression stack.

  (2) The result is a register location description, in which case the
      result is read from the register.

Based on #1 I initially wanted to pass as_lval=false to fetch_result,
passing as_lval=false means that m_location is forced to be
DWARF_VALUE_STACK.  However, this causes a problem when m_location is
initially DWARF_VALUE_REGISTER, changing to DWARF_VALUE_STACK means
that GDB reads the register number from the stack, which is never what
we want.

This got me looking at how as_lval is handled in fetch_result, and I'm
not convinced that the current handling makes much sense.  Converting
from DWARF_VALUE_MEMORY to DWARF_VALUE_STACK is fine, this causes GDB
to read the result from the expression stack rather than treating the
value on the stack as a memory location, but for DWARF_VALUE_REGISTER,
DWARF_VALUE_LITERAL, DWARF_VALUE_OPTIMIZED_OUT, and
DWARF_VALUE_IMPLICIT_POINTER converting to DWARF_VALUE_STACK doesn't
really work as there either isn't a value on the stack, or the value
on the stack is an internal detail, e.g. the DWARF register number for
DWARF_VALUE_REGISTER.

I did consider changing this existing code block:

  if (!as_lval)
    this->m_location = DWARF_VALUE_STACK;

to this:

  if (!as_lval && this->m_location == DWARF_VALUE_MEMORY)
    this->m_location = DWARF_VALUE_STACK;

But I figured it would actually be neater if I pushed the as_lval
handling down into the switch statement.  The switch is now structured
like this:

  switch (this->m_location)
    {
      ...
      case DWARF_VALUE_MEMORY:
        if (as_lval)
	  {
 	    ...
	    break;
	  }
        [[fallthrough]];
      case DWARF_VALUE_STACK:
        ...
    }

So when as_lval is false we treat it as DWARF_VALUE_STACK just like
before, but for any other location type we leave things unchanged.
This makes more sense to me.

Back in the DW_OP_entry_value handling we can now pass as_lval=false
to the fetch_result call, and we will correctly handle the two cases I
identified above, a value on the stack will be treated as an actual
value, while a register location description will result in the value
being read from the register.

There are no new tests added here.  This is a refactor.  As a smoke
test I was using gdb.dwarf2/dw2-entry-value*.exp.
---
 gdb/dwarf2/expr.c | 124 ++++++++++++++++++++++++++--------------------
 gdb/dwarf2/expr.h |   8 ++-
 2 files changed, 75 insertions(+), 57 deletions(-)

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index 934fda67ca6..15c69450b50 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -1031,11 +1031,6 @@ dwarf_expr_context::fetch_result (struct type *type, struct type *subobj_type,
     }
   else
     {
-      /* If AS_LVAL is false, means that the implicit conversion
-	 from a location description to value is expected.  */
-      if (!as_lval)
-	this->m_location = DWARF_VALUE_STACK;
-
       switch (this->m_location)
 	{
 	case DWARF_VALUE_REGISTER:
@@ -1070,21 +1065,26 @@ dwarf_expr_context::fetch_result (struct type *type, struct type *subobj_type,
 	  break;
 
 	case DWARF_VALUE_MEMORY:
-	  {
-	    struct type *ptr_type;
-	    CORE_ADDR address = this->fetch_address (0);
-	    bool in_stack_memory = this->fetch_in_stack_memory (0);
+	  /* If AS_LVAL is true, handle this memory location normally.
+	     Otherwise, fall through to DWARF_VALUE_STACK and treat
+	     the value on the DWARF stack as a plain value, not a
+	     memory location description.  */
+	  if (as_lval)
+	    {
+	      struct type *ptr_type;
+	      CORE_ADDR address = this->fetch_address (0);
+	      bool in_stack_memory = this->fetch_in_stack_memory (0);
 
-	    /* DW_OP_deref_size (and possibly other operations too) may
-	       create a pointer instead of an address.  Ideally, the
-	       pointer to address conversion would be performed as part
-	       of those operations, but the type of the object to
-	       which the address refers is not known at the time of
-	       the operation.  Therefore, we do the conversion here
-	       since the type is readily available.  */
+	      /* DW_OP_deref_size (and possibly other operations too) may
+		 create a pointer instead of an address.  Ideally, the
+		 pointer to address conversion would be performed as part
+		 of those operations, but the type of the object to
+		 which the address refers is not known at the time of
+		 the operation.  Therefore, we do the conversion here
+		 since the type is readily available.  */
 
-	    switch (subobj_type->code ())
-	      {
+	      switch (subobj_type->code ())
+		{
 		case TYPE_CODE_FUNC:
 		case TYPE_CODE_METHOD:
 		  ptr_type = builtin_type (arch)->builtin_func_ptr;
@@ -1092,16 +1092,16 @@ dwarf_expr_context::fetch_result (struct type *type, struct type *subobj_type,
 		default:
 		  ptr_type = builtin_type (arch)->builtin_data_ptr;
 		  break;
-	      }
-	    address = value_as_address (value_from_pointer (ptr_type, address));
-
-	    retval = value_at_lazy (subobj_type, address + subobj_offset,
-				    m_frame);
-	    if (in_stack_memory)
-	      retval->set_stack (true);
-	  }
-	  break;
+		}
+	      address = value_as_address (value_from_pointer (ptr_type, address));
 
+	      retval = value_at_lazy (subobj_type, address + subobj_offset,
+				      m_frame);
+	      if (in_stack_memory)
+		retval->set_stack (true);
+	      break;
+	    }
+	  [[fallthrough]];
 	case DWARF_VALUE_STACK:
 	  {
 	    value *val = this->fetch (0);
@@ -2357,26 +2357,51 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	      error (_("DW_OP_entry_value: too few bytes available."));
 
 	    auto entry_value_expr = gdb::make_array_view (op_ptr, len);
+	    op_ptr += len;
+
+	    if (trivial_entry_value (this->m_frame))
+	      {
+		/* Rather than create a whole new context, we simply backup
+		   the current stack locally and install a new empty stack,
+		   then reset it afterwards, effectively erasing whatever
+		   the recursive call put there.  */
+		std::vector<dwarf_stack_value> saved_stack = std::move (this->m_stack);
+		SCOPE_EXIT { this->m_stack = std::move (saved_stack); };
+		this->m_stack.clear ();
+
+		/* Backup the location as DW_OP_entry_value might appear as
+		   part of some complex expression that has already set a
+		   non-memory location.  We don't need to reset m_location
+		   back to a default value though as calling eval does that
+		   for us.  */
+		scoped_restore restore_m_location = make_scoped_restore (&this->m_location);
+
+		/* Backup, and arrange to restore, the saved pieces vector
+		   as this is referenced by the fetch_result call below.  It
+		   would be unusual for DW_OP_entry_value to appear within a
+		   composite location, but it's easy enough to handle this
+		   case correctly, so let's do that.  */
+		std::vector<dwarf_expr_piece> saved_pieces = std::move (this->m_pieces);
+		SCOPE_EXIT { this->m_pieces = std::move (saved_pieces); };
+		this->m_pieces.clear ();
+
+		/* Evaluate the entry expression.  */
+		eval (entry_value_expr);
+
+		/* The DWARF spec says the entry value block can contain
+		   either a DWARF expression or a register location
+		   description.  Pass as_lval=false here so that
+		   DWARF_VALUE_MEMORY locations will be treated as
+		   DWARF_VALUE_STACK, treating the expression as a value,
+		   not a location.  */
+		result_val = fetch_result (address_type, address_type, 0,
+					   false);
+		break;
+	      }
+
 	    kind_u.dwarf_reg = dwarf_block_to_dwarf_reg (entry_value_expr);
 	    if (kind_u.dwarf_reg != -1)
 	      {
-		op_ptr += len;
-
-		if (trivial_entry_value (this->m_frame))
-		  {
-		    /* We can assume that DW_OP_entry_value (expr) == expr.
-		       Handle DW_OP_regx, place register value on the
-		       stack.  */
-		    gdbarch *f_arch = get_frame_arch (this->m_frame);
-		    int dwarf_regnum = kind_u.dwarf_reg;
-		    int gdb_regnum
-		      = dwarf_reg_to_regnum_or_error (f_arch, dwarf_regnum);
-		    result_val
-		      = value_from_register (address_type, gdb_regnum,
-					     this->m_frame);
-		    break;
-		  }
-
 		this->push_dwarf_reg_entry_value (CALL_SITE_PARAMETER_DWARF_REG,
 						  kind_u,
 						  -1 /* deref_size */);
@@ -2389,17 +2414,6 @@ dwarf_expr_context::execute_stack_op (gdb::array_view<const gdb_byte> expr)
 	      {
 		if (deref_size == -1)
 		  deref_size = this->m_addr_size;
-		op_ptr += len;
-
-		if (trivial_entry_value (this->m_frame))
-		  {
-		    /* We can assume that DW_OP_entry_value (expr) == expr.
-		       Handle as DW_OP_bregx;DW_OP_deref_size.  */
-		    CORE_ADDR addr
-		      = read_addr_from_reg (this->m_frame, kind_u.dwarf_reg);
-		    result_val = this->deref (addr, deref_size);
-		    break;
-		  }
 
 		this->push_dwarf_reg_entry_value (CALL_SITE_PARAMETER_DWARF_REG,
 						  kind_u, deref_size);
diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
index 02b0e41f6fd..539880c831f 100644
--- a/gdb/dwarf2/expr.h
+++ b/gdb/dwarf2/expr.h
@@ -221,8 +221,12 @@ struct dwarf_expr_context
   /* 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.
-     AS_LVAL defines if the fetched struct value is expected to
-     be a value or a location description.  */
+
+     When AS_LVAL is false any memory location descriptions
+     (DWARF_VALUE_MEMORY) are treated as values on the stack
+     (i.e. handled as DWARF_VALUE_STACK).  Handling of any other
+     dwarf_value_location type is unchanged.  When AS_LVAL is true
+     then all dwarf_value_location types are handled as normal.  */
   value *fetch_result (struct type *type, struct type *subobj_type,
 		       LONGEST subobj_offset, bool as_lval);
 

base-commit: 21e7edb10530341334a02c873aa9f7f5aeb0138a
-- 
2.25.4