[v3,15/17] Make DWARF evaluator return a single struct value

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

Commit Message

Weimin Pan via Gdb-patches May 28, 2021, 3:46 p.m.
The patch is addressing the issue of class users writing and reading
the internal data of the dwarf_expr_context class.

At this point, all conditions are met for the DWARF evaluator to return
an evaluation result in a form of a single struct value object.

gdb/ChangeLog:

	* dwarf2/expr.c (pieced_value_funcs): Chenge to static
	function.
	(allocate_piece_closure): Change to static function.
	(dwarf_expr_context::fetch_result): New function.
	* dwarf2/expr.h (struct piece_closure): Remove declaration.
	(struct dwarf_expr_context): fetch_result new declaration.
	fetch, fetch_address and fetch_in_stack_memory members move
	to private.
	(allocate_piece_closure): Remove.
	* dwarf2/frame.c (execute_stack_op): Change to use
	fetch_result.
	* dwarf2/loc.c (dwarf2_evaluate_loc_desc_full): Change to use
	fetch_result.
	(dwarf2_locexpr_baton_eval): Change to use fetch_result.
        * dwarf2/loc.h (invalid_synthetic_pointer): Expose function.
---
 gdb/dwarf2/expr.c  | 160 ++++++++++++++++++++++++++++++++++++-
 gdb/dwarf2/expr.h  |  23 +++---
 gdb/dwarf2/frame.c |  19 ++---
 gdb/dwarf2/loc.c   | 195 ++++++---------------------------------------
 gdb/dwarf2/loc.h   |   5 ++
 5 files changed, 204 insertions(+), 198 deletions(-)

-- 
2.17.1

Comments

Tom Tromey Aug. 11, 2021, 5 p.m. | #1
>>>>> "Zoran" == Zoran Zaric via Gdb-patches <gdb-patches@sourceware.org> writes:


Zoran> The patch is addressing the issue of class users writing and reading
Zoran> the internal data of the dwarf_expr_context class.

Zoran> At this point, all conditions are met for the DWARF evaluator to return
Zoran> an evaluation result in a form of a single struct value object.

Hi.  On an internal test case, using an arm-elf target, this patch
causes a regression.  (It doesn't happen for any of the other cross
targets that I test when importing upstream gdb.)

I don't know if there's an upstream gdb test case showing the same
problem... I can only really run native tests with dejagnu AFAIK.

Anyway the failure manifests like this:

    Breakpoint 1, file_1.export_1 (param_1=<error reading variable: Unable to access DWARF register number 64>, str="test_gdb_fpregs") at [...]file_1.adb:5

Whereas when it works it looks like:

    Breakpoint 1, file_1.export_1 (param_1=99.0, str=...) at [...]/file_1.adb:5

I think the difference is that the new code does:

