[PATCH][gdb/symtab] Fix element type modification in read_array_type

[PATCH][gdb/symtab] Fix element type modification in read_array_type

[Tom de Vries]

Hi,

When running test-case gdb.fortran/function-calls.exp with target board
unix/gdb:debug_flags=-gdwarf-5, I run into:
...
(gdb) PASS: gdb.fortran/function-calls.exp: \
  p derived_types_and_module_calls::pass_cart(c)
p derived_types_and_module_calls::pass_cart_nd(c_nd)^M
^M
Program received signal SIGSEGV, Segmentation fault.^M
0x0000000000400f73 in derived_types_and_module_calls::pass_cart_nd \
  (c=<error reading variable: Cannot access memory at address 0xc>) at \
  function-calls.f90:130^M
130             pass_cart_nd = ubound(c%d,1,4)^M
The program being debugged was signaled while in a function called from GDB.^M
GDB has restored the context to what it was before the call.^M
To change this behavior use "set unwindonsignal off".^M
Evaluation of the expression containing the function^M
(derived_types_and_module_calls::pass_cart_nd) will be abandoned.^M
(gdb) FAIL: gdb.fortran/function-calls.exp: p
...

The problem originates in read_array_type, when reading a DW_TAG_array_type
with a dwarf-5 DW_TAG_generic_subrange child.  This is not supported, and the
fallout of this is that rather than constructing a new array type, the code
proceeds to modify the element type.

Fix this conservatively by bailing out in read_array_type when not being able
to construct an array type.

Tested on x86_64-linux.

Any comments?

Thanks,
- Tom

[gdb/symtab] Fix element type modification in read_array_type

gdb/ChangeLog:

2021-02-05  Tom de Vries  <tdevries@suse.de>

	PR symtab/27341
	* dwarf2/read.c (read_array_type): Return NULL when not being able to
	construct an array type.

---
 gdb/dwarf2/read.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index f60e418172c..98189f7b566 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17243,6 +17243,9 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
