[gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

Message ID 95697ec8-30e2-acaf-60fd-e46524f88e72@suse.de
State New, archived
Headers

Commit Message

Tom de Vries Sept. 4, 2018, 1:06 p.m. UTC
  On 09/04/2018 12:16 PM, Tom de Vries wrote:
> [ was: [RFC, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract
> dies ]
> 
> On 08/24/2018 03:09 PM, Tom de Vries wrote:
>> [ was: Re: [PATCH 1/3] Add support for DW_OP_GNU_variable_value ]
>>
>> On 08/23/2018 11:12 PM, Kevin Buettner wrote:
>>> On Wed, 22 Aug 2018 17:35:23 +0200
>>> Tom de Vries <tdevries@suse.de> wrote:
>>>
>>>> On 08/18/2018 10:31 PM, Kevin Buettner wrote:
>>>>>     This patch adds support for DW_OP_GNU_variable_value to GDB.
>>>>>     
>>>>>     Jakub Jelinek provides a fairly expansive discussion of this DWARF
>>>>>     expression opcode in his GCC patch...
>>>>>     
>>>>>         https://gcc.gnu.org/ml/gcc-patches/2017-02/msg01499.html
>>>>>     
>>>>>     It has also been proposed for addition to the DWARF Standard:
>>>>>     
>>>>>         http://www.dwarfstd.org/ShowIssue.php?issue=161109.2  
>>>> Hi,
>>>>
>>>> AFAIU from the discussion here (
>>>> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01351.html ) if:
>>>> - a DW_OP_GNU_variable_value refers to a die 'a', and
>>>> - there's a die 'b' with abstract_origin 'a' that does have a
>>>>   DW_AT_location, and
>>>> - die 'b' is 'in scope' in an evaluation context,
>>>> then the evaluation of DW_OP_GNU_variable_value 'a' should return the
>>>> value found at the DW_AT_location of die 'b'.
> 
> Hi,
> 
> this patch implements the handling of DW_OP_GNU_variable_value refs to
> abstract dies.
> 
> There's no test-case yet, I'll try to write one.
> 

Test-case added.

OK for trunk?

Thanks,
- Tom
  

Comments

Kevin Buettner Sept. 4, 2018, 9:54 p.m. UTC | #1
Hi Tom,

Thank you for doing this work.  It looks mostly okay, though I do have
a few comments...

Kevin

On Tue, 4 Sep 2018 15:06:34 +0200
Tom de Vries <tdevries@suse.de> wrote:

> 2018-09-04  Tom de Vries  <tdevries@suse.de>
> 
> 	* dwarf2loc.c (sect_variable_value): Call indirect_synthetic_pointer
> 	with resolve_abstract_p == true.
> 	(indirect_synthetic_pointer): Add resolve_abstract_p parameter,
> 	defaulting to false. Propagate resolve_abstract_p to
> 	dwarf2_fetch_die_loc_sect_off.
> 	* dwarf2loc.h (dwarf2_fetch_die_loc_sect_off): Add resolve_abstract_p
> 	parameter, defaulting to false.
> 	* dwarf2read.c (dwarf2_per_objfile::~dwarf2_per_objfile): Reset
> 	abstract_to_concrete.
> 	(read_variable): Add variable to abstract_to_concrete.
> 	(dwarf2_fetch_die_loc_sect_off): Add and handle resolve_abstract_p
> 	parameter.
> 	* dwarf2read.h (struct die_info): Forward-declare.
> 	(die_info_ptr): New typedef.
> 	(DEF_VEC_P (die_info_ptr)): Add.
> 	(struct dwarf2_per_objfile): Add abstract_to_concrete field.
> 
> 	* gdb.dwarf2/varval.exp: Add test.
> 
> ---
>  gdb/dwarf2loc.c                     | 10 ++++---
>  gdb/dwarf2loc.h                     |  2 +-
>  gdb/dwarf2read.c                    | 56 +++++++++++++++++++++++++++++++++++--
>  gdb/dwarf2read.h                    |  6 ++++
>  gdb/testsuite/gdb.dwarf2/varval.exp | 22 ++++++++++++++-
>  5 files changed, 88 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
> index 200fa03f46..a75eb158c7 100644
> --- a/gdb/dwarf2loc.c
> +++ b/gdb/dwarf2loc.c
> @@ -65,7 +65,7 @@ static struct value *indirect_synthetic_pointer
>    (sect_offset die, LONGEST byte_offset,
>     struct dwarf2_per_cu_data *per_cu,
>     struct frame_info *frame,
> -   struct type *type);
> +   struct type *type, bool = false);

Please add a name for the new bool parameter.

> diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
> index f82e7b2d11..08120651b6 100644
> --- a/gdb/dwarf2loc.h
> +++ b/gdb/dwarf2loc.h
> @@ -67,7 +67,7 @@ const gdb_byte *dwarf2_find_location_expression
>  struct dwarf2_locexpr_baton dwarf2_fetch_die_loc_sect_off
>    (sect_offset offset_in_cu, struct dwarf2_per_cu_data *per_cu,
>     CORE_ADDR (*get_frame_pc) (void *baton),
> -   void *baton);
> +   void *baton, bool = false);

