Fix PR debug/82509

Message ID 14641386.x2jigZTk3C@polaris
State New
Headers show
Series
  • Fix PR debug/82509
Related show

Commit Message

Eric Botcazou Oct. 12, 2017, 8:51 p.m.
Hi,

this PR reports a couple of problems with the support of the DW_AT_endianity 
attribute associated with the scalar_storage_order source attribute: it does 
not persist through typedefs and it can contaminate native order DIEs.

The attached patch revamps it by associating native order DIEs and reverse 
order DIEs into adjacent pairs for base types, as well as looking through 
typedefs for base types with reverse order.  This makes it possible to have a 
single reverse order DIE for each base type and look it up efficiently.

Tested on x86_64-suse-linux, OK for the mainline?  What about the 7 branch?


2017-10-12  Eric Botcazou  <ebotcazou@adacore.com>

	PR debug/82509
	* dwarf2out.c (base_type_die): Remove early return for corner cases.
	Allocate the new DIE manually and do not call add_pubtype on it.
	(is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
	(modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
	typedefs for base types with DW_AT_endianity.  Make sure a DIE with
	native order exists for base types, attach the DIE manually and call
	add_pubtype on it.  Do not equate a reverse order DIE to the type.


2017-10-12  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/debug/dwarf2/sso.c: Rename into...
	* gcc.dg/debug/dwarf2/sso-1.c: ...this.
	* gcc.dg/debug/dwarf2/sso-2.c: New test.
	* gcc.dg/debug/dwarf2/sso-3.c: Likewise.

Comments

Richard Biener Oct. 18, 2017, 11:17 a.m. | #1
On Thu, Oct 12, 2017 at 10:51 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this PR reports a couple of problems with the support of the DW_AT_endianity
> attribute associated with the scalar_storage_order source attribute: it does
> not persist through typedefs and it can contaminate native order DIEs.
>
> The attached patch revamps it by associating native order DIEs and reverse
> order DIEs into adjacent pairs for base types, as well as looking through
> typedefs for base types with reverse order.  This makes it possible to have a
> single reverse order DIE for each base type and look it up efficiently.
>
> Tested on x86_64-suse-linux, OK for the mainline?  What about the 7 branch?

Hmm.  It makes tracking DIE builds difficult now that not all allocations go
through new_die anymore.  Can you instead split out a new_die_raw
function with just the allocation and the die_tag initialization?  Or make
!parent_die && !t this mode, thus add

  if (parent_die != NULL)
    add_child_die (parent_die, die);
  else if (! t)
    return die;
  else
     {

?  Otherwise the patch looks sensible.

Thanks,
Richard.

>
> 2017-10-12  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR debug/82509
>         * dwarf2out.c (base_type_die): Remove early return for corner cases.
>         Allocate the new DIE manually and do not call add_pubtype on it.
>         (is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
>         (modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
>         typedefs for base types with DW_AT_endianity.  Make sure a DIE with
>         native order exists for base types, attach the DIE manually and call
>         add_pubtype on it.  Do not equate a reverse order DIE to the type.
>
>
> 2017-10-12  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gcc.dg/debug/dwarf2/sso.c: Rename into...
>         * gcc.dg/debug/dwarf2/sso-1.c: ...this.
>         * gcc.dg/debug/dwarf2/sso-2.c: New test.
>         * gcc.dg/debug/dwarf2/sso-3.c: Likewise.
>
> --
> Eric Botcazou
Eric Botcazou Oct. 18, 2017, 1:54 p.m. | #2
> Hmm.  It makes tracking DIE builds difficult now that not all allocations go
> through new_die anymore.

I wouldn't have created such a precedent though, IOW there is nothing new.

> Can you instead split out a new_die_raw
> function with just the allocation and the die_tag initialization?  Or make
> !parent_die && !t this mode, thus add
> 
>   if (parent_die != NULL)
>     add_child_die (parent_die, die);
>   else if (! t)
>     return die;
>   else
>      {
> 
> ?  Otherwise the patch looks sensible.

Here's a revision version which makes sure that there is a single call to

  ggc_cleared_alloc<die_node> ()

in the entire file.  Tested on x86_64-suse-linux.


        PR debug/82509
        * dwarf2out.c (new_die_raw): New static inline function.
	(new_die): Use it to create the DIE.
	(add_AT_external_die_ref): Likewise.
	(clone_die): Likewise.
	(clone_as_declaration): Likewise.
	(dwarf2out_vms_debug_main_pointer): Likewise.
	(base_type_die): Likewise.  Remove early return for corner cases.
        Do not call add_pubtype on the DIE here.
        (is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
        (modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
        typedefs for base types with DW_AT_endianity.  Make sure a DIE with
        native order exists for base types, attach the DIE manually and call
        add_pubtype on it.  Do not equate a reverse order DIE to the type.
Richard Biener Oct. 19, 2017, 8:11 a.m. | #3
On Wed, Oct 18, 2017 at 3:54 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Hmm.  It makes tracking DIE builds difficult now that not all allocations go
>> through new_die anymore.
>
> I wouldn't have created such a precedent though, IOW there is nothing new.

Ah, didn't notice the other existing cases.

>> Can you instead split out a new_die_raw
>> function with just the allocation and the die_tag initialization?  Or make
>> !parent_die && !t this mode, thus add
>>
>>   if (parent_die != NULL)
>>     add_child_die (parent_die, die);
>>   else if (! t)
>>     return die;
>>   else
>>      {
>>
>> ?  Otherwise the patch looks sensible.
>
> Here's a revision version which makes sure that there is a single call to
>
>   ggc_cleared_alloc<die_node> ()
>
> in the entire file.  Tested on x86_64-suse-linux.

Thanks, this is ok.

Richard.

>
>         PR debug/82509
>         * dwarf2out.c (new_die_raw): New static inline function.
>         (new_die): Use it to create the DIE.
>         (add_AT_external_die_ref): Likewise.
>         (clone_die): Likewise.
>         (clone_as_declaration): Likewise.
>         (dwarf2out_vms_debug_main_pointer): Likewise.
>         (base_type_die): Likewise.  Remove early return for corner cases.
>         Do not call add_pubtype on the DIE here.
>         (is_base_type): Remove ERROR_MARK and return 0 for VOID_TYPE.
>         (modified_type_die): Adjust the lookup for reverse order DIEs.  Skip
>         typedefs for base types with DW_AT_endianity.  Make sure a DIE with
>         native order exists for base types, attach the DIE manually and call
>         add_pubtype on it.  Do not equate a reverse order DIE to the type.
>
> --
> Eric Botcazou

Patch

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 253628)
+++ dwarf2out.c	(working copy)
@@ -12090,9 +12090,6 @@  base_type_die (tree type, bool reverse)
   struct fixed_point_type_info fpt_info;
   tree type_bias = NULL_TREE;
 
-  if (TREE_CODE (type) == ERROR_MARK || TREE_CODE (type) == VOID_TYPE)
-    return 0;
-
   /* If this is a subtype that should not be emitted as a subrange type,
      use the base type.  See subrange_type_for_debug_p.  */
   if (TREE_CODE (type) == INTEGER_TYPE && TREE_TYPE (type) != NULL_TREE)
@@ -12185,7 +12182,8 @@  base_type_die (tree type, bool reverse)
       gcc_unreachable ();
     }
 
-  base_type_result = new_die (DW_TAG_base_type, comp_unit_die (), type);
+  base_type_result = ggc_cleared_alloc<die_node> ();
+  base_type_result->die_tag = DW_TAG_base_type;
 
   add_AT_unsigned (base_type_result, DW_AT_byte_size,
 		   int_size_in_bytes (type));
@@ -12241,8 +12239,6 @@  base_type_die (tree type, bool reverse)
 		     | dw_scalar_form_reference,
 		     NULL);
 
-  add_pubtype (type, base_type_result);
-
   return base_type_result;
 }
 
@@ -12270,8 +12266,6 @@  is_base_type (tree type)
 {
   switch (TREE_CODE (type))
     {
-    case ERROR_MARK:
-    case VOID_TYPE:
     case INTEGER_TYPE:
     case REAL_TYPE:
     case FIXED_POINT_TYPE:
@@ -12280,6 +12274,7 @@  is_base_type (tree type)
     case POINTER_BOUNDS_TYPE:
       return 1;
 
+    case VOID_TYPE:
     case ARRAY_TYPE:
     case RECORD_TYPE:
     case UNION_TYPE:
@@ -12485,6 +12480,8 @@  modified_type_die (tree type, int cv_qua
   /* Only these cv-qualifiers are currently handled.  */
   const int cv_qual_mask = (TYPE_QUAL_CONST | TYPE_QUAL_VOLATILE
 			    | TYPE_QUAL_RESTRICT | TYPE_QUAL_ATOMIC);
+  const bool reverse_base_type
+    = need_endianity_attribute_p (reverse) && is_base_type (type);
 
   if (code == ERROR_MARK)
     return NULL;
@@ -12535,29 +12532,33 @@  modified_type_die (tree type, int cv_qua
 	qualified_type = size_type_node;
     }
 
-
   /* If we do, then we can just use its DIE, if it exists.  */
   if (qualified_type)
     {
       mod_type_die = lookup_type_die (qualified_type);
 
-      /* DW_AT_endianity doesn't come from a qualifier on the type.  */
+      /* DW_AT_endianity doesn't come from a qualifier on the type, so it is
+	 dealt with specially: the DIE with the attribute, if it exists, is
+	 placed immediately after the regular DIE for the same base type.  */
       if (mod_type_die
-	  && (!need_endianity_attribute_p (reverse)
-	      || !is_base_type (type)
-	      || get_AT_unsigned (mod_type_die, DW_AT_endianity)))
+	  && (!reverse_base_type
+	      || ((mod_type_die = mod_type_die->die_sib) != NULL
+		  && get_AT_unsigned (mod_type_die, DW_AT_endianity))))
 	return mod_type_die;
     }
 
   name = qualified_type ? TYPE_NAME (qualified_type) : NULL;
 
   /* Handle C typedef types.  */
-  if (name && TREE_CODE (name) == TYPE_DECL && DECL_ORIGINAL_TYPE (name)
+  if (name
+      && TREE_CODE (name) == TYPE_DECL
+      && DECL_ORIGINAL_TYPE (name)
       && !DECL_ARTIFICIAL (name))
     {
       tree dtype = TREE_TYPE (name);
 
-      if (qualified_type == dtype)
+      /* Skip the typedef for base types with DW_AT_endianity, no big deal.  */
+      if (qualified_type == dtype && !reverse_base_type)
 	{
 	  tree origin = decl_ultimate_origin (name);
 
@@ -12729,7 +12730,21 @@  modified_type_die (tree type, int cv_qua
       item_type = TREE_TYPE (type);
     }
   else if (is_base_type (type))
-    mod_type_die = base_type_die (type, reverse);
+    {
+      mod_type_die = base_type_die (type, reverse);
+
+      /* The DIE with DW_AT_endianity is placed right after the naked DIE.  */
+      if (reverse_base_type)
+	{
+	  dw_die_ref after_die
+	    = modified_type_die (type, cv_quals, false, context_die);
+	  add_child_die_after (comp_unit_die (), mod_type_die, after_die);
+	}
+      else
+	add_child_die (comp_unit_die (), mod_type_die);
+
+      add_pubtype (type, mod_type_die);
+    }
   else
     {
       gen_type_die (type, context_die);
@@ -12791,7 +12806,7 @@  modified_type_die (tree type, int cv_qua
 			  name ? IDENTIFIER_POINTER (name) : "__unknown__");
     }
 
-  if (qualified_type)
+  if (qualified_type && !reverse_base_type)
     equate_type_number_to_die (qualified_type, mod_type_die);
 
   if (item_type)