[PATCH 0/3] Add support for DW_OP_GNU_variable_value

[PATCH 0/3] Add support for DW_OP_GNU_variable_value

[Kevin Buettner]

This patch set adds support and a test case for
DW_OP_GNU_variable_value.

I've posted a patch to binutils adding readelf support for this
DWARF expression opcode:

    https://sourceware.org/ml/binutils/2018-08/msg00059.html

I'm keeping this introductory message short.  There's a lot more
info in the first patch...

Kevin

[PATCH 1/3] Add support for DW_OP_GNU_variable_value

[Kevin Buettner]

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

If compiled with a suitable version of GCC, the test case associated
with GCC Bug 77589 uses DW_OP_GNU_variable_value in a DW_AT_byte_stride
expression.  Here's a link to the bug:

    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77589

This is what the DWARF looks like.  Look at the last line, which has
the DW_AT_byte_stride expression:

 <2><e1>: Abbrev Number: 12 (DW_TAG_variable)
    <e2>   DW_AT_name        : (indirect string, offset: 0x115): span.0
    <e6>   DW_AT_type        : <0x2e>
    <ea>   DW_AT_artificial  : 1
    <ea>   DW_AT_location    : 3 byte block: 91 b0 7f 	(DW_OP_fbreg: -80)
 ...
 <2><178>: Abbrev Number: 18 (DW_TAG_subrange_type)
    <179>   DW_AT_lower_bound : 4 byte block: 97 23 20 6 	(DW_OP_push_object_address; DW_OP_plus_uconst: 32; DW_OP_deref)
    <17e>   DW_AT_upper_bound : 4 byte block: 97 23 28 6 	(DW_OP_push_object_address; DW_OP_plus_uconst: 40; DW_OP_deref)
    <183>   DW_AT_byte_stride : 10 byte block: 97 23 18 6 fd e1 0 0 0 1e 	(DW_OP_push_object_address; DW_OP_plus_uconst: 24; DW_OP_deref; DW_OP_GNU_variable_value: <0xe1>; DW_OP_mul)

A patch to readelf, which I'm also submitting, is required to do this
decoding.

I found that GDB gave me the correct answer for "p c40pt(2)" once I
(correctly) implemented DW_OP_GNU_variable_value.

I also have test case (later in this series) which uses the DWARF
assembler and, therefore, does not rely on having a compiler with this
support.

gdb/ChangeLog:
    
    	* dwarf2expr.h (struct dwarf_expr_context): Add virtual method
    	dwarf_variable_value.
    	* dwarf2-frame.c (class dwarf_expr_executor):
    	Add override for dwarf_variable_value.
    	* dwarf2loc.c (class dwarf_evaluate_loc_desc): Likewise.
    	(class symbol_needs_eval_context): Likewise.
    	(indirect_synthetic_pointer): Add forward declaration.
    	(sect_variable_value): New function.
    	(dwarf2_compile_expr_to_ax): Add case for DW_OP_GNU_variable_value.
    	* dwarf2expr.c (dwarf_expr_context::execute_stack_op): Add case
    	for DW_OP_GNU_variable_value.
---
 gdb/dwarf2-frame.c |  5 +++++
 gdb/dwarf2expr.c   | 11 +++++++++++
 gdb/dwarf2expr.h   |  3 +++
 gdb/dwarf2loc.c    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 63 insertions(+)

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 58f1ba4..f7dc820 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -284,6 +284,11 @@ 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_GNU_addr_index");
diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
index 445f857..f1ca033 100644
--- a/gdb/dwarf2expr.c
+++ b/gdb/dwarf2expr.c
@@ -1259,6 +1259,17 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	    this->dwarf_call (cu_off);
 	  }
 	  goto no_push;
+
+	case DW_OP_GNU_variable_value:
+	  {
+	    sect_offset sect_off
+	      = (sect_offset) extract_unsigned_integer (op_ptr,
+	                                                this->ref_addr_size,
+							byte_order);
+	    op_ptr += this->ref_addr_size;
+	    result_val = this->dwarf_variable_value (sect_off);
+	  }
+	  break;
 	
 	case DW_OP_entry_value:
 	case DW_OP_GNU_entry_value:
diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index c746bfe..c702a19 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -221,6 +221,9 @@ struct dwarf_expr_context
      subroutine.  */
   virtual void dwarf_call (cu_offset die_cu_off) = 0;
 