Likewise, here.

> diff --git a/gdb/dwarf2read.h b/gdb/dwarf2read.h
> index 13855bcd54..71ef781d17 100644
> --- a/gdb/dwarf2read.h
> +++ b/gdb/dwarf2read.h
> @@ -20,6 +20,7 @@
>  #ifndef DWARF2READ_H
>  #define DWARF2READ_H
>  
> +#include <unordered_map>
>  #include "dwarf-index-cache.h"
>  #include "filename-seen-cache.h"
>  #include "gdb_obstack.h"
> @@ -95,6 +96,9 @@ struct dwarf2_debug_sections;
>  struct mapped_index;
>  struct mapped_debug_names;
>  struct signatured_type;
> +struct die_info;
> +typedef struct die_info *die_info_ptr;
> +DEF_VEC_P (die_info_ptr);
>  
>  /* Collection of data recorded per objfile.
>     This hangs off of dwarf2_objfile_data_key.  */
> @@ -250,6 +254,8 @@ public:
>    /* If we loaded the index from an external file, this contains the
>       resources associated to the open file, memory mapping, etc.  */
>    std::unique_ptr<index_cache_resource> index_cache_res;
> +
> +  std::unordered_map<die_info_ptr, VEC (die_info_ptr) *> abstract_to_concrete;
>  };

Is there a good reason to use VEC instead of std::vector?  I know that
there have been a number of patches which have been replacing VEC
with std:vector.  So, unless there's a compelling reason to use VEC,
we might as well use std:vector here and save someone else the effort
of changing this use of VEC later on.

Also, can you add a comment for abstract_to_concrete?

Kevin
  
Tom Tromey Sept. 6, 2018, 3:45 a.m. UTC | #2
>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> Is there a good reason to use VEC instead of std::vector?  I know that
Kevin> there have been a number of patches which have been replacing VEC
Kevin> with std:vector.  So, unless there's a compelling reason to use VEC,
Kevin> we might as well use std:vector here and save someone else the effort
Kevin> of changing this use of VEC later on.

Thanks for this note.  One of my gdb c++-ification/cleanup goals is to
get rid of VEC.

Tom
  
Tom de Vries Sept. 6, 2018, 6:40 a.m. UTC | #3
On 09/06/2018 05:45 AM, Tom Tromey wrote:
>>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:
> 
> Kevin> Is there a good reason to use VEC instead of std::vector?  I know that
> Kevin> there have been a number of patches which have been replacing VEC
> Kevin> with std:vector.  So, unless there's a compelling reason to use VEC,
> Kevin> we might as well use std:vector here and save someone else the effort
> Kevin> of changing this use of VEC later on.
> 
> Thanks for this note.  One of my gdb c++-ification/cleanup goals is to
> get rid of VEC.

