[debug-early] equate new DIE with DW_AT_specificationto a previous declaration

[debug-early] equate new DIE with DW_AT_specificationto a previous declaration

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]

Hi Jason.

I merged mainline into the debug-early branch and I ran into a problem 
compiling a libstdc++ file with -fno-implicit-templates.  The attached 
patch is what I used to solve the problem but I wanted to run it by you, 
to make sure I'm not overlooking something silly, or worse...something 
much more complicated.

The reduced testcase is the following, compiled with 
-fno-implicit-templates -g -O2 -std=gnu++11:

class Object
{
public:
   void Method();
};

void funky()
{
   Object<int> foobar;
   foobar.Method();
}

template<typename SomeType>
void
Object<SomeType>::Method()
{
}

In mainline, we call gen_subprogram_die() twice for 
Object<int>::Method(): once, while generating class members, and once 
while inlining (outlining_inline_function hook).  The debug-early path 
is somewhat different, and we end up calling gen_subprogram_die() three 
times, the last of which ICEs.  What happens is the following...

We call gen_subprogram_die() as usual while generating class members, 
but then we call it again by virtue of it being a reachable function. 
This extra call will follow the DW_AT_specification code path because we 
have a previously cached die:

	  subr_die = new_die (DW_TAG_subprogram, context_die, decl);
	  add_AT_specification (subr_die, old_die);
           add_pubname (decl, subr_die);

The problem is that, for -fno-implicit-templates, the decl is now 
DECL_EXTERNAL, which means we never equate this new "DIE with 
DW_AT_specification" to the DECL.  That is, we never fall through here:

   else if (!DECL_EXTERNAL (decl))
     {
       HOST_WIDE_INT cfa_fb_offset;

       struct function *fun = DECL_STRUCT_FUNCTION (decl);

       if (!old_die || !get_AT (old_die, DW_AT_inline))
	equate_decl_number_to_die (decl, subr_die);

However, when we call gen_subprogram_die() the third time through the 
outlining_inline_function hook (late debug), we again try to add a 
DW_AT_specification to the DIE cached from the first time around, but 
this time we ICE because we're not supposed to have multiple 
DW_AT_specification's pointing to the same DIE (the old original DIE).

My solution is just to call equate_decl_number_to_die() as soon as we 
create the DW_AT_specification marked DIE the second time around.  The 
third time we will just pick up this last cached DIE with 
DW_AT_specification, mark it as DW_AT_inline, and voila, everything 
works.  The dwarf generation is as mainline, and we can build libstdc++ 
with no regressions to the guality testsuite.

Does this sound reasonable, or is this something a lot more complicated?

Thanks.
Aldy

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 670 bytes --]

commit 0a49042b9151e0387efc5f87c32cb24968896ae4
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Mar 17 12:29:27 2015 -0700

    Equate new DIE containing a DW_AT_specification, to the original
    declaration.

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 86815be..c7345d9 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -18809,6 +18809,8 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
 		add_type_attribute (subr_die, TREE_TYPE (TREE_TYPE (decl)),
 				    TYPE_UNQUALIFIED, context_die);
 	    }
+	  if (early_dwarf_dumping)
+	    equate_decl_number_to_die (decl, subr_die);
 	}
     }
   /* Anything else... create a brand new DIE.  */

Re: [debug-early] equate new DIE with DW_AT_specificationto a previous declaration

[Jason Merrill]