Zoran> +/* See expr.h.  */
Zoran> +
Zoran> +struct value *
Zoran> +dwarf_expr_context::fetch_result (struct type *type,
Zoran> +				  struct type *subobj_type,
Zoran> +				  LONGEST subobj_offset)
Zoran> +{
[...]
Zoran> +	case DWARF_VALUE_REGISTER:
Zoran> +	  {
Zoran> +	    int dwarf_regnum
Zoran> +	      = longest_to_int (value_as_long (this->fetch (0)));
Zoran> +	    int gdb_regnum = dwarf_reg_to_regnum_or_error (this->gdbarch,
Zoran> +							   dwarf_regnum);

... whereas the old code did:

Zoran> -      switch (ctx.location)
Zoran> -	{
Zoran> -	case DWARF_VALUE_REGISTER:
Zoran> -	  {
Zoran> -	    struct gdbarch *arch = get_frame_arch (frame);
Zoran> -	    int dwarf_regnum
Zoran> -	      = longest_to_int (value_as_long (ctx.fetch (0)));
Zoran> -	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, dwarf_regnum);


That is, the old code used get_frame_arch, which is different from
this->gdbarch.

Now, it seems to me that when working with registers, the frame's
architecture is the one to use.  Other spots seem to use
read_addr_from_reg, which does this same thing.

I'm going to try out that change for a number of cross targets and see
if it looks ok.  Meanwhile I thought I'd send this email in case anyone
else has more insight.

thanks,
Tom
Tom Tromey Aug. 12, 2021, 6:03 p.m. | #2
Zoran> At this point, all conditions are met for the DWARF evaluator to return
Zoran> an evaluation result in a form of a single struct value object.

Tom> Hi.  On an internal test case, using an arm-elf target, this patch
Tom> causes a regression.  (It doesn't happen for any of the other cross
Tom> targets that I test when importing upstream gdb.)

I've tested this patch, plus another one related to this series that I
didn't bring up yesterday.

They are here:

    https://sourceware.org/pipermail/gdb-patches/2021-August/181432.html

I'd appreciate any input you might have.

thanks,
Tom
Weimin Pan via Gdb-patches Aug. 13, 2021, 9:57 a.m. | #3
Hi Tom,

Thanks for reporting this, I will look into it ASAP.

I am curious about how can the frame arch and the arch given for the 
evaluation of the expression while focused on the same frame be different.

Something suspicious is happening here, but I first need to find a way 
to reproduce this.

Zoran

On 8/12/21 7:03 PM, Tom Tromey wrote:
> [CAUTION: External Email]

> 

> Zoran> At this point, all conditions are met for the DWARF evaluator to return

> Zoran> an evaluation result in a form of a single struct value object.

> 

> Tom> Hi.  On an internal test case, using an arm-elf target, this patch

> Tom> causes a regression.  (It doesn't happen for any of the other cross

> Tom> targets that I test when importing upstream gdb.)

> 

> I've tested this patch, plus another one related to this series that I

> didn't bring up yesterday.

> 

> They are here:

> 

>      https://sourceware.org/pipermail/gdb-patches/2021-August/181432.html

> 

> I'd appreciate any input you might have.

> 

> thanks,

> Tom

>
Tom Tromey Aug. 13, 2021, 4:59 p.m. | #4
Zoran> Thanks for reporting this, I will look into it ASAP.

Take a look at the patches I sent.

Zoran> I am curious about how can the frame arch and the arch given for the
Zoran> evaluation of the expression while focused on the same frame be
Zoran> different.

The arch that's currently used here comes from:

  gdbarch *arch = this->m_per_objfile->objfile->arch ();

The objfile's arch is just whatever gdb guesses from the executable.
I think it may be fairly non-specific, especially if the architecture
has a lot of variants.  In a case like this, the arch returned by
gdbserver, say, may include more details -- the specific registers
available, etc.

Tom
Weimin Pan via Gdb-patches Aug. 13, 2021, 5:57 p.m. | #5
On 8/13/21 5:59 PM, Tom Tromey wrote:
> [CAUTION: External Email]

> 

> Zoran> Thanks for reporting this, I will look into it ASAP.

> 

> Take a look at the patches I sent.


Thank you Tom, what you just said makes sense. The patches look good to 
me considering that information.

I will need to check (in my later patches) where the architecture is 
used and if I have the frame arch I should probably prioritize that.

Zoran
Tom Tromey Aug. 16, 2021, 3:37 p.m. | #6
Tom> Take a look at the patches I sent.

Zoran> Thank you Tom, what you just said makes sense. The patches look good
Zoran> to me considering that information.

Thanks.  I'm going to check them in now.

Tom
Weimin Pan via Gdb-patches Aug. 16, 2021, 4:05 p.m. | #7
On 8/16/21 4:37 PM, Tom Tromey wrote:
> [CAUTION: External Email]

> 

> Tom> Take a look at the patches I sent.

> 

> Zoran> Thank you Tom, what you just said makes sense. The patches look good

> Zoran> to me considering that information.

> 

> Thanks.  I'm going to check them in now.

> 

> Tom

> 


These look OK to me, but what I would like to try is to always use the 
frame architecture if the frame context is available for the evaluation. 
This should make all the location descriptions behave in a consistent way.

Would you mind me adding a patch that does that a bit later? Or do you 
see a problem with that implementation?

Regards,
Zoran
Tom Tromey Aug. 16, 2021, 5:32 p.m. | #8
Zoran> These look OK to me, but what I would like to try is to always use the
Zoran> frame architecture if the frame context is available for the
Zoran> evaluation. This should make all the location descriptions behave in a
Zoran> consistent way.

Zoran> Would you mind me adding a patch that does that a bit later? Or do you
Zoran> see a problem with that implementation?

I think it would probably be fine.  It's hard to be certain -- like, if
there is a situation where an expression only needs read-only memory,
then this could be evaluated without needing a frame (or running
inferior) and so then, in theory, this patch could cause a regression.
But does this case occur and does it matter?

The other thing is, looking at dwarf_expr_context::fetch_result, most
uses of the architecture are basic:

	    if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG)

I suppose it's possible for the objfile arch and the frame arch to
disagree about byte order, but does it happen in practice?

Maybe it would be difficult to test such a patch.  Though I suppose I'd
be alright with not having a test case for this particular corner.
Not sure what others may think.

Tom

Patch

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index 022a97f1a07..bc74fc1e582 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -125,9 +125,10 @@  struct piece_closure
   struct frame_id frame_id;
 };
 
-/* See expr.h.  */
+/* Allocate a closure for a value formed from separately-described
+   PIECES.  */
 
-struct piece_closure *
+static struct piece_closure *
 allocate_piece_closure (dwarf2_per_cu_data *per_cu,
 			dwarf2_per_objfile *per_objfile,
 			std::vector<dwarf_expr_piece> &&pieces,
@@ -623,7 +624,7 @@  free_pieced_value_closure (struct value *v)
 }
 
 /* Functions for accessing a variable described by DW_OP_piece.  */
-const struct lval_funcs pieced_value_funcs = {
+static const struct lval_funcs pieced_value_funcs = {
   read_pieced_value,
   write_pieced_value,
   indirect_pieced_value,
@@ -879,6 +880,159 @@  dwarf_expr_context::push_dwarf_reg_entry_value
   this->eval (data_src, size);
 }
 
+/* See expr.h.  */
+
+struct value *
+dwarf_expr_context::fetch_result (struct type *type,
+				  struct type *subobj_type,
+				  LONGEST subobj_offset)
+{
+  struct value *retval = nullptr;
+
+  if (type == nullptr)
+    type = address_type ();
+
+  if (subobj_type == nullptr)
+    subobj_type = type;
+
+  if (this->pieces.size () > 0)
+    {
+      ULONGEST bit_size = 0;
+
+      for (dwarf_expr_piece &piece : this->pieces)
+	bit_size += piece.size;
+      /* Complain if the expression is larger than the size of the
+	 outer type.  */
+      if (bit_size > 8 * TYPE_LENGTH (type))
+	invalid_synthetic_pointer ();
+
+      piece_closure *c
+	= allocate_piece_closure (this->per_cu, this->per_objfile,
+				  std::move (this->pieces), this->frame);
+      retval = allocate_computed_value (subobj_type,
+					&pieced_value_funcs, c);
+      set_value_offset (retval, subobj_offset);
+    }
+  else
+    {
+      switch (this->location)
+	{
+	case DWARF_VALUE_REGISTER:
+	  {
+	    int dwarf_regnum
+	      = longest_to_int (value_as_long (this->fetch (0)));
+	    int gdb_regnum = dwarf_reg_to_regnum_or_error (this->gdbarch,
+							   dwarf_regnum);
+
+	    if (subobj_offset != 0)
+	      error (_("cannot use offset on synthetic pointer to register"));
+
+	    gdb_assert (this->frame != NULL);
+
+	    retval = value_from_register (subobj_type, gdb_regnum,
+					  this->frame);
+	    if (value_optimized_out (retval))
+	      {
+		/* This means the register has undefined value / was
+		   not saved.  As we're computing the location of some
+		   variable etc. in the program, not a value for
+		   inspecting a register ($pc, $sp, etc.), return a
+		   generic optimized out value instead, so that we show
+		   <optimized out> instead of <not saved>.  */
+		struct value *tmp = allocate_value (subobj_type);
+		value_contents_copy (tmp, 0, retval, 0,
+				     TYPE_LENGTH (subobj_type));
+		retval = tmp;
+	      }
+	  }
+	  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);
+
+	    /* 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 ())
+	      {
+		case TYPE_CODE_FUNC:
+		case TYPE_CODE_METHOD:
+		  ptr_type = builtin_type (this->gdbarch)->builtin_func_ptr;
+		  break;
+		default:
+		  ptr_type = builtin_type (this->gdbarch)->builtin_data_ptr;
+		  break;
+	      }
+	    address = value_as_address (value_from_pointer (ptr_type, address));
+
+	    retval = value_at_lazy (subobj_type,
+				    address + subobj_offset);
+	    if (in_stack_memory)
+	      set_value_stack (retval, 1);
+	  }
+	  break;
+
+	case DWARF_VALUE_STACK:
+	  {
+	    struct value *value = this->fetch (0);
+	    size_t n = TYPE_LENGTH (value_type (value));
+	    size_t len = TYPE_LENGTH (subobj_type);
+	    size_t max = TYPE_LENGTH (type);
+
+	    if (subobj_offset + len > max)
+	      invalid_synthetic_pointer ();
+
+	    retval = allocate_value (subobj_type);
+
+	    /* The given offset is relative to the actual object.  */
+	    if (gdbarch_byte_order (this->gdbarch) == BFD_ENDIAN_BIG)
+	      subobj_offset += n - max;
+
+	    memcpy (value_contents_raw (retval),
+		    value_contents_all (value) + subobj_offset, len);
+	  }
+	  break;
+
+	case DWARF_VALUE_LITERAL:
+	  {
+	    size_t n = TYPE_LENGTH (subobj_type);
+
+	    if (subobj_offset + n > this->len)
+	      invalid_synthetic_pointer ();
+
+	    retval = allocate_value (subobj_type);
+	    bfd_byte *contents = value_contents_raw (retval);
+	    memcpy (contents, this->data + subobj_offset, n);
+	  }
+	  break;
+
+	case DWARF_VALUE_OPTIMIZED_OUT:
+	  retval = allocate_optimized_out_value (subobj_type);
+	  break;
+
+	  /* DWARF_VALUE_IMPLICIT_POINTER was converted to a pieced
+	     operation by execute_stack_op.  */
+	case DWARF_VALUE_IMPLICIT_POINTER:
+	  /* DWARF_VALUE_OPTIMIZED_OUT can't occur in this context --
+	     it can only be encountered when making a piece.  */
+	default:
+	  internal_error (__FILE__, __LINE__, _("invalid location type"));
+	}
+    }
+
+  set_value_initialized (retval, this->initialized);
+
+  return retval;
+}
+
 /* Require that TYPE be an integral type; throw an exception if not.  */
 
 static void
diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h
index 07d12305be9..c2138b31d79 100644
--- a/gdb/dwarf2/expr.h
+++ b/gdb/dwarf2/expr.h
@@ -26,7 +26,6 @@ 
 #include "gdbtypes.h"
 
 struct dwarf2_per_objfile;
-struct piece_closure;
 
 /* The location of a value.  */
 enum dwarf_value_location
@@ -125,9 +124,13 @@  struct dwarf_expr_context
 
   void push_address (CORE_ADDR value, bool in_stack_memory);
   void eval (const gdb_byte *addr, size_t len);
-  struct value *fetch (int n);
-  CORE_ADDR fetch_address (int n);
-  bool fetch_in_stack_memory (int n);
+
+  /* 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);
 
   /* The stack of values.  */
   std::vector<dwarf_stack_value> stack;
@@ -203,6 +206,9 @@  struct dwarf_expr_context
   void add_piece (ULONGEST size, ULONGEST offset);
   void execute_stack_op (const gdb_byte *op_ptr, const gdb_byte *op_end);
   void pop ();
+  struct value *fetch (int n);
+  CORE_ADDR fetch_address (int n);
+  bool fetch_in_stack_memory (int n);
 
   /* Return the location expression for the frame base attribute, in
      START and LENGTH.  The result must be live until the current
@@ -301,13 +307,4 @@  extern const gdb_byte *safe_read_sleb128 (const gdb_byte *buf,
 extern const gdb_byte *safe_skip_leb128 (const gdb_byte *buf,
 					 const gdb_byte *buf_end);
 
-extern const struct lval_funcs pieced_value_funcs;
-
-/* Allocate a closure for a value formed from separately-described
-   PIECES.  */
-
-struct piece_closure *allocate_piece_closure
-  (dwarf2_per_cu_data *per_cu, dwarf2_per_objfile *per_objfile,
-   std::vector<dwarf_expr_piece> &&pieces, struct frame_info *frame);
-
 #endif /* dwarf2expr.h */