+  /* Execute "variable value" operation on DIED 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
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index a98b676..5c7c4ba 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -61,6 +61,12 @@ static struct call_site_parameter *dwarf_expr_reg_to_entry_parameter
      union call_site_parameter_u kind_u,
      struct dwarf2_per_cu_data **per_cu_return);
 
+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);
+
 /* Until these have formal names, we define these here.
    ref: http://gcc.gnu.org/wiki/DebugFission
    Each entry in .debug_loc.dwo begins with a byte that describes the entry,
@@ -546,6 +552,25 @@ per_cu_dwarf_call (struct dwarf_expr_context *ctx, cu_offset die_offset,
   ctx->eval (block.data, block.size);
 }
 
+struct value *
+sect_variable_value (struct dwarf_expr_context *ctx, sect_offset sect_off,
+		     struct dwarf2_per_cu_data *per_cu)
+{
+  struct type *die_type = dwarf2_fetch_die_type_sect_off (sect_off, per_cu);
+
+  if (die_type == NULL)
+    error (_("Bad DW_OP_GNU_variable_value DIE."));
+
+  /* Note: Things still work when the following test is removed.  This
+     test and error is here to conform to the proposed specification.  */
+  if (!is_scalar_type (die_type) || TYPE_CODE (die_type) == TYPE_CODE_VOID)
+    error (_("Type of DW_OP_GNU_variable_value DIE must be an integer or pointer."));
+
+  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);
+}
+
 class dwarf_evaluate_loc_desc : public dwarf_expr_context
 {
  public:
@@ -587,6 +612,14 @@ class dwarf_evaluate_loc_desc : public dwarf_expr_context
     per_cu_dwarf_call (this, die_offset, per_cu);
   }
 
+  /* 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);
+  }
+
   struct type *get_base_type (cu_offset die_offset, int size) override
   {
     struct type *result = dwarf2_get_die_type (die_offset, per_cu);
@@ -2815,6 +2848,14 @@ class symbol_needs_eval_context : public dwarf_expr_context
     per_cu_dwarf_call (this, die_offset, per_cu);
   }
 
+  /* Helper interface of sect_variable_value for
+     dwarf2_loc_desc_get_symbol_read_needs.  */
+
+  struct value *dwarf_variable_value (sect_offset sect_off) override
+  {
+    return sect_variable_value (this, sect_off, per_cu);
+  }
+
   /* DW_OP_entry_value accesses require a caller, therefore a
      frame.  */
 
@@ -3655,6 +3696,9 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
 	case DW_OP_call_ref:
 	  unimplemented (op);
 
+	case DW_OP_GNU_variable_value:
+	  unimplemented (op);
+
 	default:
 	  unimplemented (op);
 	}

[PATCH 2/3] Add support of DW_OP_GNU_variable_value to DWARF assembler

[Kevin Buettner]

gdb/testsuite/ChangeLog:
    
    	* lib/dwarf.exp: Add support for DW_OP_GNU_variable_value.
---
 gdb/testsuite/lib/dwarf.exp | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index 82ec29b..0c3f50a 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -962,6 +962,20 @@ namespace eval Dwarf {
 		    _op .sleb128 [lindex $line 2]
 		}
 
