Fix Ada crash with .debug_types

Message ID 20200428165854.5548-1-tromey@adacore.com
State New
Headers show
Series
  • Fix Ada crash with .debug_types
Related show

Commit Message

Tom Tromey April 28, 2020, 4:58 p.m.
PR ada/25875 concerns a gdb crash when gdb.ada/arr_enum_idx_w_gap.exp
is run using the .debug_types board.

The problem turns out to be caused by weird compiler output.  In this
test, the compiler emits a top-level type that refers to an
enumeration type which is nested in a function.  However, this
function is just a declaration.

This results in gdb calling read_enumeration_type for the enum type,
but process_enumeration_scope is never called, yielding an enum with
no fields.  This causes the crash.

This patch fixes the problem by arranging to create the enum fields in
read_enumeration_type.

Tested on x86-64 Fedora 30.

gdb/ChangeLog
2020-04-28  Tom Tromey  <tromey@adacore.com>

	PR ada/25875:
	* dwarf2/read.c (update_enumeration_type_from_children): Compute
	type fields here.
	(read_enumeration_type): Call
	update_enumeration_type_from_children later.  Update comments.
	(process_enumeration_scope): Don't create type fields.
---
 gdb/ChangeLog     |  9 +++++++
 gdb/dwarf2/read.c | 62 +++++++++++++++++++++--------------------------
 2 files changed, 37 insertions(+), 34 deletions(-)

-- 
2.21.1

Comments

Simon Marchi April 28, 2020, 5:51 p.m. | #1
On 2020-04-28 12:58 p.m., Tom Tromey wrote:
> @@ -15838,10 +15840,20 @@ update_enumeration_type_from_children (struct die_info *die,

>  	    flag_enum = 0;

>  	}

>  

> -      /* If we already know that the enum type is neither unsigned, nor

> -	 a flag type, no need to look at the rest of the enumerates.  */

> -      if (!unsigned_enum && !flag_enum)

> -	break;

> +      fields.emplace_back ();

> +      struct field &field = fields.back ();

> +      FIELD_NAME (field) = dwarf2_physname (name, child_die, cu);

> +      if (bytes == nullptr && baton == nullptr)

> +	SET_FIELD_ENUMVAL (field, value);


What would be the consequences if this `if` turned out to be false, and we didn't
se the enum field's value?  Does that leave it in a valid state?

The code above that already checks and uses `value`, so it already assumes that
`dwarf2_const_value_attr` returned something meaningful in there.  Therefore
I don't think this last check is really useful.

Simon
Tom Tromey April 28, 2020, 6:59 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


>> +      if (bytes == nullptr && baton == nullptr)

>> +	SET_FIELD_ENUMVAL (field, value);


Simon> What would be the consequences if this `if` turned out to be false, and we didn't
Simon> se the enum field's value?  Does that leave it in a valid state?

Hard to know.  I guess it could only happen if an enumerator value had a
block or expression form, which seems pretty unlikely.

I guess block form could happen for an enum with a 128-bit base type.
That's not handled at all by gdb though.

Simon> The code above that already checks and uses `value`, so it already assumes that
Simon> `dwarf2_const_value_attr` returned something meaningful in there.  Therefore
Simon> I don't think this last check is really useful.

I'll remove it.

Tom
Tom de Vries April 29, 2020, 11:54 a.m. | #3
On 28-04-2020 18:58, Tom Tromey wrote:
> PR ada/25875 concerns a gdb crash when gdb.ada/arr_enum_idx_w_gap.exp

> is run using the .debug_types board.

> 

> The problem turns out to be caused by weird compiler output.  In this

> test, the compiler emits a top-level type that refers to an

> enumeration type which is nested in a function.  However, this

> function is just a declaration.

> 

> This results in gdb calling read_enumeration_type for the enum type,

> but process_enumeration_scope is never called, yielding an enum with

> no fields.  This causes the crash.

> 

> This patch fixes the problem by arranging to create the enum fields in

> read_enumeration_type.

> 

> Tested on x86-64 Fedora 30.

> 


Hi,

I tested this with target board debug-types, and I can confirm that it
fixes all the ERRORs I reported in PR25875.

Thanks,
- Tom

> gdb/ChangeLog

> 2020-04-28  Tom Tromey  <tromey@adacore.com>

> 

> 	PR ada/25875:

> 	* dwarf2/read.c (update_enumeration_type_from_children): Compute

> 	type fields here.

> 	(read_enumeration_type): Call

> 	update_enumeration_type_from_children later.  Update comments.

> 	(process_enumeration_scope): Don't create type fields.

> ---

>  gdb/ChangeLog     |  9 +++++++

>  gdb/dwarf2/read.c | 62 +++++++++++++++++++++--------------------------

>  2 files changed, 37 insertions(+), 34 deletions(-)