diff --git a/gdb/dwarf2/frame.c b/gdb/dwarf2/frame.c
index 78dbb489506..8aaa0af7e91 100644
--- a/gdb/dwarf2/frame.c
+++ b/gdb/dwarf2/frame.c
@@ -229,8 +229,6 @@  execute_stack_op (const gdb_byte *exp, ULONGEST len, int addr_size,
 		  struct frame_info *this_frame, CORE_ADDR initial,
 		  int initial_in_stack_memory, dwarf2_per_objfile *per_objfile)
 {
-  CORE_ADDR result;
-
   dwarf_expr_context ctx (per_objfile);
   scoped_value_mark free_values;
 
@@ -241,18 +239,13 @@  execute_stack_op (const gdb_byte *exp, ULONGEST len, int addr_size,
   ctx.push_address (initial, initial_in_stack_memory);
   ctx.eval (exp, len);
 
-  if (ctx.location == DWARF_VALUE_MEMORY)
-    result = ctx.fetch_address (0);
-  else if (ctx.location == DWARF_VALUE_REGISTER)
-    result = read_addr_from_reg (this_frame, value_as_long (ctx.fetch (0)));
+  CORE_ADDR result;
+  struct value *result_val = ctx.fetch_result ();
+
+  if (VALUE_LVAL (result_val) == lval_memory)
+    result = value_address (result_val);
   else
-    {
-      /* This is actually invalid DWARF, but if we ever do run across
-	 it somehow, we might as well support it.  So, instead, report
-	 it as unimplemented.  */
-      error (_("\
-Not implemented: computing unwound register using explicit value operator"));
-    }
+    result = value_as_address (result_val);
 
   return result;
 }
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 9514d5ed668..6584a1615f7 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -92,7 +92,7 @@  enum debug_loc_kind
 /* Helper function which throws an error if a synthetic pointer is
    invalid.  */
 
-static void
+void
 invalid_synthetic_pointer (void)
 {
   error (_("access outside bounds of object "
@@ -1457,6 +1457,8 @@  dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
   try
     {
       ctx.eval (data, size);
+      retval = ctx.fetch_result (type, subobj_type,
+				 subobj_byte_offset);
     }
   catch (const gdb_exception_error &ex)
     {
@@ -1479,155 +1481,15 @@  dwarf2_evaluate_loc_desc_full (struct type *type, struct frame_info *frame,
 	throw;
     }
 
-  if (ctx.pieces.size () > 0)
-    {
-      struct piece_closure *c;
-      ULONGEST bit_size = 0;
-
-      for (dwarf_expr_piece &piece : ctx.pieces)
-	bit_size += piece.size;
-      /* Complain if the expression is larger than the size of the
-	 outer type.  */
-      if (bit_size > 8 * TYPE_LENGTH (type))
-	invalid_synthetic_pointer ();
-
-      c = allocate_piece_closure (per_cu, per_objfile, std::move (ctx.pieces),
-				  frame);
-      /* We must clean up the value chain after creating the piece
-	 closure but before allocating the result.  */
-      free_values.free_to_mark ();
-      retval = allocate_computed_value (subobj_type,
-					&pieced_value_funcs, c);
-      set_value_offset (retval, subobj_byte_offset);
-    }
-  else
-    {
-      switch (ctx.location)
-	{
-	case DWARF_VALUE_REGISTER:
-	  {
-	    struct gdbarch *arch = get_frame_arch (frame);
-	    int dwarf_regnum
-	      = longest_to_int (value_as_long (ctx.fetch (0)));
-	    int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, dwarf_regnum);
-
-	    if (subobj_byte_offset != 0)
-	      error (_("cannot use offset on synthetic pointer to register"));
-	    free_values.free_to_mark ();
-	    retval = value_from_register (subobj_type, gdb_regnum, frame);
-	    if (value_optimized_out (retval))
-	      {
-		struct value *tmp;
-
-		/* This means the register has undefined value / was
-		   not saved.  As we're computing the location of some
-		   variable etc. in the program, not a value for
-		   inspecting a register ($pc, $sp, etc.), return a
-		   generic optimized out value instead, so that we show
-		   <optimized out> instead of <not saved>.  */
-		tmp = allocate_value (subobj_type);
-		value_contents_copy (tmp, 0, retval, 0,
-				     TYPE_LENGTH (subobj_type));
-		retval = tmp;
-	      }
-	  }
-	  break;
-
-	case DWARF_VALUE_MEMORY:
-	  {
-	    struct type *ptr_type;
-	    CORE_ADDR address = ctx.fetch_address (0);
-	    bool in_stack_memory = ctx.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.  */
-
-	    switch (subobj_type->code ())
-	      {
-		case TYPE_CODE_FUNC:
-		case TYPE_CODE_METHOD:
-		  ptr_type = builtin_type (ctx.gdbarch)->builtin_func_ptr;
-		  break;
-		default:
-		  ptr_type = builtin_type (ctx.gdbarch)->builtin_data_ptr;
-		  break;
-	      }
-	    address = value_as_address (value_from_pointer (ptr_type, address));
-
-	    free_values.free_to_mark ();
-	    retval = value_at_lazy (subobj_type,
-				    address + subobj_byte_offset);
-	    if (in_stack_memory)
-	      set_value_stack (retval, 1);
-	  }
-	  break;
-
-	case DWARF_VALUE_STACK:
-	  {
-	    struct value *value = ctx.fetch (0);
-	    size_t n = TYPE_LENGTH (value_type (value));
-	    size_t len = TYPE_LENGTH (subobj_type);
-	    size_t max = TYPE_LENGTH (type);
-	    gdbarch *objfile_gdbarch = per_objfile->objfile->arch ();
-
-	    if (subobj_byte_offset + len > max)
-	      invalid_synthetic_pointer ();
-
-	    /* Preserve VALUE because we are going to free values back
-	       to the mark, but we still need the value contents
-	       below.  */
-	    value_ref_ptr value_holder = value_ref_ptr::new_reference (value);
-	    free_values.free_to_mark ();
-
-	    retval = allocate_value (subobj_type);
-
-	    /* The given offset is relative to the actual object.  */
-	    if (gdbarch_byte_order (objfile_gdbarch) == BFD_ENDIAN_BIG)
-	      subobj_byte_offset += n - max;
-
-	    memcpy (value_contents_raw (retval),
-		    value_contents_all (value) + subobj_byte_offset, len);
-	  }
-	  break;
-
-	case DWARF_VALUE_LITERAL:
-	  {
-	    bfd_byte *contents;
-	    size_t n = TYPE_LENGTH (subobj_type);
-
-	    if (subobj_byte_offset + n > ctx.len)
-	      invalid_synthetic_pointer ();
-
-	    free_values.free_to_mark ();
-	    retval = allocate_value (subobj_type);
-	    contents = value_contents_raw (retval);
-	    memcpy (contents, ctx.data + subobj_byte_offset, n);
-	  }
-	  break;
-
-	case DWARF_VALUE_OPTIMIZED_OUT:
-	  free_values.free_to_mark ();
-	  retval = allocate_optimized_out_value (subobj_type);
-	  break;
-
-	  /* DWARF_VALUE_IMPLICIT_POINTER was converted to a pieced
-	     operation by execute_stack_op.  */
-	case DWARF_VALUE_IMPLICIT_POINTER:
-	  /* DWARF_VALUE_OPTIMIZED_OUT can't occur in this context --
-	     it can only be encountered when making a piece.  */
-	default:
-	  internal_error (__FILE__, __LINE__, _("invalid location type"));
-	}
-    }
-
-  set_value_initialized (retval, ctx.initialized);
+  /* We need to clean up all the values that are not needed any more.
+     The problem with a value_ref_ptr class is that it disconnects the
+     RETVAL from the value garbage collection, so we need to make
+     a copy of that value on the stack to keep everything consistent.
+     The value_ref_ptr will clean up after itself at the end of this block.  */
+  value_ref_ptr value_holder = value_ref_ptr::new_reference (retval);
+  free_values.free_to_mark ();
 
-  return retval;
+  return value_copy (retval);
 }
 
 /* The exported interface to dwarf2_evaluate_loc_desc_full; it always
@@ -1668,6 +1530,9 @@  dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
   dwarf2_per_objfile *per_objfile = dlbaton->per_objfile;
   dwarf_expr_context ctx (per_objfile);
 
+  struct value *result;
+  scoped_value_mark free_values;
+
   ctx.frame = frame;
   ctx.per_cu = dlbaton->per_cu;
   if (addr_stack != nullptr)
@@ -1685,6 +1550,7 @@  dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
   try
     {
       ctx.eval (dlbaton->data, dlbaton->size);
+      result = ctx.fetch_result ();
     }
   catch (const gdb_exception_error &ex)
     {
@@ -1702,29 +1568,20 @@  dwarf2_locexpr_baton_eval (const struct dwarf2_locexpr_baton *dlbaton,
 	throw;
     }
 
-  switch (ctx.location)
+  if (value_optimized_out (result))
+    return 0;
+
+  if (VALUE_LVAL (result) == lval_memory)
+    *valp = value_address (result);
+  else
     {
-    case DWARF_VALUE_STACK:
-      *is_reference = false;
-      /* FALLTHROUGH */
-
-    case DWARF_VALUE_REGISTER:
-    case DWARF_VALUE_MEMORY:
-      *valp = ctx.fetch_address (0);
-      if (ctx.location == DWARF_VALUE_REGISTER)
-	*valp = read_addr_from_reg (frame, *valp);
-      return 1;
-    case DWARF_VALUE_LITERAL:
-      *valp = extract_signed_integer (ctx.data, ctx.len,
-				      gdbarch_byte_order (ctx.gdbarch));
-      return 1;
-      /* Unsupported dwarf values.  */
-    case DWARF_VALUE_OPTIMIZED_OUT:
-    case DWARF_VALUE_IMPLICIT_POINTER:
-      break;
+      if (VALUE_LVAL (result) == not_lval)
+	*is_reference = false;
+
+      *valp = value_as_address (result);
     }
 
-  return 0;
+  return 1;
 }
 
 /* See dwarf2loc.h.  */
diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
index 598dbcd4fbc..06b25fbb78e 100644
--- a/gdb/dwarf2/loc.h
+++ b/gdb/dwarf2/loc.h
@@ -276,6 +276,11 @@  extern int dwarf_reg_to_regnum (struct gdbarch *arch, int dwarf_reg);
 extern int dwarf_reg_to_regnum_or_error (struct gdbarch *arch,
 					 ULONGEST dwarf_reg);
 
+/* Helper function which throws an error if a synthetic pointer is
+   invalid.  */
+
+extern void invalid_synthetic_pointer ();
+
 /* Fetch the value pointed to by a synthetic pointer.  */
 
 extern struct value *indirect_synthetic_pointer