+		DW_OP_GNU_variable_value {
+		    if {[llength $line] != 2} {
+			error "usage: $opcode LABEL"
+		    }
+
+		    # Here label is a section offset.
+		    set label [lindex $line 1]
+		    if { $_cu_version == 2 } {
+			_op .${_cu_addr_size}byte $label
+		    } else {
+			_op .${_cu_offset_size}byte $label
+		    }
+		}
+
 		DW_OP_deref_size {
 		    if {[llength $line] != 2} {
 			error "usage: DW_OP_deref_size SIZE"

[PATCH 3/3] Test case for DW_OP_GNU_variable_value

[Kevin Buettner]

gdb/testsuite/ChangeLog:
    
    	* gdb.dwarf2/varval.c: New file.
    	* gdb.dwarf2/varval.exp: New file.
---
 gdb/testsuite/gdb.dwarf2/varval.c   |  30 ++++
 gdb/testsuite/gdb.dwarf2/varval.exp | 279 ++++++++++++++++++++++++++++++++++++
 2 files changed, 309 insertions(+)

diff --git a/gdb/testsuite/gdb.dwarf2/varval.c b/gdb/testsuite/gdb.dwarf2/varval.c
new file mode 100644
index 0000000..ce5592e
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/varval.c
@@ -0,0 +1,30 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* Test program for DW_OP_GNU_variable_value.  */
+
+int var_a = 8;
+int var_b = 3;
+int *var_p = &var_b;
+struct { int a, b, c, d, e, f, g, h; } var_s = { 101, 102, 103, 104, 105, 106, 107, 108 };
+
+int
+main (void)
+{
+  asm ("main_label: .globl main_label");
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/varval.exp b/gdb/testsuite/gdb.dwarf2/varval.exp
new file mode 100644
index 0000000..f4319ae
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/varval.exp
@@ -0,0 +1,279 @@
+# Copyright 2018 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test support for DW_OP_GNU_variable_value. 
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if ![dwarf2_support] {
+    return 0
+}
+
+# We'll place the output of Dwarf::assemble in varval.S.
+standard_testfile .c .S
+
+# ${testfile} is now "varval".  srcfile2 is "varval.S".
+set executable ${testfile}
+set asm_file [standard_output_file ${srcfile2}]
+
+# We need to know the size of integer and address types in order
+# to write some of the debugging info we'd like to generate.
+#
+# For that, we ask GDB by debugging our varval program.
+# Any program would do, but since we already have varval
+# specifically for this testcase, might as well use that.
+if [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] {
+    return -1
+}
+
+# Create the DWARF.  
+Dwarf::assemble ${asm_file} {
+    global srcdir subdir srcfile
+
+    cu {} {
+	DW_TAG_compile_unit {
+	    {DW_AT_language @DW_LANG_C_plus_plus}
+	} {
+	    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
+
+	    set int_size [get_sizeof "int" -1]
+
+	    # gdb always assumes references are implemented as pointers.
+	    set addr_size [get_sizeof "void *" -1]
+
+	    int_label: DW_TAG_base_type {
+		{DW_AT_byte_size ${int_size} DW_FORM_udata}
+		{DW_AT_encoding @DW_ATE_signed}
+		{DW_AT_name "int"}
+	    }
+
+	    ptr_label: DW_TAG_pointer_type {
+		{DW_AT_type :$int_label}
+	    }
+
+	    var_a_label: DW_TAG_variable {
+		{DW_AT_name "var_a"}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_a"]} SPECIAL_expr}
+	    }
+
+	    var_b_label: DW_TAG_variable {
+		{DW_AT_name "var_b"}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_b"]} SPECIAL_expr}
+	    }
+
+	    var_c_label: DW_TAG_variable {
+		{DW_AT_name "var_c"}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+		{DW_AT_const_value 53 DW_FORM_sdata}
+	    }
+
+	    var_p_label: DW_TAG_variable {
+		{DW_AT_name "var_p"}
+		{DW_AT_type :${ptr_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_p"]} SPECIAL_expr}
+	    }
+
+	    var_bad_label: DW_TAG_variable {
+		{DW_AT_name "var_bad"}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+	    }
+
+	    struct_label: DW_TAG_structure_type {
+		{DW_AT_byte_size 8*$int_size DW_FORM_sdata}
+	    } {
+		DW_TAG_member {
+		    {DW_AT_name "a"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 0*$int_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "b"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 1*$int_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "c"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 2*$int_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "d"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 3*$int_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "e"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 4*$int_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "f"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 5*$int_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "g"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 6*$int_size DW_FORM_udata}
+		}
+		DW_TAG_member {
+		    {DW_AT_name "h"}
+		    {DW_AT_type :$int_label}
+		    {DW_AT_data_member_location 7*$int_size DW_FORM_udata}
+		}
+	    }
+
+	    var_s_label: DW_TAG_variable {
+		{DW_AT_name "var_s"}
+		{DW_AT_type :${struct_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_s"]} SPECIAL_expr}
+	    }
+
+	    var_untyped_label: DW_TAG_variable {
+		{DW_AT_name "var_untyped"}
+		{DW_AT_external 1 DW_FORM_flag}
+		{DW_AT_location {DW_OP_addr [gdb_target_symbol "var_b"]} SPECIAL_expr}
+	    }
+
+	    DW_TAG_subprogram {
+		{MACRO_AT_func { "main" "${srcdir}/${subdir}/${srcfile}" }}
+		{DW_AT_type :${int_label}}
+		{DW_AT_external 1 DW_FORM_flag}
+	    } {
+		varval_label: DW_TAG_variable {
+		    {DW_AT_name "varval"}
+		    {DW_AT_type :${int_label}}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_a_label}
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "constval"}
+		    {DW_AT_type :${int_label}}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_c_label}
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "mixedval"}
+		    {DW_AT_type :${int_label}}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_c_label}
+			DW_OP_GNU_variable_value ${var_b_label}
+			DW_OP_div
+			DW_OP_GNU_variable_value ${varval_label}
+			DW_OP_plus
+			DW_OP_dup
+			DW_OP_plus
+			DW_OP_GNU_variable_value ${varval_label}
+			DW_OP_minus
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "pointerval"}
+		    {DW_AT_type :${ptr_label}}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_p_label}
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "badval"}
+		    {DW_AT_type :${int_label}}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_bad_label}
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "structval"}
+		    {DW_AT_type :${struct_label}}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_s_label}
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "untypedval"}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${var_untyped_label}
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "bad_die_val1"}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value 0xabcdef11
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+		DW_TAG_variable {
+		    {DW_AT_name "bad_die_val2"}
+		    {DW_AT_location {
+			DW_OP_GNU_variable_value ${ptr_label}+1
+			DW_OP_stack_value
+		    } SPECIAL_expr}
+		}
+	    }
+	}
+    }
+}
+
+if [prepare_for_testing "failed to prepare" ${executable} [list ${asm_file} ${srcfile}] {}] {
+    return -1
+}
+
+# DW_OP_GNU_variable_value implementation requires a valid frame.
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "print varval" "= 8"
+gdb_test "print constval" "= 53"
+gdb_test "print mixedval" "= 42"
+gdb_test "print pointerval" "= \\(int \\*\\) $hex <var_b>"
+gdb_test "print *pointerval" "= 3"
+gdb_test "print badval" "value has been optimized out"
+
+# Jakub says:  "The intended behavior is that the debug info consumer
+# computes the value of that referenced variable at the current PC,
+# and if it can compute it and pushes the value as a generic type
+# integer into the DWARF stack (it is really only meaningful when
+# referring to integral/pointer typed variables)."
+
+gdb_test "print structval" \
+         "Type of DW_OP_GNU_variable_value DIE must be an integer or pointer\\."
+
+gdb_test "print untypedval" \
+         "Type of DW_OP_GNU_variable_value DIE must be an integer or pointer\\."
+
+gdb_test "print bad_die_val1" \
+         "invalid dwarf2 offset 0xabcdef11"
+gdb_test "print bad_die_val2" \
+         "Bad DW_OP_GNU_variable_value DIE\\."

