[PATCH v3 0/1] gdb: Fix assertion in 'value_primitive_field'

[PATCH v3 0/1] gdb: Fix assertion in 'value_primitive_field'

[Stephan Rohr]

Hi all,

this is version 3 of my patch.

V2 of my proposed patch can be found here:
https://sourceware.org/pipermail/gdb-patches/2024-March/207018.html

V1 of this series can be found here:
https://sourceware.org/pipermail/gdb-patches/2024-February/206520.html

Changes since V2:
* Updated testcase to enable testcase if compiler options
  '-D_GLIBCXX_DEBUG=1' and '-D_GLIBCXX_DEBUG_PEDANTIC=1'
  are used.

Changes since V1:
* Added a testcase.
* Rebased change to GDB-14.

I appreciate your feedback.

Thanks
Stephan

Rohr, Stephan (1):
  gdb: Fix assertion in 'value_primitive_field'

 .../locexpr-data-member-dynamic-location.c    |  32 +++++
 .../locexpr-data-member-dynamic-location.exp  | 129 ++++++++++++++++++
 gdb/value.c                                   |   4 +-
 3 files changed, 164 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.exp

-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCH v3 1/1] gdb: Fix assertion in 'value_primitive_field'

[Stephan Rohr]

From: "Rohr, Stephan" <stephan.rohr@intel.com>

GDB asserts that the data location of a value's field with a dynamic
data location is resolved when fetching the field's value in
'value_primitive_field'.  This assertion was hit because of bogus DWARF
generated by the Intel Fortran compiler.  While that compiler should fix
the DWARF, we prefer GDB to error out here instead, e.g. to allow the
user to continue debugging other parts of the program.
---
 .../locexpr-data-member-dynamic-location.c    |  32 +++++
 .../locexpr-data-member-dynamic-location.exp  | 129 ++++++++++++++++++
 gdb/value.c                                   |   4 +-
 3 files changed, 164 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.exp

diff --git a/gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.c b/gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.c
new file mode 100644
index 00000000000..dbe591ab884
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.c
@@ -0,0 +1,32 @@
+/* Copyright 2024 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/>.  */
+
+struct container
+{
+  int *data;
+};
+
+int size = 5;
+
+int var_a_data[] = {5, 8, 13, 21, 34};
+struct container var_a = {var_a_data};
+
+int
+main (void)
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.exp b/gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.exp
new file mode 100644
index 00000000000..1dd2e91ffdd
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/locexpr-data-member-dynamic-location.exp
@@ -0,0 +1,129 @@
+# Copyright 2024 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/>.
+#
+# Tests that GDB prints an error when fetching a value's (dynamic) field
+# with unresolved data location.  For a reproducer, we setup the following
+# struct type:
+#
+#   struct container
+#     {
+#       int *data;
+#     };
+#
+# We use a variable 'size' to define the upper bound 'DW_AT_upper_bound' for
+# the non-static field 'data'.  The error reproduces with invalid DWARF, i.e.
+# when pushing the memory location of an instance 'var_a' of struct
+# 'container' on the DWARF stack:
+#
+#   DW_AT_location {
+#     DW_OP_addr [gdb_target_symbol var_a]
+#     DW_OP_deref
+#     DW_OP_stack_value
+#   } SPECIAL_expr}
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+if {![dwarf2_support]} {
+    return 0
+}
+
+standard_testfile .c -dw.S
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+# Make some DWARF for the test.
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    cu {} {
+	DW_TAG_compile_unit {} {
+	    declare_labels integer_label array_label struct_label var_a_label
+	    set int_size [get_sizeof "int" 4]
+	    set addr_size [get_sizeof "void *" 8]
+
+	    integer_label: DW_TAG_base_type {
+		{DW_AT_byte_size $int_size DW_FORM_sdata}
+		{DW_AT_encoding  @DW_ATE_signed}
+	    }
+
+	    array_label: DW_TAG_array_type {
+		{DW_AT_type :$integer_label}
+		{DW_AT_data_location {
+		    DW_OP_push_object_address
+		    DW_OP_deref
+		} SPECIAL_expr}
+	    } {
+		DW_TAG_subrange_type {
+		    {DW_AT_type :$integer_label}
+		    {DW_AT_upper_bound {
+			DW_OP_addr [gdb_target_symbol size]
+			DW_OP_deref
+			DW_OP_lit1
+			DW_OP_minus
+		    } SPECIAL_expr}
+		}
+	    }
+
+	    struct_label: DW_TAG_structure_type {
+		{DW_AT_byte_size 8 DW_FORM_udata}
+	    } {
+		DW_TAG_member {
+		    {DW_AT_name "data"}
+		    {DW_AT_type :${array_label}}
+		    {DW_AT_data_member_location 0 DW_FORM_udata}
+		}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name var_a_valid_location}
+		{DW_AT_type :$struct_label}
+		{DW_AT_location {
+		    DW_OP_addr [gdb_target_symbol var_a]
+		} SPECIAL_expr}
+	    }
+
+	    DW_TAG_variable {
+		{DW_AT_name var_a_invalid_location}
+		{DW_AT_type :$struct_label}
+		{DW_AT_location {
+		    DW_OP_addr [gdb_target_symbol var_a]
+		    DW_OP_deref
+		    DW_OP_stack_value
+		} SPECIAL_expr}
+	    }
+	}
+    }
+}
+
+# Now that we've generated the DWARF debugging info, rebuild our
+# program using our debug info instead of the info generated by
+# the compiler.
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "print var_a_valid_location" \
+	 " = \\{data = \\{5, 8, 13, 21, 34\\}\\}"
+
+gdb_test "print var_a_invalid_location" \
+	 " = \\{data = <error reading variable: cannot read data.*"
diff --git a/gdb/value.c b/gdb/value.c
index a2b2721d183..145e70074a9 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3060,7 +3060,9 @@ value::primitive_field (LONGEST offset, int fieldno, struct type *arg_type)
 
       gdb_assert (0 == offset);
       /* We expect an already resolved data location.  */
