Re: [PATCH v4] Dwarf: Don't add nameless modules to partial symbol table.

[Pedro Alves <palves at redhat dot com>]

Hi,

Looks like this has been reviewed a number of times over
the years but never managed to get merged.  I was asked
internally at Red Hat to take a look, and I've merged it now.

I confirmed that this version addresses Joel's comments to v3.

See more below.

On 2/27/19 2:21 PM, Tomasz Kulasek wrote:
> From: Bernhard Heckel <bernhard.heckel@intel.com>
> 
> A name for BLOCK DATA in Fortran is optional.  If no
> name has been assigned, GDB will crash during read-in of DWARF
> when BLOCK DATA is represented via DW_TAG_module.
> BLOCK DATA is used for one-time initialization of
> non-pointer variables in named common blocks.
> 
> As of now there is no issue when gfortran is used as
> DW_TAG_module is not emited.  For Intel ifort the nameless
> DW_TAG_module is present and have following form:
> 
> ...
>  <1><dd>: Abbrev Number: 7 (DW_TAG_module)
>     <de>   DW_AT_decl_line   : 46
>     <df>   DW_AT_decl_file   : 1
>     <e0>   DW_AT_description : (indirect string, offset: 0x110): block
> data
>     <e4>   DW_AT_high_pc     : 0x402bb7
>     <ec>   DW_AT_low_pc      : 0x402bb7
> ...
> 
> Missing name lead to crash in add_partial_symbol function
> during length calculation.
> 
> 2016-06-15  Bernhard Heckel  <bernhard.heckel@intel.com>
> 
> gdb/Changelog:
> 	* dwarf2read.c (add_partial_symbol): Skip nameless modules.
> 
> gdb/testsuite/Changelog:
> 	* gdb.fortran/block-data.f: New.
> 	* gdb.fortran/block-data.exp: New.
> 
> Changes from V1 to V2:
> 	* No changes - just sent with other patches in series.
> 
> Changes from V2 to V3:
> 	* Decouple from patch series.
> 	* Provide example DW_TAG_module in commit message.
> 	* Fix mistakes in comments.
> 
> Changes from V3 to V4:
> 	* Dont complain if module have no name in read_module_type.
> 	* Fix period excapeing in block-data.exp.
> ---
>  gdb/dwarf2read.c                         | 16 ++++-----
>  gdb/testsuite/gdb.fortran/block-data.exp | 49 ++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.fortran/block-data.f   | 56 ++++++++++++++++++++++++++++++++
>  3 files changed, 113 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.fortran/block-data.exp
>  create mode 100644 gdb/testsuite/gdb.fortran/block-data.f
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 98f46e0..3b0274d 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -9018,11 +9018,14 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
>  			   0, cu->language, objfile);
>        break;
>      case DW_TAG_module:
> -      add_psymbol_to_list (actual_name, strlen (actual_name),
> -			   built_actual_name != NULL,
> -			   MODULE_DOMAIN, LOC_TYPEDEF, -1,
> -			   psymbol_placement::GLOBAL,
> -			   0, cu->language, objfile);
> +      /* In Fortran 77 there might be a "BLOCK DATA" module available wihout

Typo "wihout".

> +         any name.  If so, we skip the module as it doesn't bring any value.  */
> +      if (actual_name != nullptr)
> +	add_psymbol_to_list (actual_name, strlen (actual_name),
> +			     built_actual_name != NULL,
> +			     MODULE_DOMAIN, LOC_TYPEDEF, -1,
> +			     psymbol_placement::GLOBAL,
> +			     0, cu->language, objfile);
>        break;
>      case DW_TAG_class_type:
>      case DW_TAG_interface_type:
> @@ -16929,9 +16932,6 @@ read_module_type (struct die_info *die, struct dwarf2_cu *cu)
>    struct type *type;
>  
>    module_name = dwarf2_name (die, cu);
> -  if (!module_name)
> -    complaint (_("DW_TAG_module has no name, offset %s"),
> -               sect_offset_str (die->sect_off));
>    type = init_type (objfile, TYPE_CODE_MODULE, 0, module_name);
>  
>    return set_die_type (die, type, cu);
> diff --git a/gdb/testsuite/gdb.fortran/block-data.exp b/gdb/testsuite/gdb.fortran/block-data.exp
> new file mode 100644
> index 0000000..389ae15
> --- /dev/null
> +++ b/gdb/testsuite/gdb.fortran/block-data.exp
> @@ -0,0 +1,49 @@
> +# Copyright 2019 Free Software Foundation, Inc.

This file was first posted in 2016, so the copyright years should include
that.