Re: [PATCH 1/3] Add support for DW_OP_GNU_variable_value

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> This patch adds support for DW_OP_GNU_variable_value to GDB.
Kevin> Jakub Jelinek provides a fairly expansive discussion of this DWARF
Kevin> expression opcode in his GCC patch...

Thank you for doing this.  A few minor comments below.

Kevin> I also have test case (later in this series) which uses the DWARF
Kevin> assembler and, therefore, does not rely on having a compiler with this
Kevin> support.

Very nice.
    
Kevin> +  /* Execute "variable value" operation on DIED at SECT_OFF.  */

I think that should probably read "... on the DIE at ...".

Kevin> +struct value *
Kevin> +sect_variable_value (struct dwarf_expr_context *ctx, sect_offset sect_off,
Kevin> +		     struct dwarf2_per_cu_data *per_cu)

A new function should have an introductory comment explaining the
arguments, return value, and semantics.

Kevin> @@ -3655,6 +3696,9 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
Kevin>  	case DW_OP_call_ref:
Kevin>  	  unimplemented (op);
 
Kevin> +	case DW_OP_GNU_variable_value:
Kevin> +	  unimplemented (op);

Is it possible to implement this?  Just curious, it doesn't seem super
important to me.  In fact, is it ever even useful in an agent
expression?

I think the patch should also update disassemble_dwarf_expression.
It's an oddity that there are two DWARF disassembler in the tree, but
not your problem :)

Tom

Re: [PATCH 2/3] Add support of DW_OP_GNU_variable_value to DWARF assembler

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> gdb/testsuite/ChangeLog:
Kevin>     	* lib/dwarf.exp: Add support for DW_OP_GNU_variable_value.

This is ok.  Thank you.

Tom

Re: [PATCH 3/3] Test case for DW_OP_GNU_variable_value

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

Kevin> gdb/testsuite/ChangeLog:
Kevin>     	* gdb.dwarf2/varval.c: New file.
Kevin>     	* gdb.dwarf2/varval.exp: New file.

This is a really great test case, wow.  Thank you.
This is ok.

Tom

Re: [PATCH 1/3] Add support for DW_OP_GNU_variable_value

[Tom Tromey]

>>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:

I forgot a note earlier...

Kevin> +  /* Note: Things still work when the following test is removed.  This
Kevin> +     test and error is here to conform to the proposed specification.  */
Kevin> +  if (!is_scalar_type (die_type) || TYPE_CODE (die_type) == TYPE_CODE_VOID)
Kevin> +    error (_("Type of DW_OP_GNU_variable_value DIE must be an integer or pointer."));

I suspect is_scalar_type isn't what you want here.
Maybe instead either "is_integral_type || TYPE_CODE == TYPE_CODE_PTR",
or just checking for TYPE_CODE_PTR and TYPE_CODE_INT (since, bizarrely
to me at least, is_integral_type includes range types).

Tom

Re: [PATCH 1/3] Add support for DW_OP_GNU_variable_value

[Kevin Buettner]

On Fri, 03 Aug 2018 12:36:35 -0600
Tom Tromey <tom@tromey.com> wrote:

> Kevin> @@ -3655,6 +3696,9 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
> Kevin>  	case DW_OP_call_ref:
> Kevin>  	  unimplemented (op);  
>  
> Kevin> +	case DW_OP_GNU_variable_value:
> Kevin> +	  unimplemented (op);  
> 
> Is it possible to implement this?  Just curious, it doesn't seem super
> important to me.  In fact, is it ever even useful in an agent
> expression?

When I first started looking at it, it seemed to me that
DW_OP_GNU_variable_value was similar to DW_OP_call_ref.  Implementing
the latter using existing machinery should be possible by compiling an
agent expression.  It might be possible to do something similar for
DW_OP_GNU_variable_value, but it's more complicated since the
referenced DIE might not have DW_AT_location, but instead have
DW_AT_const_value instead.

In any case, my understanding of agent expressions isn't good enough
to attempt an implementation.

> I think the patch should also update disassemble_dwarf_expression.
> It's an oddity that there are two DWARF disassembler in the tree, but
> not your problem :)

I fixed this, plus the other things that you mentioned.  (Thanks for the
quick review!)

This is what I pushed...

commit a6b786da4e3353bf634ec7d9c7bffbd7569e73c6
Author: Kevin Buettner <kevinb@redhat.com>
Date:   Mon Jul 30 15:41:56 2018 -0700

    Add support for DW_OP_GNU_variable_value
    
    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
    
    If compiled with a suitable version of GCC, the test case associated
    with GCC Bug 77589 uses DW_OP_GNU_variable_value in a DW_AT_byte_stride
    expression.  Here's a link to the bug:
    
        https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77589
    
    This is what the DWARF looks like.  Look at the last line, which has
    the DW_AT_byte_stride expression:
    
     <2><e1>: Abbrev Number: 12 (DW_TAG_variable)
        <e2>   DW_AT_name        : (indirect string, offset: 0x115): span.0
        <e6>   DW_AT_type        : <0x2e>
        <ea>   DW_AT_artificial  : 1
        <ea>   DW_AT_location    : 3 byte block: 91 b0 7f 	(DW_OP_fbreg: -80)
     ...
     <2><178>: Abbrev Number: 18 (DW_TAG_subrange_type)
        <179>   DW_AT_lower_bound : 4 byte block: 97 23 20 6 	(DW_OP_push_object_address; DW_OP_plus_uconst: 32; DW_OP_deref)
        <17e>   DW_AT_upper_bound : 4 byte block: 97 23 28 6 	(DW_OP_push_object_address; DW_OP_plus_uconst: 40; DW_OP_deref)
        <183>   DW_AT_byte_stride : 10 byte block: 97 23 18 6 fd e1 0 0 0 1e 	(DW_OP_push_object_address; DW_OP_plus_uconst: 24; DW_OP_deref; DW_OP_GNU_variable_value: <0xe1>; DW_OP_mul)
    
    A patch to readelf, which I'm also submitting, is required to do this
    decoding.
    
    I found that GDB gave me the correct answer for "p c40pt(2)" once I
    (correctly) implemented DW_OP_GNU_variable_value.
    
    I also have test case (later in this series) which uses the DWARF
    assembler and, therefore, do not rely on having a compiler with this
    support.
    
    gdb/ChangeLog:
    
    	* dwarf2expr.h (struct dwarf_expr_context): Add virtual method
    	dwarf_variable_value.
    	* dwarf2-frame.c (class dwarf_expr_executor):
    	Add override for dwarf_variable_value.
    	* dwarf2loc.c (class dwarf_evaluate_loc_desc): Likewise.
    	(class symbol_needs_eval_context): Likewise.
    	(indirect_synthetic_pointer): Add forward declaration.
    	(sect_variable_value): New function.
    	(dwarf2_compile_expr_to_ax): Add case for DW_OP_GNU_variable_value.
    	* dwarf2expr.c (dwarf_expr_context::execute_stack_op): Add case
    	for DW_OP_GNU_variable_value.
---
 gdb/ChangeLog      | 14 ++++++++++++++
 gdb/dwarf2-frame.c |  5 +++++
 gdb/dwarf2expr.c   | 11 +++++++++++
 gdb/dwarf2expr.h   |  3 +++
 gdb/dwarf2loc.c    | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 89 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 9ae0810..c75c0e1 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,17 @@