> 

> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c

> index 130c20dbd82..dc841476d6b 100644

> --- a/gdb/dwarf2/read.c

> +++ b/gdb/dwarf2/read.c

> @@ -15790,8 +15790,9 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)

>      }

>  }

>  

> -/* Assuming DIE is an enumeration type, and TYPE is its associated type,

> -   update TYPE using some information only available in DIE's children.  */

> +/* Assuming DIE is an enumeration type, and TYPE is its associated

> +   type, update TYPE using some information only available in DIE's

> +   children.  In particular, the fields are computed.  */

>  

>  static void

>  update_enumeration_type_from_children (struct die_info *die,

> @@ -15803,6 +15804,7 @@ update_enumeration_type_from_children (struct die_info *die,

>    int flag_enum = 1;

>  

>    auto_obstack obstack;

> +  std::vector<struct field> fields;

>  

>    for (child_die = die->child;

>         child_die != NULL && child_die->tag;

> @@ -15838,10 +15840,20 @@ update_enumeration_type_from_children (struct die_info *die,

>  	    flag_enum = 0;

>  	}

>  

> -      /* If we already know that the enum type is neither unsigned, nor

> -	 a flag type, no need to look at the rest of the enumerates.  */

> -      if (!unsigned_enum && !flag_enum)

> -	break;

> +      fields.emplace_back ();

> +      struct field &field = fields.back ();

> +      FIELD_NAME (field) = dwarf2_physname (name, child_die, cu);

> +      if (bytes == nullptr && baton == nullptr)

> +	SET_FIELD_ENUMVAL (field, value);

> +    }

> +

> +  if (!fields.empty ())

> +    {

> +      TYPE_NFIELDS (type) = fields.size ();

> +      TYPE_FIELDS (type) = (struct field *)

> +	TYPE_ALLOC (type, sizeof (struct field) * fields.size ());

> +      memcpy (TYPE_FIELDS (type), fields.data (),

> +	      sizeof (struct field) * fields.size ());

>      }

>  

>    if (unsigned_enum)

> @@ -15909,11 +15921,6 @@ read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)

>    if (die_is_declaration (die, cu))

>      TYPE_STUB (type) = 1;

>  

> -  /* Finish the creation of this type by using the enum's children.

> -     We must call this even when the underlying type has been provided

> -     so that we can determine if we're looking at a "flag" enum.  */

> -  update_enumeration_type_from_children (die, type, cu);

> -

>    /* If this type has an underlying type that is not a stub, then we

>       may use its attributes.  We always use the "unsigned" attribute

>       in this situation, because ordinarily we guess whether the type

> @@ -15935,7 +15942,15 @@ read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)

>  

>    TYPE_DECLARED_CLASS (type) = dwarf2_flag_true_p (die, DW_AT_enum_class, cu);

>  

> -  return set_die_type (die, type, cu);

> +  set_die_type (die, type, cu);

> +

> +  /* Finish the creation of this type by using the enum's children.

> +     Note that, as usual, this must come after set_die_type to avoid

> +     infinite recursion when trying to compute the names of the

> +     enumerators.  */

> +  update_enumeration_type_from_children (die, type, cu);

> +

> +  return type;

>  }

>  

