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

[Tom de Vries <tdevries@suse.de>]

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