On 03/17/2015 03:58 PM, Aldy Hernandez wrote:
> The problem is that, for -fno-implicit-templates, the decl is now
> DECL_EXTERNAL, which means we never equate this new "DIE with
> DW_AT_specification" to the DECL.  That is, we never fall through here:
>
>    else if (!DECL_EXTERNAL (decl))
>      {
>        HOST_WIDE_INT cfa_fb_offset;
>
>        struct function *fun = DECL_STRUCT_FUNCTION (decl);
>
>        if (!old_die || !get_AT (old_die, DW_AT_inline))
>      equate_decl_number_to_die (decl, subr_die);
>
> However, when we call gen_subprogram_die() the third time through the
> outlining_inline_function hook (late debug), we again try to add a
> DW_AT_specification to the DIE cached from the first time around, but
> this time we ICE because we're not supposed to have multiple
> DW_AT_specification's pointing to the same DIE (the old original DIE).

Why are we outlining a DECL_EXTERNAL function?

Incidentally,

>           /* If we have no location information, this must be a
>              partially generated DIE from early dwarf generation.
>              Fall through and generate it.  */

Why aren't we checking dumped_early here?

Jason

Re: [debug-early] equate new DIE with DW_AT_specificationto a previous declaration

[Aldy Hernandez]

On 03/17/2015 07:12 PM, Jason Merrill wrote:
> On 03/17/2015 03:58 PM, Aldy Hernandez wrote:
>> The problem is that, for -fno-implicit-templates, the decl is now
>> DECL_EXTERNAL, which means we never equate this new "DIE with
>> DW_AT_specification" to the DECL.  That is, we never fall through here:
>>
>>    else if (!DECL_EXTERNAL (decl))
>>      {
>>        HOST_WIDE_INT cfa_fb_offset;
>>
>>        struct function *fun = DECL_STRUCT_FUNCTION (decl);
>>
>>        if (!old_die || !get_AT (old_die, DW_AT_inline))
>>      equate_decl_number_to_die (decl, subr_die);
>>
>> However, when we call gen_subprogram_die() the third time through the
>> outlining_inline_function hook (late debug), we again try to add a
>> DW_AT_specification to the DIE cached from the first time around, but
>> this time we ICE because we're not supposed to have multiple
>> DW_AT_specification's pointing to the same DIE (the old original DIE).
>
> Why are we outlining a DECL_EXTERNAL function?

SRA is analyzing Object<int>::Method() and noticing that `this' is never 
used, so it's trying to rewrite the call to avoid passing `this' (by 
creating a clone).

SRA has no restrictions on whether a function is DECL_EXTERNAL.  For 
that matter, the SRA pass is called on all functions that have a gimple 
body, irregardless of DECL_EXTERNAL, courtesy of the pass manager:

           if (node->has_gimple_body_p ())
             callback (DECL_STRUCT_FUNCTION (node->decl), data);

...and since Object<int>::Method() has a gimple body even though it is 
marked DECL_EXTERNAL...we get the call into dwarf2out_abstract_decl.

>
> Incidentally,
>
>>           /* If we have no location information, this must be a
>>              partially generated DIE from early dwarf generation.
>>              Fall through and generate it.  */
>
> Why aren't we checking dumped_early here?

Good point.  I'll add an assert.

Aldy

Re: [debug-early] equate new DIE with DW_AT_specificationto a previous declaration

[Jason Merrill]

On 03/18/2015 11:51 AM, Aldy Hernandez wrote:
> On 03/17/2015 07:12 PM, Jason Merrill wrote:
>> Why are we outlining a DECL_EXTERNAL function?
>
> SRA has no restrictions on whether a function is DECL_EXTERNAL.

Aha.

So it seems that DECL_EXTERNAL is the wrong gate for 
equate_decl_number_to_die here.  I think the rule we want is that if we 
don't already have a non-declaration DIE for a function, we should 
equate the new DIE.  Let's remove the existing calls and replace them 
with a single conditional call before the if (declaration).

Incidentally,

>       /* A declaration that has been previously dumped needs no
>          additional information.  */
>       if (dumped_early && declaration)
>         return;

Do we need to check dumped_early here?  I would think that any 
declaration that has an old_die needs no additional information.

Jason

Re: [debug-early] equate new DIE with DW_AT_specificationto a previous declaration

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

On 04/03/2015 07:41 AM, Jason Merrill wrote:
> On 03/18/2015 11:51 AM, Aldy Hernandez wrote:
>> On 03/17/2015 07:12 PM, Jason Merrill wrote:
>>> Why are we outlining a DECL_EXTERNAL function?
>>
>> SRA has no restrictions on whether a function is DECL_EXTERNAL.
>
> Aha.
>
> So it seems that DECL_EXTERNAL is the wrong gate for
> equate_decl_number_to_die here.  I think the rule we want is that if we
> don't already have a non-declaration DIE for a function, we should
> equate the new DIE.  Let's remove the existing calls and replace them
> with a single conditional call before the if (declaration).

Nice.  Done.

> Incidentally,
>
>>       /* A declaration that has been previously dumped needs no
>>          additional information.  */
>>       if (dumped_early && declaration)
>>         return;
>
> Do we need to check dumped_early here?  I would think that any
> declaration that has an old_die needs no additional information.

Indeed, good catch.  Fixed.

The attached patch fixes the original regression, and introduces none in 
either gdb or the guality tests on gcc.

OK for branch?

Aldy

[-- Attachment #2: curr --]
[-- Type: text/plain, Size: 3584 bytes --]

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 0976415..d7c367e 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -18743,13 +18743,13 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
      much as possible.  */
   else if (old_die)
     {
-      dumped_early = old_die->dumped_early;
-
       /* A declaration that has been previously dumped needs no
 	 additional information.  */
-      if (dumped_early && declaration)
+      if (declaration)
 	return;
 
+      dumped_early = old_die->dumped_early;
+
       if (!get_AT_flag (old_die, DW_AT_declaration)
 	  /* We can have a normal definition following an inline one in the
 	     case of redefinition of GNU C extern inlines.
@@ -18829,8 +18829,6 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
 		add_type_attribute (subr_die, TREE_TYPE (TREE_TYPE (decl)),
 				    TYPE_UNQUALIFIED, context_die);
 	    }
-	  if (early_dwarf_dumping)
-	    equate_decl_number_to_die (decl, subr_die);
 	}
     }
   /* Anything else... create a brand new DIE.  */
@@ -18860,6 +18858,11 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
       add_accessibility_attribute (subr_die, decl);
     }
 
+  /* Unless we have an existing non-declaration DIE, eqaute the new
+     DIE.  */
+  if (!old_die || is_declaration_die (old_die))
+    equate_decl_number_to_die (decl, subr_die);
+
   if (declaration)
     {
       if (!old_die || !get_AT (old_die, DW_AT_inline))
@@ -18877,15 +18880,6 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
 	  if (lang_hooks.decls.function_decl_deleted_p (decl)
 	      && (! dwarf_strict))
 	    add_AT_flag (subr_die, DW_AT_GNU_deleted, 1);
-
-	  /* The first time we see a member function, it is in the context of
-	     the class to which it belongs.  We make sure of this by emitting
-	     the class first.  The next time is the definition, which is
-	     handled above.  The two may come from the same source text.
-
-	     Note that force_decl_die() forces function declaration die. It is
-	     later reused to represent definition.  */
-	  equate_decl_number_to_die (decl, subr_die);
 	}
     }
   /* Tag abstract instances with DW_AT_inline.  */
@@ -18909,8 +18903,6 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
       if (DECL_DECLARED_INLINE_P (decl)
 	  && lookup_attribute ("artificial", DECL_ATTRIBUTES (decl)))
 	add_AT_flag (subr_die, DW_AT_artificial, 1);
-
-      equate_decl_number_to_die (decl, subr_die);
     }
   /* For non DECL_EXTERNALs, if range information is available, fill
      the DIE with it.  */
@@ -18920,9 +18912,6 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
 
       struct function *fun = DECL_STRUCT_FUNCTION (decl);
 
-      if (!old_die || !get_AT (old_die, DW_AT_inline))
-	equate_decl_number_to_die (decl, subr_die);
-
       /* If we have no fun->fde, we have no range information.
 	 Skip over and fill in range information in the second
 	 dwarf pass.  */
@@ -19090,14 +19079,9 @@ gen_subprogram_die (tree decl, dw_die_ref context_die)
     }
 
   /* Generate child dies for template paramaters.  */
-  if (debug_info_level > DINFO_LEVEL_TERSE)
-    {
-      /* XXX */
-      if (!lookup_decl_die (decl))
-	equate_decl_number_to_die (decl, subr_die);
-      if (early_dwarf_dumping)
-	gen_generic_params_dies (decl);
-    }
+  if (debug_info_level > DINFO_LEVEL_TERSE
+      && early_dwarf_dumping)
+    gen_generic_params_dies (decl);
 
   /* Now output descriptions of the arguments for this function. This gets
      (unnecessarily?) complex because of the fact that the DECL_ARGUMENT list