-      gdb_assert (TYPE_DATA_LOCATION (type)->is_constant ());
+      if (!TYPE_DATA_LOCATION (type)->is_constant ())
+	error (_("cannot read %s, expected an already resolved data "
+		 "location."), arg_type->field (fieldno).name ());
       /* For dynamic data types defer memory allocation
 	 until we actual access the value.  */
       v = value::allocate_lazy (type);
-- 
2.34.1

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v3 1/1] gdb: Fix assertion in 'value_primitive_field'

[Tom Tromey]

>>>>> Stephan Rohr <stephan.rohr@intel.com> writes:

> From: "Rohr, Stephan" <stephan.rohr@intel.com>
> GDB asserts that the data location of a value's field with a dynamic
> data location is resolved when fetching the field's value in
> 'value_primitive_field'.  This assertion was hit because of bogus DWARF
> generated by the Intel Fortran compiler.  While that compiler should fix
> the DWARF, we prefer GDB to error out here instead, e.g. to allow the
> user to continue debugging other parts of the program.

I have been trying to understand this patch for a while but I really
don't.  Could you explain it in more detail, preferably in the commit
message?

> +	    DW_TAG_variable {
> +		{DW_AT_name var_a_invalid_location}
> +		{DW_AT_type :$struct_label}
> +		{DW_AT_location {
> +		    DW_OP_addr [gdb_target_symbol var_a]
> +		    DW_OP_deref
> +		    DW_OP_stack_value
> +		} SPECIAL_expr}
> +	    }

For example, I don't understand what is invalid about this.
It's weird for sure, but why is gdb's failure to evaluate it anything
other than a bug somewhere else in gdb?

In the "valid" case I see the field type being resolved.  Why doesn't
this happen in the invalid case?

> +	error (_("cannot read %s, expected an already resolved data "
> +		 "location."), arg_type->field (fieldno).name ());

This error shouldn't have a trailing ".", it makes the output look
weird:

$1 = {data = <error reading variable: cannot read data, expected an already resolved data location.>

Tom