+  if (type == element_type)
+    return NULL;
+
   /* Understand Dwarf2 support for vector types (like they occur on
      the PowerPC w/ AltiVec).  Gcc just adds another attribute to the
      array type.  This is not part of the Dwarf2/3 standard yet, but a

Re: [PATCH][gdb/symtab] Fix element type modification in read_array_type

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this conservatively by bailing out in read_array_type when not being able
Tom> to construct an array type.

Perhaps it should issue a complaint?

It seems to me though that if gcc emits DW_TAG_generic_subrange, then
gdb ought to handle it.

Tom

Re: [PATCH][gdb/symtab] Fix element type modification in read_array_type

[Tom de Vries]

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

On 2/5/21 5:30 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix this conservatively by bailing out in read_array_type when not being able
> Tom> to construct an array type.
> 
> Perhaps it should issue a complaint?
> 

Indeed.  I've added one.

> It seems to me though that if gcc emits DW_TAG_generic_subrange, then
> gdb ought to handle it.

True, but that's not the issue I'm trying to handle here.  I'm trying to
fix the bug that the element type is modified, which could also happen
f.i. when an array type without any subrange children is processed.

I want to file an enhancement PR for supporting DW_TAG_generic_subrange,
which I then can mention in the KFAIL I've also added.

WDYT?

Thanks,
- Tom

[-- Attachment #2: 0006-gdb-symtab-Fix-element-type-modification-in-read_array_type.patch --]
[-- Type: text/x-patch, Size: 6759 bytes --]

[gdb/symtab] Fix element type modification in read_array_type

When running test-case gdb.fortran/function-calls.exp with target board
unix/gdb:debug_flags=-gdwarf-5, I run into:
...
(gdb) PASS: gdb.fortran/function-calls.exp: \
  p derived_types_and_module_calls::pass_cart(c)
p derived_types_and_module_calls::pass_cart_nd(c_nd)^M
^M
Program received signal SIGSEGV, Segmentation fault.^M
0x0000000000400f73 in derived_types_and_module_calls::pass_cart_nd \
  (c=<error reading variable: Cannot access memory at address 0xc>) at \
  function-calls.f90:130^M
130             pass_cart_nd = ubound(c%d,1,4)^M
The program being debugged was signaled while in a function called from GDB.^M
GDB has restored the context to what it was before the call.^M
To change this behavior use "set unwindonsignal off".^M
Evaluation of the expression containing the function^M
(derived_types_and_module_calls::pass_cart_nd) will be abandoned.^M
(gdb) FAIL: gdb.fortran/function-calls.exp: p
...

The problem originates in read_array_type, when reading a DW_TAG_array_type
with a dwarf-5 DW_TAG_generic_subrange child.  This is not supported, and the
fallout of this is that rather than constructing a new array type, the code
proceeds to modify the element type.

Fix this conservatively by issuing a complaint and bailing out in
read_array_type when not being able to construct an array type, such that we
have:
...
(gdb) maint expand-symtabs function-calls.f90^M
During symbol reading: unable to handle array child DIE \
  DW_TAG_generic_subrange - DIE at 0xe1e [in module function-calls]^M
During symbol reading: unable to handle array child DIE \
  DW_TAG_generic_subrange - DIE at 0xe1e [in module function-calls]^M
(gdb) KFAIL: gdb.fortran/function-calls.exp: no complaints in srcfile \
  (PRMS: symtab/nnnnn)
...

Tested on x86_64-linux.

gdb/ChangeLog:

2021-02-06  Tom de Vries  <tdevries@suse.de>

	PR symtab/27341
	* dwarf2/read.c (read_array_type): Return NULL when not being able to
	construct an array type.  Add assert to ensure that element_type is
	not being modified.

2021-02-06  Tom de Vries  <tdevries@suse.de>

	PR symtab/27341
	* lib/gdb.exp (with_complaints): New proc, factored out of ...
	(gdb_load_no_complaints): ... here.
	* gdb.fortran/function-calls.exp: Add test-case.

---
 gdb/dwarf2/read.c                            | 11 +++++++
 gdb/testsuite/gdb.fortran/function-calls.exp | 16 ++++++++++
 gdb/testsuite/lib/gdb.exp                    | 46 ++++++++++++++++++++--------
 3 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b0b37358866..2f8869bd11c 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17246,6 +17246,15 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
 	      range_types.push_back (child_type);
 	    }
 	}
+      else
+	{
+	  complaint (_("unable to handle array child DIE %s"
+		       " - DIE at %s [in module %s]"),
+		     dwarf_tag_name (child_die->tag),
+		     sect_offset_str (die->sect_off),
+		     objfile_name (cu->per_objfile->objfile));
+	  return NULL;
+	}
       child_die = child_die->sibling;
     }
 
@@ -17278,6 +17287,8 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
+  gdb_assert (type != element_type);
+
   /* Understand Dwarf2 support for vector types (like they occur on
      the PowerPC w/ AltiVec).  Gcc just adds another attribute to the
      array type.  This is not part of the Dwarf2/3 standard yet, but a
diff --git a/gdb/testsuite/gdb.fortran/function-calls.exp b/gdb/testsuite/gdb.fortran/function-calls.exp
index a14cfaeb08a..d1d781ff59f 100644
--- a/gdb/testsuite/gdb.fortran/function-calls.exp
+++ b/gdb/testsuite/gdb.fortran/function-calls.exp
@@ -24,6 +24,22 @@ if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug f90}]} {
     return -1
 }
 
+with_complaints 5 {
+    set cmd "maint expand-symtabs $srcfile"
+    set cmd_regexp [string_to_regexp $cmd]
+    set re_kfail [concat "During symbol reading:" \
+		      " unable to handle array child" \
+		      " DIE DW_TAG_generic_subrange"]
+    gdb_test_multiple $cmd "no complaints in srcfile" {
+	-re -wrap "$re_kfail.*" {
+	    kfail symtab/nnnnn $gdb_test_name
+	}
+        -re "^$cmd_regexp\r\n$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
 if {![runto [gdb_get_line_number "post_init"]]} then {
     perror "couldn't run to breakpoint post_init"
     continue
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index dbbf239dab9..69a81353b05 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5096,11 +5096,11 @@ proc gdb_load { arg } {
 }
 
 #
-# gdb_load_no_complaints -- As gdb_load, but in addition verifies that
-# loading caused no symbol reading complaints.
+# with_complaints -- Execute BODY and set complaints temporary to N for the
+# duration.
 #
-proc gdb_load_no_complaints { arg } {
-    global gdb_prompt gdb_file_cmd_msg decimal
+proc with_complaints { n body } {
+    global decimal
 
     # Save current setting of complaints.
     set save ""
@@ -5112,23 +5112,43 @@ proc gdb_load_no_complaints { arg } {
 	}
     }
 
-    # Fall back to regular gdb_load if we couldn't get the current setting
-    # of complaints.
     if { $save == "" } {
-	return gdb_load $arg
+	perror "Did not manage to set complaints"
+    } else {
+	# Set complaints.
+	gdb_test_no_output "set complaints $n" ""
     }
 
-    # Temporarily set complaint to a small non-zero number.
-    gdb_test_no_output "set complaints 5" ""
+    set code [catch {uplevel 1 $body} result]
+
+    # Restore saved setting of complaints.
+    if { $save != "" } {
+	gdb_test_no_output "set complaints $save" ""
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
+    } else {
+	return -code $code $result
+    }
+}
 
-    gdb_load $arg
+#
+# gdb_load_no_complaints -- As gdb_load, but in addition verifies that
+# loading caused no symbol reading complaints.
+#
+proc gdb_load_no_complaints { arg } {
+    global gdb_prompt gdb_file_cmd_msg decimal
+
+    # Temporarily set complaint to a small non-zero number.
+    with_complaints 5 {
+	gdb_load $arg
+    }
 
     # Verify that there were no complaints.
     set re "\r\nDuring symbol reading: "
     gdb_assert {![regexp $re $gdb_file_cmd_msg]} "No complaints"
-
-    # Restore saved setting of complaints.
-    gdb_test_no_output "set complaints $save" ""
 }
 
 # gdb_reload -- load a file into the target.  Called before "running",

Re: [PATCH][gdb/symtab] Fix element type modification in read_array_type

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +	{
Tom> +	  complaint (_("unable to handle array child DIE %s"
Tom> +		       " - DIE at %s [in module %s]"),
Tom> +		     dwarf_tag_name (child_die->tag),
Tom> +		     sect_offset_str (die->sect_off),
Tom> +		     objfile_name (cu->per_objfile->objfile));
Tom> +	  return NULL;
Tom> +	}

This seems a bit too strong to me.  A normal DWARF extension mechanism
is to add new "unexpected" children to a DIE.  In many (maybe most)
cases, it's fine to just ignore these.

Maybe another approach would be to bail out if range_types.empty(),
issuing a complaint then.  This wouldn't mention
DW_TAG_generic_subrange, but that seems ok to me.
Users don't really read complaints.  Maybe even gdb developers don't ;)

Tom

Re: [PATCH][gdb/symtab] Fix element type modification in read_array_type

[Tom de Vries]

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

On 2/9/21 9:24 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +	{
> Tom> +	  complaint (_("unable to handle array child DIE %s"
> Tom> +		       " - DIE at %s [in module %s]"),
> Tom> +		     dwarf_tag_name (child_die->tag),
> Tom> +		     sect_offset_str (die->sect_off),
> Tom> +		     objfile_name (cu->per_objfile->objfile));
> Tom> +	  return NULL;
> Tom> +	}
> 
> This seems a bit too strong to me.  A normal DWARF extension mechanism
> is to add new "unexpected" children to a DIE.  In many (maybe most)
> cases, it's fine to just ignore these.
> 
> Maybe another approach would be to bail out if range_types.empty(),
> issuing a complaint then.  This wouldn't mention
> DW_TAG_generic_subrange, but that seems ok to me.

Ack, done.

> Users don't really read complaints.  Maybe even gdb developers don't ;)

Heh :)

Committed as attached.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-symtab-Fix-element-type-modification-in-read_array_type.patch --]
[-- Type: text/x-patch, Size: 6269 bytes --]

[gdb/symtab] Fix element type modification in read_array_type

When running test-case gdb.fortran/function-calls.exp with target board
unix/gdb:debug_flags=-gdwarf-5, I run into:
...
(gdb) PASS: gdb.fortran/function-calls.exp: \
  p derived_types_and_module_calls::pass_cart(c)
p derived_types_and_module_calls::pass_cart_nd(c_nd)^M
^M
Program received signal SIGSEGV, Segmentation fault.^M
0x0000000000400f73 in derived_types_and_module_calls::pass_cart_nd \
  (c=<error reading variable: Cannot access memory at address 0xc>) at \
  function-calls.f90:130^M
130             pass_cart_nd = ubound(c%d,1,4)^M
The program being debugged was signaled while in a function called from GDB.^M
GDB has restored the context to what it was before the call.^M
To change this behavior use "set unwindonsignal off".^M
Evaluation of the expression containing the function^M
(derived_types_and_module_calls::pass_cart_nd) will be abandoned.^M
(gdb) FAIL: gdb.fortran/function-calls.exp: p
...

The problem originates in read_array_type, when reading a DW_TAG_array_type
with a dwarf-5 DW_TAG_generic_subrange child.  This is not supported, and the
fallout of this is that rather than constructing a new array type, the code
proceeds to modify the element type.

Fix this conservatively by issuing a complaint and bailing out in
read_array_type when not being able to construct an array type, such that we
have:
...
(gdb) maint expand-symtabs function-calls.f90^M
During symbol reading: unable to find array range \
  - DIE at 0xe1e [in module function-calls]^M
During symbol reading: unable to find array range \
  - DIE at 0xe1e [in module function-calls]^M
(gdb) KFAIL: gdb.fortran/function-calls.exp: no complaints in srcfile \
  (PRMS: symtab/27388)
...

Tested on x86_64-linux.

gdb/ChangeLog:

2021-02-06  Tom de Vries  <tdevries@suse.de>

	PR symtab/27341
	* dwarf2/read.c (read_array_type): Return NULL when not being able to
	construct an array type.  Add assert to ensure that element_type is
	not being modified.

2021-02-06  Tom de Vries  <tdevries@suse.de>

	PR symtab/27341
	* lib/gdb.exp (with_complaints): New proc, factored out of ...
	(gdb_load_no_complaints): ... here.
	* gdb.fortran/function-calls.exp: Add test-case.

---
 gdb/dwarf2/read.c                            | 10 +++++++
 gdb/testsuite/gdb.fortran/function-calls.exp | 15 ++++++++++
 gdb/testsuite/lib/gdb.exp                    | 43 +++++++++++++++++++++-------
 3 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 3f60ce6a4f1..4b901c565f8 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -17217,6 +17217,14 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
       child_die = child_die->sibling;
     }
 
+  if (range_types.empty ())
+    {
+      complaint (_("unable to find array range - DIE at %s [in module %s]"),
+		 sect_offset_str (die->sect_off),
+		 objfile_name (cu->per_objfile->objfile));
+      return NULL;
+    }
+
   /* Dwarf2 dimensions are output from left to right, create the
      necessary array types in backwards order.  */
 