>  /* Given a pointer to a die which begins an enumeration, process all

> @@ -15956,8 +15971,6 @@ process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)

>    if (die->child != NULL)

>      {

>        struct die_info *child_die;

> -      struct symbol *sym;

> -      std::vector<struct field> fields;

>        const char *name;

>  

>        child_die = die->child;

> @@ -15971,30 +15984,11 @@ process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)

>  	    {

>  	      name = dwarf2_name (child_die, cu);

>  	      if (name)

> -		{

> -		  sym = new_symbol (child_die, this_type, cu);

> -

> -		  fields.emplace_back ();

> -		  struct field &field = fields.back ();

> -

> -		  FIELD_NAME (field) = sym->linkage_name ();

> -		  FIELD_TYPE (field) = NULL;

> -		  SET_FIELD_ENUMVAL (field, SYMBOL_VALUE (sym));

> -		  FIELD_BITSIZE (field) = 0;

> -		}

> +		new_symbol (child_die, this_type, cu);

>  	    }

>  

>  	  child_die = child_die->sibling;

>  	}

> -

> -      if (!fields.empty ())

> -	{

> -	  TYPE_NFIELDS (this_type) = fields.size ();

> -	  TYPE_FIELDS (this_type) = (struct field *)

> -	    TYPE_ALLOC (this_type, sizeof (struct field) * fields.size ());

> -	  memcpy (TYPE_FIELDS (this_type), fields.data (),

> -		  sizeof (struct field) * fields.size ());

> -	}

>      }

>  

>    /* If we are reading an enum from a .debug_types unit, and the enum

>
Tom Tromey April 29, 2020, 5:16 p.m. | #4
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:


Tom> I tested this with target board debug-types, and I can confirm that it
Tom> fixes all the ERRORs I reported in PR25875.

Thanks.  I'm going to check it in, then.

Tom

Patch

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 130c20dbd82..dc841476d6b 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -15790,8 +15790,9 @@  process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
 }
 
-/* Assuming DIE is an enumeration type, and TYPE is its associated type,
-   update TYPE using some information only available in DIE's children.  */
+/* Assuming DIE is an enumeration type, and TYPE is its associated
+   type, update TYPE using some information only available in DIE's
+   children.  In particular, the fields are computed.  */
 
 static void
 update_enumeration_type_from_children (struct die_info *die,
@@ -15803,6 +15804,7 @@  update_enumeration_type_from_children (struct die_info *die,
   int flag_enum = 1;
 
   auto_obstack obstack;
+  std::vector<struct field> fields;
 
   for (child_die = die->child;
        child_die != NULL && child_die->tag;
@@ -15838,10 +15840,20 @@  update_enumeration_type_from_children (struct die_info *die,
 	    flag_enum = 0;
 	}
 
-      /* If we already know that the enum type is neither unsigned, nor
-	 a flag type, no need to look at the rest of the enumerates.  */
-      if (!unsigned_enum && !flag_enum)
-	break;
+      fields.emplace_back ();
+      struct field &field = fields.back ();
+      FIELD_NAME (field) = dwarf2_physname (name, child_die, cu);
+      if (bytes == nullptr && baton == nullptr)
+	SET_FIELD_ENUMVAL (field, value);
+    }
+
+  if (!fields.empty ())
+    {
+      TYPE_NFIELDS (type) = fields.size ();
+      TYPE_FIELDS (type) = (struct field *)
+	TYPE_ALLOC (type, sizeof (struct field) * fields.size ());
+      memcpy (TYPE_FIELDS (type), fields.data (),
+	      sizeof (struct field) * fields.size ());
     }
 
   if (unsigned_enum)
@@ -15909,11 +15921,6 @@  read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)
   if (die_is_declaration (die, cu))
     TYPE_STUB (type) = 1;
 
-  /* Finish the creation of this type by using the enum's children.
-     We must call this even when the underlying type has been provided
-     so that we can determine if we're looking at a "flag" enum.  */
-  update_enumeration_type_from_children (die, type, cu);
-
   /* If this type has an underlying type that is not a stub, then we
      may use its attributes.  We always use the "unsigned" attribute
      in this situation, because ordinarily we guess whether the type
@@ -15935,7 +15942,15 @@  read_enumeration_type (struct die_info *die, struct dwarf2_cu *cu)
 
   TYPE_DECLARED_CLASS (type) = dwarf2_flag_true_p (die, DW_AT_enum_class, cu);
 
-  return set_die_type (die, type, cu);
+  set_die_type (die, type, cu);
+
+  /* Finish the creation of this type by using the enum's children.
+     Note that, as usual, this must come after set_die_type to avoid
+     infinite recursion when trying to compute the names of the
+     enumerators.  */
+  update_enumeration_type_from_children (die, type, cu);
+
+  return type;
 }
 
 /* Given a pointer to a die which begins an enumeration, process all
@@ -15956,8 +15971,6 @@  process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)
   if (die->child != NULL)
     {
       struct die_info *child_die;
-      struct symbol *sym;
-      std::vector<struct field> fields;
       const char *name;
 
       child_die = die->child;
@@ -15971,30 +15984,11 @@  process_enumeration_scope (struct die_info *die, struct dwarf2_cu *cu)
 	    {
 	      name = dwarf2_name (child_die, cu);
 	      if (name)
-		{
-		  sym = new_symbol (child_die, this_type, cu);
-
-		  fields.emplace_back ();
-		  struct field &field = fields.back ();
-
-		  FIELD_NAME (field) = sym->linkage_name ();
-		  FIELD_TYPE (field) = NULL;
-		  SET_FIELD_ENUMVAL (field, SYMBOL_VALUE (sym));
-		  FIELD_BITSIZE (field) = 0;
-		}
+		new_symbol (child_die, this_type, cu);
 	    }
 
 	  child_die = child_die->sibling;
 	}
-
-      if (!fields.empty ())
-	{
-	  TYPE_NFIELDS (this_type) = fields.size ();
-	  TYPE_FIELDS (this_type) = (struct field *)
-	    TYPE_ALLOC (this_type, sizeof (struct field) * fields.size ());
-	  memcpy (TYPE_FIELDS (this_type), fields.data (),
-		  sizeof (struct field) * fields.size ());
-	}
     }
 
   /* If we are reading an enum from a .debug_types unit, and the enum