It would be nice if the sources reflected that fact.

F.i., we could move the contents from vec.{c,h} to vec-deprecated.{c.h}
and include the new files in vec.{c,h}, and add a note there why they're
deprecated.

I can prepare a patch implementing this or another approach, if there
are better suggestions.

Thanks,
- Tom
  
Tom Tromey Sept. 6, 2018, 1:11 p.m. UTC | #4
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> It would be nice if the sources reflected that fact.

Tom> F.i., we could move the contents from vec.{c,h} to vec-deprecated.{c.h}
Tom> and include the new files in vec.{c,h}, and add a note there why they're
Tom> deprecated.

Tom> I can prepare a patch implementing this or another approach, if there
Tom> are better suggestions.

I think that probably would not prevent the introduction of new uses,
because normally code wouldn't be including this header directly anyway.

Even adding a deprecated_ onto a name isn't really enough, I think on
occasion we let a new use of a deprecated function in -- maybe a sign
that the function shouldn't be deprecated, or maybe just an admission
that nobody is working on finishing those transitions.

Still, no objections from me if you want to do this!  But I tend to
think the current situation is fine.

Finally FWIW this isn't the only such project that is ongoing.  Cleanup
removal is the other big one, but I'd also eventually like to be able to
remove queue.h (I can't recall but I think I sent some patches); and
eventually also buffer.h.

Tom
  

Patch

[gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

Consider a vla variable 'a' in function f1:
...
 <2><1a7>: Abbrev Number: 11 (DW_TAG_variable)
    <1a8>   DW_AT_description : a
    <1aa>   DW_AT_abstract_origin: <0x311>
...
with abstract origin 'a':
...
 <2><311>: Abbrev Number: 3 (DW_TAG_variable)
    <312>   DW_AT_name        : a
    <317>   DW_AT_type        : <0x325>
...
and inherited abstract vla type:
...
 <1><325>: Abbrev Number: 9 (DW_TAG_array_type)
    <326>   DW_AT_type        : <0x33a>
 <2><32e>: Abbrev Number: 10 (DW_TAG_subrange_type)
    <32f>   DW_AT_type        : <0x2ea>
    <333>   DW_AT_upper_bound : 5 byte block: fd 1b 3 0 0
                                (DW_OP_GNU_variable_value: <0x31b>)
...
where the upper bound refers to this artificial variable D.1922 without location
attribute:
...
 <2><31b>: Abbrev Number: 8 (DW_TAG_variable)
    <31c>   DW_AT_description : (indirect string, offset: 0x39a): D.1922
    <320>   DW_AT_type        : <0x2ea>
    <324>   DW_AT_artificial  : 1
...

Currently, when we execute "p sizeof (a)" in f1, the upper bound is calculated
by evaluating the DW_OP_GNU_variable_value expression referring to D.1922, but
since that die doesn't have a location attribute, we get:
...
value has been optimized out
...

However, there's also artificial variable D.4283 that is sibling of vla
variable 'a', has artificial variable D.1922 as abstract origin, and has a
location attribute:
...
 <2><1ae>: Abbrev Number: 12 (DW_TAG_variable)
    <1af>   DW_AT_description : (indirect string, offset: 0x1f8): D.4283
    <1b3>   DW_AT_abstract_origin: <0x31b>
    <1b7>   DW_AT_location    : 11 byte block: 75 1 8 20 24 8 20 26 31 1c 9f
                                (DW_OP_breg5 (rdi):1; DW_OP_const1u: 32;
				 DW_OP_shl; DW_OP_const1u: 32; DW_OP_shra;
				 DW_OP_lit1; DW_OP_minus; DW_OP_stack_value)
...

The intended behaviour for DW_OP_GNU_variable_value is to find a die that
refers to D.1922 as abstract origin, has a location attribute and is
'in scope', so the expected behaviour is:
...
$1 = 6
...

The 'in scope' concept can be thought of as variable D.1922 having name
attribute "D.1922", and variable D.4283 inheriting that attribute, resulting
in D.4283 being declared with name "D.1922" alongside vla a in f1, and when we
lookup "DW_OP_GNU_variable_value D.1922", it should work as if we try to find
the value of a variable named "D.1922" on the gdb command line using
"p D.1922", and we should return the value of D.4283.

This patch fixes the case described above, by:
- adding a field abstract_to_concrete to struct dwarf2_per_objfile,
- using that field to keep track of which concrete dies are instances of an
  abstract die, and
- using that information when getting the value DW_OP_GNU_variable_value.

Build and reg-tested on x86_64-linux.

2018-09-04  Tom de Vries  <tdevries@suse.de>

	* dwarf2loc.c (sect_variable_value): Call indirect_synthetic_pointer
	with resolve_abstract_p == true.
	(indirect_synthetic_pointer): Add resolve_abstract_p parameter,
	defaulting to false. Propagate resolve_abstract_p to
	dwarf2_fetch_die_loc_sect_off.
	* dwarf2loc.h (dwarf2_fetch_die_loc_sect_off): Add resolve_abstract_p
	parameter, defaulting to false.
	* dwarf2read.c (dwarf2_per_objfile::~dwarf2_per_objfile): Reset
	abstract_to_concrete.
	(read_variable): Add variable to abstract_to_concrete.
	(dwarf2_fetch_die_loc_sect_off): Add and handle resolve_abstract_p
	parameter.
	* dwarf2read.h (struct die_info): Forward-declare.
	(die_info_ptr): New typedef.
	(DEF_VEC_P (die_info_ptr)): Add.
	(struct dwarf2_per_objfile): Add abstract_to_concrete field.

	* gdb.dwarf2/varval.exp: Add test.

---
 gdb/dwarf2loc.c                     | 10 ++++---
 gdb/dwarf2loc.h                     |  2 +-
 gdb/dwarf2read.c                    | 56 +++++++++++++++++++++++++++++++++++--
 gdb/dwarf2read.h                    |  6 ++++
 gdb/testsuite/gdb.dwarf2/varval.exp | 22 ++++++++++++++-
 5 files changed, 88 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 200fa03f46..a75eb158c7 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -65,7 +65,7 @@  static struct value *indirect_synthetic_pointer
   (sect_offset die, LONGEST byte_offset,
    struct dwarf2_per_cu_data *per_cu,
    struct frame_info *frame,
-   struct type *type);
+   struct type *type, bool = false);
 
 /* Until these have formal names, we define these here.
    ref: http://gcc.gnu.org/wiki/DebugFission
@@ -573,7 +573,7 @@  sect_variable_value (struct dwarf_expr_context *ctx, sect_offset sect_off,
 
   struct type *type = lookup_pointer_type (die_type);
   struct frame_info *frame = get_selected_frame (_("No frame selected."));
-  return indirect_synthetic_pointer (sect_off, 0, per_cu, frame, type);
+  return indirect_synthetic_pointer (sect_off, 0, per_cu, frame, type, true);
 }
 
 class dwarf_evaluate_loc_desc : public dwarf_expr_context
@@ -2181,12 +2181,14 @@  fetch_const_value_from_synthetic_pointer (sect_offset die, LONGEST byte_offset,
 static struct value *
 indirect_synthetic_pointer (sect_offset die, LONGEST byte_offset,
 			    struct dwarf2_per_cu_data *per_cu,
-			    struct frame_info *frame, struct type *type)
+			    struct frame_info *frame, struct type *type,
+			    bool resolve_abstract_p)
 {
   /* Fetch the location expression of the DIE we're pointing to.  */
   struct dwarf2_locexpr_baton baton
     = dwarf2_fetch_die_loc_sect_off (die, per_cu,
-				     get_frame_address_in_block_wrapper, frame);
+				     get_frame_address_in_block_wrapper, frame,
+				     resolve_abstract_p);
 
   /* Get type of pointed-to DIE.  */
   struct type *orig_type = dwarf2_fetch_die_type_sect_off (die, per_cu);