@@ -17246,6 +17254,8 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
 	}
     }
 
+  gdb_assert (type != element_type);
+
   /* Understand Dwarf2 support for vector types (like they occur on
      the PowerPC w/ AltiVec).  Gcc just adds another attribute to the
      array type.  This is not part of the Dwarf2/3 standard yet, but a
diff --git a/gdb/testsuite/gdb.fortran/function-calls.exp b/gdb/testsuite/gdb.fortran/function-calls.exp
index a14cfaeb08a..00036d96c7c 100644
--- a/gdb/testsuite/gdb.fortran/function-calls.exp
+++ b/gdb/testsuite/gdb.fortran/function-calls.exp
@@ -24,6 +24,21 @@ if {[prepare_for_testing ${testfile}.exp ${testfile} ${srcfile} {debug f90}]} {
     return -1
 }
 
+with_complaints 5 {
+    set cmd "maint expand-symtabs $srcfile"
+    set cmd_regexp [string_to_regexp $cmd]
+    set re_kfail [concat "During symbol reading:" \
+		      " unable to find array range"]
+    gdb_test_multiple $cmd "no complaints in srcfile" {
+	-re -wrap "$re_kfail.*" {
+	    kfail symtab/27388 $gdb_test_name
+	}
+        -re "^$cmd_regexp\r\n$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
 if {![runto [gdb_get_line_number "post_init"]]} then {
     perror "couldn't run to breakpoint post_init"
     continue
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 53ac9f1408c..1406b917151 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5096,11 +5096,11 @@ proc gdb_load { arg } {
 }
 
 #
-# gdb_load_no_complaints -- As gdb_load, but in addition verifies that
-# loading caused no symbol reading complaints.
+# with_complaints -- Execute BODY and set complaints temporary to N for the
+# duration.
 #
-proc gdb_load_no_complaints { arg } {
-    global gdb_prompt gdb_file_cmd_msg decimal
+proc with_complaints { n body } {
+    global decimal
 
     # Save current setting of complaints.
     set save ""
@@ -5112,16 +5112,39 @@ proc gdb_load_no_complaints { arg } {
 	}
     }
 
-    # Fall back to regular gdb_load if we couldn't get the current setting
-    # of complaints.
     if { $save == "" } {
-	return gdb_load $arg
+	perror "Did not manage to set complaints"
+    } else {
+	# Set complaints.
+	gdb_test_no_output "set complaints $n" ""
     }
 
-    # Temporarily set complaint to a small non-zero number.
-    gdb_test_no_output "set complaints 5" ""
+    set code [catch {uplevel 1 $body} result]
+
+    # Restore saved setting of complaints.
+    if { $save != "" } {
+	gdb_test_no_output "set complaints $save" ""
+    }
+
+    if {$code == 1} {
+	global errorInfo errorCode
+	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
+    } else {
+	return -code $code $result
+    }
+}
+
+#
+# gdb_load_no_complaints -- As gdb_load, but in addition verifies that
+# loading caused no symbol reading complaints.
+#
+proc gdb_load_no_complaints { arg } {
+    global gdb_prompt gdb_file_cmd_msg decimal
 
-    gdb_load $arg
+    # Temporarily set complaint to a small non-zero number.
+    with_complaints 5 {
+	gdb_load $arg
+    }
 
     # Verify that there were no complaints.
     set re "^Reading symbols from \[^\r\n\]*\r\n$gdb_prompt $"

Re: [PATCH][gdb/symtab] Fix element type modification in read_array_type

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> 2021-02-06  Tom de Vries  <tdevries@suse.de>

Tom> 	PR symtab/27341
Tom> 	* dwarf2/read.c (read_array_type): Return NULL when not being able to
Tom> 	construct an array type.  Add assert to ensure that element_type is
Tom> 	not being modified.

Looks good.  Thank you.

Tom