Fix crash in DWARF indexer

Message ID 20250106204028.3587605-1-tromey@adacore.com
State New
Headers
Series Fix crash in DWARF indexer |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 fail Patch failed to apply

Commit Message

Tom Tromey Jan. 6, 2025, 8:40 p.m. UTC
  Iain pointed out a crash in the DWARF indexer when run on a certain D
program.  The DWARF in this case has a nameless enum class; this
causes an assertion failure.

This patch arranges to simply ignore such types.  The fact that an
enum class is nameless in this case appears to be a compiler bug.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32518
---
 gdb/dwarf2/read.c                          | 35 ++++++------
 gdb/testsuite/gdb.dwarf2/nameless-enum.exp | 62 ++++++++++++++++++++++
 2 files changed, 82 insertions(+), 15 deletions(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/nameless-enum.exp
  

Comments

Tom de Vries Jan. 7, 2025, 12:10 p.m. UTC | #1
On 1/6/25 21:40, Tom Tromey wrote:
> Iain pointed out a crash in the DWARF indexer when run on a certain D
> program.  The DWARF in this case has a nameless enum class; this
> causes an assertion failure.
> 
> This patch arranges to simply ignore such types.  The fact that an
> enum class is nameless in this case appears to be a compiler bug.
> 

Hi Tom,

thanks for fixing this.

I've:
- applied the patch,
- ran the test-case and verified that it failed, then
- rebuild and ran the test-case again, and
- verified that it passes.

Approved-By: Tom de Vries <tdevries@suse.de>

I've got two nits, one in read.c and one in nameless-enum.exp.

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32518
> ---
>   gdb/dwarf2/read.c                          | 35 ++++++------
>   gdb/testsuite/gdb.dwarf2/nameless-enum.exp | 62 ++++++++++++++++++++++
>   2 files changed, 82 insertions(+), 15 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.dwarf2/nameless-enum.exp
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index a008f0ee88b..034bba01ad6 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -16814,22 +16814,27 @@ cooked_indexer::index_dies (cutu_reader *reader,
>   		 whether we're reading an "enum class".  If so, we use
>   		 the enum itself as the parent, yielding names like
>   		 "enum_class::enumerator"; otherwise we inject the
> -		 names into our own parent scope.  */
> -	      {
> -		std::variant<const cooked_index_entry *,
> -			     parent_map::addr_type> recurse_parent;
> -		if (is_enum_class)
> -		  {
> -		    gdb_assert (this_entry != nullptr);
> -		    recurse_parent = this_entry;
> -		  }
> -		else if (defer != 0)
> -		  recurse_parent = defer;
> -		else
> -		  recurse_parent = this_parent_entry;
> +		 names into our own parent scope.
>   
> -		info_ptr = recurse (reader, info_ptr, recurse_parent, fully);
> -	      }
> +		 Some versions of gdc could emit an "enum class"
> +		 without a name, which is nonsensical.  These are
> +		 skipped.  */
> +	      if (!is_enum_class || this_entry != nullptr)
> +		{
> +		  std::variant<const cooked_index_entry *,
> +			       parent_map::addr_type> recurse_parent;
> +		  if (is_enum_class)
> +		    {
> +		      gdb_assert (this_entry != nullptr);
> +		      recurse_parent = this_entry;
> +		    }
> +		  else if (defer != 0)
> +		    recurse_parent = defer;
> +		  else
> +		    recurse_parent = this_parent_entry;
> +
> +		  info_ptr = recurse (reader, info_ptr, recurse_parent, fully);
> +		}
>   	      continue;
>   
>   	    case DW_TAG_module:

I think that indenting the code makes it harder to read.

Also, I think that the comment describing an exception is better added 
in an if-clause matching the exception condition, rather than appended 
to the earlier comment.

But these may be a personal preferences.

Anyway, FWIW I prefer something like this:
...
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a008f0ee88b..3392dfa1aa2 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16818,11 +16818,14 @@ cooked_indexer::index_dies (cutu_reader *reader,
  	      {
  		std::variant<const cooked_index_entry *,
  			     parent_map::addr_type> recurse_parent;
-		if (is_enum_class)
+		if (is_enum_class && this_entry == nullptr)
  		  {
-		    gdb_assert (this_entry != nullptr);
-		    recurse_parent = this_entry;
+		    /* Some versions of gdc could emit an "enum class"
+		       without a name, which is nonsensical.  Skip this
+		       DIE.  */
  		  }
+		else if (is_enum_class)
+		  recurse_parent = this_entry;
  		else if (defer != 0)
  		  recurse_parent = defer;
  		else
...
or:
...
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a008f0ee88b..b512ac84c5c 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16809,6 +16809,14 @@ cooked_indexer::index_dies (cutu_reader *reader,
  	      break;

  	    case DW_TAG_enumeration_type:
+	      if (is_enum_class && this_entry == nullptr)
+		{
+		  /* Some versions of gdc could emit an "enum class"
+		     without a name, which is nonsensical.  Skip this
+		     DIE.  */
+		  continue;
+		}
+
  	      /* We need to recurse even for an anonymous enumeration.
  		 Which scope we record as the parent scope depends on
  		 whether we're reading an "enum class".  If so, we use
@@ -16819,10 +16827,7 @@ cooked_indexer::index_dies (cutu_reader *reader,
  		std::variant<const cooked_index_entry *,
  			     parent_map::addr_type> recurse_parent;
  		if (is_enum_class)
-		  {
-		    gdb_assert (this_entry != nullptr);
-		    recurse_parent = this_entry;
-		  }
+		  recurse_parent = this_entry;
  		else if (defer != 0)
  		  recurse_parent = defer;
  		else
...

> diff --git a/gdb/testsuite/gdb.dwarf2/nameless-enum.exp b/gdb/testsuite/gdb.dwarf2/nameless-enum.exp
> new file mode 100644
> index 00000000000..c2eda994984
> --- /dev/null
> +++ b/gdb/testsuite/gdb.dwarf2/nameless-enum.exp
> @@ -0,0 +1,62 @@
> +# Copyright 2025 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 a nameless "enum class".  This is nonsensical but previously
> +# made gdb crash.
> +
> +load_lib dwarf.exp
> +require dwarf2_support
> +
> +standard_testfile main.c nameless-enum.S
> +

Using nameless-enum.S is not necessary, we can just use:
...
standard_testfile main.c .S
...

Thanks,
- Tom

> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcfile
> +
> +    cu {} {
> +	DW_TAG_compile_unit {
> +	    {DW_AT_language @DW_LANG_D}
> +	    {DW_AT_name	$srcfile}
> +	    {DW_AT_comp_dir /tmp}
> +	} {
> +	    declare_labels integer_label
> +
> +	    integer_label: DW_TAG_base_type {
> +		{DW_AT_byte_size 4 DW_FORM_sdata}
> +		{DW_AT_encoding	 @DW_ATE_signed}
> +		{DW_AT_name	 int}
> +	    }
> +
> +	    DW_TAG_enumeration_type {
> +		{DW_AT_type :$integer_label}
> +		{DW_AT_enum_class 1 DW_FORM_flag}
> +	    } {
> +		DW_TAG_enumerator {
> +		    {DW_AT_name VALUE}
> +		    {DW_AT_const_value 17 DW_FORM_sdata}
> +		}
> +	    }
> +	}
> +    }
> +}
> +
> +if {[prepare_for_testing "failed to prepare" ${testfile} \
> +	 [list $srcfile $asm_file] {nodebug}]} {
> +    return -1
> +}
> +
> +# The bug was a crash, so just do anything here to verify gdb is still
> +# alive.
> +gdb_test "print 23" " = 23"
  
Tom Tromey Jan. 7, 2025, 2:40 p.m. UTC | #2
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I think that indenting the code makes it harder to read.
...

>> +standard_testfile main.c nameless-enum.S

Tom> Using nameless-enum.S is not necessary, we can just use:
Tom> ...
Tom> standard_testfile main.c .S
Tom> ...

I made these changes.

Tom
  
Tom Tromey Jan. 7, 2025, 2:45 p.m. UTC | #3
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> Iain pointed out a crash in the DWARF indexer when run on a certain D
Tom> program.  The DWARF in this case has a nameless enum class; this
Tom> causes an assertion failure.

Tom> This patch arranges to simply ignore such types.  The fact that an
Tom> enum class is nameless in this case appears to be a compiler bug.

Tom> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32518

I'm also going to merge this one to gdb 16.
It may be a regression; and in any case the fix is very simple.

Tom
  

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a008f0ee88b..034bba01ad6 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16814,22 +16814,27 @@  cooked_indexer::index_dies (cutu_reader *reader,
 		 whether we're reading an "enum class".  If so, we use
 		 the enum itself as the parent, yielding names like
 		 "enum_class::enumerator"; otherwise we inject the
-		 names into our own parent scope.  */
-	      {
-		std::variant<const cooked_index_entry *,
-			     parent_map::addr_type> recurse_parent;
-		if (is_enum_class)
-		  {
-		    gdb_assert (this_entry != nullptr);
-		    recurse_parent = this_entry;
-		  }
-		else if (defer != 0)
-		  recurse_parent = defer;
-		else
-		  recurse_parent = this_parent_entry;
+		 names into our own parent scope.
 
-		info_ptr = recurse (reader, info_ptr, recurse_parent, fully);
-	      }
+		 Some versions of gdc could emit an "enum class"
+		 without a name, which is nonsensical.  These are
+		 skipped.  */
+	      if (!is_enum_class || this_entry != nullptr)
+		{
+		  std::variant<const cooked_index_entry *,
+			       parent_map::addr_type> recurse_parent;
+		  if (is_enum_class)
+		    {
+		      gdb_assert (this_entry != nullptr);
+		      recurse_parent = this_entry;
+		    }
+		  else if (defer != 0)
+		    recurse_parent = defer;
+		  else
+		    recurse_parent = this_parent_entry;
+
+		  info_ptr = recurse (reader, info_ptr, recurse_parent, fully);
+		}
 	      continue;
 
 	    case DW_TAG_module:
diff --git a/gdb/testsuite/gdb.dwarf2/nameless-enum.exp b/gdb/testsuite/gdb.dwarf2/nameless-enum.exp
new file mode 100644
index 00000000000..c2eda994984
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/nameless-enum.exp
@@ -0,0 +1,62 @@ 
+# Copyright 2025 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 a nameless "enum class".  This is nonsensical but previously
+# made gdb crash.
+
+load_lib dwarf.exp
+require dwarf2_support
+
+standard_testfile main.c nameless-enum.S
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcfile
+
+    cu {} {
+	DW_TAG_compile_unit {
+	    {DW_AT_language @DW_LANG_D}
+	    {DW_AT_name	$srcfile}
+	    {DW_AT_comp_dir /tmp}
+	} {
+	    declare_labels integer_label
+
+	    integer_label: DW_TAG_base_type {
+		{DW_AT_byte_size 4 DW_FORM_sdata}
+		{DW_AT_encoding	 @DW_ATE_signed}
+		{DW_AT_name	 int}
+	    }
+
+	    DW_TAG_enumeration_type {
+		{DW_AT_type :$integer_label}
+		{DW_AT_enum_class 1 DW_FORM_flag}
+	    } {
+		DW_TAG_enumerator {
+		    {DW_AT_name VALUE}
+		    {DW_AT_const_value 17 DW_FORM_sdata}
+		}
+	    }
+	}
+    }
+}
+
+if {[prepare_for_testing "failed to prepare" ${testfile} \
+	 [list $srcfile $asm_file] {nodebug}]} {
+    return -1
+}
+
+# The bug was a crash, so just do anything here to verify gdb is still
+# alive.
+gdb_test "print 23" " = 23"