diff --git a/gdb/dwarf2loc.h b/gdb/dwarf2loc.h
index f82e7b2d11..08120651b6 100644
--- a/gdb/dwarf2loc.h
+++ b/gdb/dwarf2loc.h
@@ -67,7 +67,7 @@  const gdb_byte *dwarf2_find_location_expression
 struct dwarf2_locexpr_baton dwarf2_fetch_die_loc_sect_off
   (sect_offset offset_in_cu, struct dwarf2_per_cu_data *per_cu,
    CORE_ADDR (*get_frame_pc) (void *baton),
-   void *baton);
+   void *baton, bool = false);
 
 struct dwarf2_locexpr_baton dwarf2_fetch_die_loc_cu_off
   (cu_offset offset_in_cu, struct dwarf2_per_cu_data *per_cu,
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index d66dfeaf2d..419858bb98 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2159,6 +2159,13 @@  dwarf2_per_objfile::~dwarf2_per_objfile ()
 
   VEC_free (dwarf2_section_info_def, types);
 
+  for (std::unordered_map<die_info_ptr, VEC (die_info_ptr) *>::iterator it
+	 = abstract_to_concrete.begin ();
+       it != abstract_to_concrete.end ();
+       it++)
+    VEC_free (die_info_ptr, it->second);
+  abstract_to_concrete.clear ();
+
   if (dwo_files != NULL)
     free_dwo_files (dwo_files, objfile);
 
@@ -14283,7 +14290,23 @@  read_variable (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
-  new_symbol (die, NULL, cu, storage);
+  struct symbol *res = new_symbol (die, NULL, cu, storage);
+  struct attribute *abstract_origin
+    = dwarf2_attr (die, DW_AT_abstract_origin, cu);
+  struct attribute *loc = dwarf2_attr (die, DW_AT_location, cu);
+  if (res == NULL && loc && abstract_origin)
+    {
+      /* We have a variable without a name, but with a location and an abstract
+	 origin.  This may be a concrete instance of an abstract variable
+	 referenced from an DW_OP_GNU_variable_value, so save it to find it back
+	 later.  */
+      struct dwarf2_cu *origin_cu = cu;
+      struct die_info *origin_die
+	= follow_die_ref (die, abstract_origin, &origin_cu);
+      dwarf2_per_objfile *dpo = cu->per_cu->dwarf2_per_objfile;
+      VEC_safe_push (die_info_ptr, dpo->abstract_to_concrete[origin_die],
+		     die);
+    }
 }
 
 /* Call CALLBACK from DW_AT_ranges attribute value OFFSET
@@ -23010,7 +23033,7 @@  struct dwarf2_locexpr_baton
 dwarf2_fetch_die_loc_sect_off (sect_offset sect_off,
 			       struct dwarf2_per_cu_data *per_cu,
 			       CORE_ADDR (*get_frame_pc) (void *baton),
-			       void *baton)
+			       void *baton, bool resolve_abstract_p)
 {
   struct dwarf2_cu *cu;
   struct die_info *die;
@@ -23036,6 +23059,35 @@  dwarf2_fetch_die_loc_sect_off (sect_offset sect_off,
 	   sect_offset_str (sect_off), objfile_name (objfile));
 
   attr = dwarf2_attr (die, DW_AT_location, cu);
+  if (!attr && resolve_abstract_p
+      && (dwarf2_per_objfile->abstract_to_concrete.find (die)
+	  != dwarf2_per_objfile->abstract_to_concrete.end ()))
+    {
+      CORE_ADDR pc = (*get_frame_pc) (baton);
+
+      unsigned i;
+      struct die_info *cand;
+      for (i = 0;
+	   VEC_iterate (die_info_ptr,
+			dwarf2_per_objfile->abstract_to_concrete[die], i, cand);
+	   i++)
+	{
+	  if (!cand->parent
+	      || cand->parent->tag != DW_TAG_subprogram)
+	    continue;
+
+	  CORE_ADDR pc_low, pc_high;
+	  get_scope_pc_bounds (cand->parent, &pc_low, &pc_high, cu);
+	  if (pc_low == ((CORE_ADDR) -1)
+	      || !(pc_low <= pc && pc < pc_high))
+	    continue;
+
+	  die = cand;
+	  attr = dwarf2_attr (die, DW_AT_location, cu);
+	  break;
+	}
+    }
+
   if (!attr)
     {
       /* DWARF: "If there is no such attribute, then there is no effect.".
diff --git a/gdb/dwarf2read.h b/gdb/dwarf2read.h
index 13855bcd54..71ef781d17 100644
--- a/gdb/dwarf2read.h
+++ b/gdb/dwarf2read.h
@@ -20,6 +20,7 @@ 
 #ifndef DWARF2READ_H
 #define DWARF2READ_H
 
+#include <unordered_map>
 #include "dwarf-index-cache.h"
 #include "filename-seen-cache.h"
 #include "gdb_obstack.h"
@@ -95,6 +96,9 @@  struct dwarf2_debug_sections;
 struct mapped_index;
 struct mapped_debug_names;
 struct signatured_type;
+struct die_info;
+typedef struct die_info *die_info_ptr;
+DEF_VEC_P (die_info_ptr);
 
 /* Collection of data recorded per objfile.
    This hangs off of dwarf2_objfile_data_key.  */
@@ -250,6 +254,8 @@  public:
   /* If we loaded the index from an external file, this contains the
      resources associated to the open file, memory mapping, etc.  */
   std::unique_ptr<index_cache_resource> index_cache_res;
+
+  std::unordered_map<die_info_ptr, VEC (die_info_ptr) *> abstract_to_concrete;
 };
 
 /* Get the dwarf2_per_objfile associated to OBJFILE.  */