+2018-08-18  Kevin Buettner  <kevinb@redhat.com>
+
+	* dwarf2expr.h (struct dwarf_expr_context): Add virtual method
+	dwarf_variable_value.
+	* dwarf2-frame.c (class dwarf_expr_executor):
+	Add override for dwarf_variable_value.
+	* dwarf2loc.c (class dwarf_evaluate_loc_desc): Likewise.
+	(class symbol_needs_eval_context): Likewise.
+	(indirect_synthetic_pointer): Add forward declaration.
+	(sect_variable_value): New function.
+	(dwarf2_compile_expr_to_ax): Add case for DW_OP_GNU_variable_value.
+	* dwarf2expr.c (dwarf_expr_context::execute_stack_op): Add case
+	for DW_OP_GNU_variable_value.
+
 2018-08-16  Tom Tromey  <tom@tromey.com>
 
 	* top.c (read_command_file): Update.
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index 58f1ba4..f7dc820 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -284,6 +284,11 @@ 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_GNU_addr_index");
diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
index 445f857..f1ca033 100644
--- a/gdb/dwarf2expr.c
+++ b/gdb/dwarf2expr.c
@@ -1259,6 +1259,17 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr,
 	    this->dwarf_call (cu_off);
 	  }
 	  goto no_push;
+
+	case DW_OP_GNU_variable_value:
+	  {
+	    sect_offset sect_off
+	      = (sect_offset) extract_unsigned_integer (op_ptr,
+	                                                this->ref_addr_size,
+							byte_order);
+	    op_ptr += this->ref_addr_size;
+	    result_val = this->dwarf_variable_value (sect_off);
+	  }
+	  break;
 	
 	case DW_OP_entry_value:
 	case DW_OP_GNU_entry_value:
diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index c746bfe..a98edc9 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -221,6 +221,9 @@ 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
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index a98b676..df2f231 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -61,6 +61,12 @@ static struct call_site_parameter *dwarf_expr_reg_to_entry_parameter
      union call_site_parameter_u kind_u,
      struct dwarf2_per_cu_data **per_cu_return);
 
+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);
+
 /* Until these have formal names, we define these here.
    ref: http://gcc.gnu.org/wiki/DebugFission
    Each entry in .debug_loc.dwo begins with a byte that describes the entry,
@@ -546,6 +552,30 @@ 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.  */
+
+static struct value *
+sect_variable_value (struct dwarf_expr_context *ctx, sect_offset sect_off,
+		     struct dwarf2_per_cu_data *per_cu)
+{
+  struct type *die_type = dwarf2_fetch_die_type_sect_off (sect_off, per_cu);
+
+  if (die_type == NULL)
+    error (_("Bad DW_OP_GNU_variable_value DIE."));
+
+  /* Note: Things still work when the following test is removed.  This
+     test and error is here to conform to the proposed specification.  */
+  if (TYPE_CODE (die_type) != TYPE_CODE_INT
+      && TYPE_CODE (die_type) != TYPE_CODE_PTR)
+    error (_("Type of DW_OP_GNU_variable_value DIE must be an integer or pointer."));
+
+  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);
+}
+
 class dwarf_evaluate_loc_desc : public dwarf_expr_context
 {
  public:
@@ -587,6 +617,14 @@ class dwarf_evaluate_loc_desc : public dwarf_expr_context
     per_cu_dwarf_call (this, die_offset, per_cu);
   }
 
+  /* 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);
+  }
+
   struct type *get_base_type (cu_offset die_offset, int size) override
   {
     struct type *result = dwarf2_get_die_type (die_offset, per_cu);
@@ -2815,6 +2853,14 @@ class symbol_needs_eval_context : public dwarf_expr_context
     per_cu_dwarf_call (this, die_offset, per_cu);
   }
 
+  /* Helper interface of sect_variable_value for
+     dwarf2_loc_desc_get_symbol_read_needs.  */
+
+  struct value *dwarf_variable_value (sect_offset sect_off) override
+  {
+    return sect_variable_value (this, sect_off, per_cu);
+  }
+
   /* DW_OP_entry_value accesses require a caller, therefore a
      frame.  */
 
@@ -3655,6 +3701,9 @@ dwarf2_compile_expr_to_ax (struct agent_expr *expr, struct axs_value *loc,
 	case DW_OP_call_ref:
 	  unimplemented (op);
 
+	case DW_OP_GNU_variable_value:
+	  unimplemented (op);
+
 	default:
 	  unimplemented (op);
 	}
@@ -4280,6 +4329,13 @@ disassemble_dwarf_expression (struct ui_file *stream,
 	  ul = dwarf2_read_addr_index (per_cu, ul);
 	  fprintf_filtered (stream, " %s", pulongest (ul));
 	  break;
+
+	case DW_OP_GNU_variable_value:
+	  ul = extract_unsigned_integer (data, offset_size,
+					 gdbarch_byte_order (arch));
+	  data += offset_size;
+	  fprintf_filtered (stream, " offset %s", phex_nz (ul, offset_size));
+	  break;
 	}
 
       fprintf_filtered (stream, "\n");