> +
> +gdb_test "print doub1" "= 1\.11\\d+" "print doub1, default values"
> +gdb_test "print doub2" "= 2\.22\\d+" "print doub2, default values"
> +gdb_test "print char1" "= 'abcdef'" "print char1, default values"
> +gdb_test "print char2" "= 'ghijkl'" "print char2, default values"
> +
> +gdb_breakpoint [gdb_get_line_number "! BP_BEFORE_SUB"]
> +gdb_continue_to_breakpoint "! BP_BEFORE_SUB" ".*! BP_BEFORE_SUB.*"
> +gdb_test "print doub1" "= 11\.11\\d+" "print doub1, before sub"
> +gdb_test "print doub2" "= 22\.22\\d+" "print doub2, before sub"
> +gdb_test "print char1" "= 'ABCDEF'" "print char1, before sub"
> +gdb_test "print char2" "= 'GHIJKL'" "print char2, before sub"
> +
> +gdb_breakpoint [gdb_get_line_number "! BP_SUB"]
> +gdb_continue_to_breakpoint "! BP_SUB" ".*! BP_SUB.*"
> +gdb_test "print doub1" "= 11\.11\\d+" "print char1, in sub"
                                                ^^^^^

I checked for duplicate test names, and that revealed the typo
above.  Should be "doub1" instead of "char1".

But better is to use with_test_prefix instead.

> +gdb_test "print doub2" "= 22\.22\\d+" "print doub2, in sub"
> +gdb_test "print char1" "= 'ABCDEF'" "print char1, in sub"
> +gdb_test "print char2" "= 'GHIJKL'" "print char2, in sub"


> +!
> +! Test if GDB can handle block data without global name

Missing period.

I've now applied the patch, with the issues above fixed, along
with a few other editorial tweaks to commit log and comments.

Note that I've extended the intro comment in the .exp file.

Here's what I merged.

>From a5fd13a91534b1c79a4b61995894a5bb4f08d3b0 Mon Sep 17 00:00:00 2001
From: Bernhard Heckel <bernhard.heckel@intel.com>
Date: Wed, 27 Feb 2019 15:21:12 +0100
Subject: [PATCH] Dwarf: Don't add nameless modules to partial symbol table

A name for BLOCK DATA in Fortran is optional.  If no name has been
assigned, GDB crashes during read-in of DWARF when BLOCK DATA is
represented via DW_TAG_module.  BLOCK DATA is used for one-time
initialization of non-pointer variables in named common blocks.

As of now there is no issue when gfortran is used as DW_TAG_module is
not emitted.  However, with Intel ifort the nameless DW_TAG_module is
present and has the following form:

 ...
  <1><dd>: Abbrev Number: 7 (DW_TAG_module)
     <de>   DW_AT_decl_line   : 46
     <df>   DW_AT_decl_file   : 1
     <e0>   DW_AT_description : (indirect string, offset: 0x110): block
 data
     <e4>   DW_AT_high_pc     : 0x402bb7
     <ec>   DW_AT_low_pc      : 0x402bb7
 ...

The missing name leads to a crash in add_partial_symbol, during length
calculation.

gdb/ChangeLog:
2019-06-11  Bernhard Heckel  <bernhard.heckel@intel.com>

	* dwarf2read.c (add_partial_symbol): Skip nameless modules.

gdb/testsuite/Changelog:
2019-06-11  Bernhard Heckel  <bernhard.heckel@intel.com>

	* gdb.fortran/block-data.f: New.
	* gdb.fortran/block-data.exp: New.
---
 gdb/dwarf2read.c                         | 17 +++++----
 gdb/testsuite/gdb.fortran/block-data.exp | 63 ++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.fortran/block-data.f   | 56 ++++++++++++++++++++++++++++
 3 files changed, 128 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.fortran/block-data.exp
 create mode 100644 gdb/testsuite/gdb.fortran/block-data.f

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 06c319dac02..4cf9fcfa218 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -9018,11 +9018,15 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
 			   0, cu->language, objfile);
       break;
     case DW_TAG_module:
-      add_psymbol_to_list (actual_name, strlen (actual_name),
-			   built_actual_name != NULL,
-			   MODULE_DOMAIN, LOC_TYPEDEF, -1,
-			   psymbol_placement::GLOBAL,
-			   0, cu->language, objfile);
+      /* With Fortran 77 there might be a "BLOCK DATA" module
+         available without any name.  If so, we skip the module as it
+         doesn't bring any value.  */
+      if (actual_name != nullptr)
+	add_psymbol_to_list (actual_name, strlen (actual_name),
+			     built_actual_name != NULL,
+			     MODULE_DOMAIN, LOC_TYPEDEF, -1,
+			     psymbol_placement::GLOBAL,
+			     0, cu->language, objfile);
       break;
     case DW_TAG_class_type:
     case DW_TAG_interface_type:
@@ -16955,9 +16959,6 @@ read_module_type (struct die_info *die, struct dwarf2_cu *cu)
   struct type *type;
 
   module_name = dwarf2_name (die, cu);
-  if (!module_name)
-    complaint (_("DW_TAG_module has no name, offset %s"),
-               sect_offset_str (die->sect_off));
   type = init_type (objfile, TYPE_CODE_MODULE, 0, module_name);
 
   return set_die_type (die, type, cu);
diff --git a/gdb/testsuite/gdb.fortran/block-data.exp b/gdb/testsuite/gdb.fortran/block-data.exp
new file mode 100644
index 00000000000..8da8c8824c8
--- /dev/null
+++ b/gdb/testsuite/gdb.fortran/block-data.exp
@@ -0,0 +1,63 @@
+# Copyright 2016-2019 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 anonymous block-data statements.
+
+# A name for BLOCK DATA in Fortran is optional.  BLOCK DATA is used
+# for one-time initialization of non-pointer variables in named common
+# blocks.  GDB used to crash with 'Intel ifort'-generated code, which
+# outputs nameless DW_TAG_module, unlike with gfortran which just
+# doesn't emit DW_TAG_module in this case.
+
+if { [skip_fortran_tests] } { return -1 }
+
+standard_testfile .f
+load_lib "fortran.exp"
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug f90}]} {
+    return -1
+}
+
+if ![runto MAIN__] then {
+    untested "couldn't run to breakpoint MAIN__"
+    return -1
+}
+
+with_test_prefix "default values" {
+    gdb_test "print doub1" "= 1\.11\\d+"
+    gdb_test "print doub2" "= 2\.22\\d+"
+    gdb_test "print char1" "= 'abcdef'"
+    gdb_test "print char2" "= 'ghijkl'"
+}
+
+gdb_breakpoint [gdb_get_line_number "! BP_BEFORE_SUB"]
+gdb_continue_to_breakpoint "! BP_BEFORE_SUB" ".*! BP_BEFORE_SUB.*"
+
+with_test_prefix "before sub" {
+    gdb_test "print doub1" "= 11\.11\\d+"
+    gdb_test "print doub2" "= 22\.22\\d+"
+    gdb_test "print char1" "= 'ABCDEF'"
+    gdb_test "print char2" "= 'GHIJKL'"
+}
+
+gdb_breakpoint [gdb_get_line_number "! BP_SUB"]
+gdb_continue_to_breakpoint "! BP_SUB" ".*! BP_SUB.*"
+
+with_test_prefix "in sub" {
+    gdb_test "print doub1" "= 11\.11\\d+"
+    gdb_test "print doub2" "= 22\.22\\d+"
+    gdb_test "print char1" "= 'ABCDEF'"
+    gdb_test "print char2" "= 'GHIJKL'"
+}
diff --git a/gdb/testsuite/gdb.fortran/block-data.f b/gdb/testsuite/gdb.fortran/block-data.f
new file mode 100644
index 00000000000..acb2c5a8591
--- /dev/null
+++ b/gdb/testsuite/gdb.fortran/block-data.f
@@ -0,0 +1,56 @@
+! Copyright 2016-2019 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/>.
+!
+! Check that GDB can handle block data with no global name.
+!
+! MAIN
+        PROGRAM bdata
+        DOUBLE PRECISION doub1, doub2
+        CHARACTER*6 char1, char2
+
+        COMMON /BLK1/ doub1, char1
+        COMMON /BLK2/ doub2, char2
+
+        doub1 = 11.111
+        doub2 = 22.222
+        char1 = 'ABCDEF'
+        char2 = 'GHIJKL'
+        CALL sub_block_data      ! BP_BEFORE_SUB
+        STOP
+        END
+
+! BLOCK DATA
+        BLOCK DATA
+
+        DOUBLE PRECISION doub1, doub2
+        CHARACTER*6 char1, char2
+
+        COMMON /BLK1/ doub1, char1
+        COMMON /BLK2/ doub2, char2
+        DATA doub1, doub2 /1.111, 2.222/
+        DATA char1, char2 /'abcdef', 'ghijkl'/
+        END
+
+! SUBROUTINE
+        SUBROUTINE sub_block_data
+
+        DOUBLE PRECISION doub1, doub2
+        CHARACTER*6 char1, char2
+
+        COMMON /BLK1/ doub1, char1
+        COMMON /BLK2/ doub2, char2
+
+        char1 = char2;    ! BP_SUB
+        END
-- 
2.14.5