diff --git a/gdb/testsuite/gdb.dwarf2/varval.exp b/gdb/testsuite/gdb.dwarf2/varval.exp
index f4319ae7d2..400ad60873 100644
--- a/gdb/testsuite/gdb.dwarf2/varval.exp
+++ b/gdb/testsuite/gdb.dwarf2/varval.exp
@@ -49,7 +49,9 @@  Dwarf::assemble ${asm_file} {
 	} {
 	    declare_labels int_label ptr_label struct_label var_a_label \
 	                   var_b_label var_c_label var_p_label var_bad_label \
-			   varval_label var_s_label var_untyped_label
+			   varval_label var_s_label var_untyped_label \
+			   var_a_abstract_label var_a_concrete_label \
+			   varval2_label
 
 	    set int_size [get_sizeof "int" -1]
 
@@ -73,6 +75,11 @@  Dwarf::assemble ${asm_file} {
 		{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
 	    }
 
+	    var_a_abstract_label: DW_TAG_variable {
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+	    }
+
 	    var_b_label: DW_TAG_variable {
 		{DW_AT_name "var_b"}
 		{DW_AT_type :${int_label}}
@@ -171,6 +178,18 @@  Dwarf::assemble ${asm_file} {
 			DW_OP_stack_value
 		    } SPECIAL_expr}
 		}
+		varval2_label: DW_TAG_variable {
+		    {DW_AT_name "varval2"}
+		    {DW_AT_type :${int_label}}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_a_abstract_label}
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		var_a_concrete_label: DW_TAG_variable {
+		    {DW_AT_abstract_origin :${var_a_abstract_label}}
+		    {DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
+		}
 		DW_TAG_variable {
 		    {DW_AT_name "constval"}
 		    {DW_AT_type :${int_label}}
@@ -255,6 +274,7 @@  if ![runto_main] {
 }
 
 gdb_test "print varval" "= 8"
+gdb_test "print varval2" "= 8"
 gdb_test "print constval" "= 53"
 gdb_test "print mixedval" "= 42"
 gdb_test "print pointerval" "= \\(int \\*\\) $hex <var_b>"