Re: [PATCH 3/3] Test case for DW_OP_GNU_variable_value

[Kevin Buettner]

On Fri, 03 Aug 2018 12:40:42 -0600
Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:  
> 
> Kevin> gdb/testsuite/ChangeLog:
> Kevin>     	* gdb.dwarf2/varval.c: New file.
> Kevin>     	* gdb.dwarf2/varval.exp: New file.  
> 
> This is a really great test case, wow.  Thank you.
> This is ok.
> 
> Tom

Pushed.

Re: [PATCH 2/3] Add support of DW_OP_GNU_variable_value to DWARF assembler

[Kevin Buettner]

On Fri, 03 Aug 2018 12:37:14 -0600
Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Kevin" == Kevin Buettner <kevinb@redhat.com> writes:  
> 
> Kevin> gdb/testsuite/ChangeLog:
> Kevin>     	* lib/dwarf.exp: Add support for DW_OP_GNU_variable_value.  
> 
> This is ok.  Thank you.
> 
> Tom

Pushed.

Re: [PATCH 1/3] Add support for DW_OP_GNU_variable_value

[Tom de Vries]

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'.

I've written a gcc demonstrator patch to generate code like this for
VLAs, and found that gdb master (containing this patch series) does not
support this.

Is this further support of DW_OP_GNU_variable_value something you're
currently working on, or plan to work on?

Thanks,
- Tom

Re: [PATCH 1/3] Add support for DW_OP_GNU_variable_value

[Kevin Buettner]

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'.
> 
> I've written a gcc demonstrator patch to generate code like this for
> VLAs, and found that gdb master (containing this patch series) does not
> support this.
> 
> Is this further support of DW_OP_GNU_variable_value something you're
> currently working on, or plan to work on?

Tom and I discussed this briefly on IRC.  Tom says that he'll take
a look at it...

Kevin

[RFC, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1759 bytes --]

[ 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'.
>>
>> I've written a gcc demonstrator patch to generate code like this for
>> VLAs, and found that gdb master (containing this patch series) does not
>> support this.
>>
>> Is this further support of DW_OP_GNU_variable_value something you're
>> currently working on, or plan to work on?
> 
> Tom and I discussed this briefly on IRC.  Tom says that he'll take
> a look at it...
> 

Hi,

sofar I've written:
- a hack that fixes the test-case I have, without regressing anything,
  and
- the rationale for the change,
hoping that this should sufficiently explain what we're trying to implement.

Hints on how to implement are welcome.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-exp-Handle-DW_OP_GNU_variable_value-refs-to-abstract-dies.patch --]
[-- Type: text/x-patch, Size: 7391 bytes --]

[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 contains a hack that handles the case described above.

Build and reg-tested on x86_64-linux.

---
 gdb/dwarf2loc.c  | 10 ++++++----
 gdb/dwarf2loc.h  |  2 +-
 gdb/dwarf2read.c | 28 ++++++++++++++++++++++++++--
 3 files changed, 33 insertions(+), 7 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 8834d08a1c..3d214b7cee 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -92,6 +92,8 @@
 #include "rust-lang.h"
 #include "common/pathstuff.h"
 
+std::unordered_map<struct die_info *,struct die_info *> abstract_to_concrete;
+
 /* When == 1, print basic high level tracing messages.
    When > 1, be more verbose.
    This is in contrast to the low level DIE reading of dwarf_die_debug.  */
@@ -14283,7 +14285,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);
+  if (res == NULL && dwarf2_attr (die, DW_AT_location, cu))
+    {
+      struct attribute *attr = dwarf2_attr (die, DW_AT_abstract_origin, cu);
+      if (attr)
+	{
+	  struct dwarf2_cu *origin_cu = cu;
+	  struct die_info *origin_die = follow_die_ref (die, attr, &origin_cu);
+	  /* 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.  Hack: assume 1-on-1 relationship.  Of course,
+	     in reality one abstract variable may have many concrete instances.
+	     TODO: Fix this hack by storing into a 1-to-many data structure.  */
+	  abstract_to_concrete[origin_die] = die;
+	}
+    }
 }
 
 /* Call CALLBACK from DW_AT_ranges attribute value OFFSET
@@ -22998,7 +23016,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;
@@ -23023,6 +23041,12 @@ dwarf2_fetch_die_loc_sect_off (sect_offset sect_off,
     error (_("Dwarf Error: Cannot find DIE at %s referenced in module %s"),
 	   sect_offset_str (sect_off), objfile_name (objfile));
 
+  if (resolve_abstract_p
+      && abstract_to_concrete.find (die) != abstract_to_concrete.end ())
+    /* TODO: Rewrite abstract_to_concrete into a 1-to-many data structure, and
+       find the appropriate concrete instance.  */
+    die = abstract_to_concrete[die];
+
   attr = dwarf2_attr (die, DW_AT_location, cu);
   if (!attr)
     {

Re: [RFC, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Richard Biener]

On Fri, 24 Aug 2018, 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'.
> >>
> >> I've written a gcc demonstrator patch to generate code like this for
> >> VLAs, and found that gdb master (containing this patch series) does not
> >> support this.
> >>
> >> Is this further support of DW_OP_GNU_variable_value something you're
> >> currently working on, or plan to work on?
> > 
> > Tom and I discussed this briefly on IRC.  Tom says that he'll take
> > a look at it...
> > 
> 
> Hi,
> 
> sofar I've written:
> - a hack that fixes the test-case I have, without regressing anything,
>   and
> - the rationale for the change,
> hoping that this should sufficiently explain what we're trying to implement.
> 
> Hints on how to implement are welcome.

I wonder if there isn't already support to lookup 'd' when the
DIE of the concrete instance just has the location and refers to
'd' via an abstract origin.  So the idea is to do this kind of
lookup from the original context of the conrete instance we came
from when evaluating DW_OP_GNU_variable_value on the 
abstract instance DIE.

Btw, apart from the use with early debug / LTO this would make it
possible to remove repeating VLA types in inline instances.  If you
build the following with -O -g you get three instances of int[n]
DIEs, one in the abstract copy and two generated for the
DW_TAG_inlined_subroutine in bar.  Ideally we could elide those,
having the type of a fully specified in the abstract instance
and the concrete instances providing locations for n.

static inline void foo (int n) { int a[n]; }
int bar(int n1, int n2)
{
  foo (n1);
  foo (n2);
}

Richard.

[PATCH, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

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

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-exp-Handle-DW_OP_GNU_variable_value-refs-to-abstract-dies.patch --]
[-- Type: text/x-patch, Size: 10031 bytes --]

[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/dwarf2loc.c  | 10 ++++++----
 gdb/dwarf2loc.h  |  2 +-
 gdb/dwarf2read.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/dwarf2read.h |  6 ++++++
 4 files changed, 67 insertions(+), 7 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.  */

Re: [PATCH, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

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


[-- Attachment #2: 0001-gdb-exp-Handle-DW_OP_GNU_variable_value-refs-to-abstract-dies.patch --]
[-- Type: text/x-patch, Size: 11976 bytes --]

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

Re: [PATCH, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Kevin Buettner]

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

Re: [PATCH, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 3952 bytes --]

On 09/04/2018 11:54 PM, Kevin Buettner wrote:
> 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.
> 

Done.

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

Done.

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

No, I just used what was most familiar to me.

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

Done.

> Also, can you add a comment for abstract_to_concrete?
> 

Done.

Committed as attached.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-exp-Handle-DW_OP_GNU_variable_value-refs-to-abstract-dies.patch --]
[-- Type: text/x-patch, Size: 11457 bytes --]

[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 (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.
	(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                    | 43 +++++++++++++++++++++++++++++++++++--
 gdb/dwarf2read.h                    |  8 +++++++
 gdb/testsuite/gdb.dwarf2/varval.exp | 22 ++++++++++++++++++-
 5 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 200fa03f46..84e3c3ca69 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 resolve_abstract_p = 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..d02e3cd354 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 resolve_abstract_p = 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..4a35e389e9 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -14283,7 +14283,22 @@ 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;
+      dpo->abstract_to_concrete[origin_die].push_back (die);
+    }
 }
 
 /* Call CALLBACK from DW_AT_ranges attribute value OFFSET
@@ -23010,7 +23025,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 +23051,30 @@ 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);
+
+      for (const auto &cand : dwarf2_per_objfile->abstract_to_concrete[die])
+	{
+	  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..67ebc1902b 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,8 @@ struct dwarf2_debug_sections;
 struct mapped_index;
 struct mapped_debug_names;
 struct signatured_type;
+struct die_info;
+typedef struct die_info *die_info_ptr;
 
 /* Collection of data recorded per objfile.
    This hangs off of dwarf2_objfile_data_key.  */
@@ -250,6 +253,11 @@ 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;
+
+  /* Mapping from abstract origin DIE to concrete DIEs that reference it as
+     DW_AT_abstract_origin.  */
+  std::unordered_map<die_info_ptr, std::vector<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>"

Re: [PATCH, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Tom Tromey]

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

Re: [PATCH, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Tom de Vries]

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

Re: [PATCH, gdb/exp] Handle DW_OP_GNU_variable_value refs to abstract dies

[Tom Tromey]

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