[PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Richard Guenther]

This removes defering DECL_ASSEMBLER_NAME computation inside dwarf2out.c,
something which is not possible when LTO is enabled during compilation
as we free up tree fields after we think the frontend is finished - but
DECL_ASSEMBLER_NAME computation involves a langhook and frontend specific
data.  In theory we try hard to properly assign assembler names for
everything reachable in free_lang_data, but the frontend hands even
"unreachable" types to dwarf2out via rest_of_type_compilation
and debug_hooks->type_decl.

Another approach would be to only call add_linkage_attr if the
assembler name was set meanwhile by other means, thus

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c     (revision 188063)
+++ gcc/dwarf2out.c     (working copy)
@@ -22158,7 +22158,8 @@ dwarf2out_finish (const char *filename)
   for (node = deferred_asm_name; node; node = node->next)
     {
       tree decl = node->created_for;
-      if (DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
+      if (DECL_ASSEMBLER_NAME_SET_P (decl)
+         && DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
        {
          add_linkage_attr (node->die, decl);
          move_linkage_attr (node->die);

But I bootstrapped and tested the following variant instead
which generates the same code and debuginfo with -O0 -g for
tramp3d at the cost of a 0.01% compile-time hit (not sure,
but I suppose the deferral was for a reason?).

Ok?

Thanks,
Richard.

2012-05-31  Richard Guenther  <rguenther@suse.de>

	PR debug/53471
	* dwarf2out.c (deferred_asm_name): Remove.
	(add_linkage_name): Force DECL_ASSEMBLER_NAME creation here.
	(move_linkage_attr): Remove.
	(dwarf2out_finish): Do not process deferred_asm_name.

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 188056)
+++ gcc/dwarf2out.c	(working copy)
@@ -2652,10 +2652,6 @@ static GTY(()) comdat_type_node *comdat_
 /* A list of DIEs with a NULL parent waiting to be relocated.  */
 static GTY(()) limbo_die_node *limbo_die_list;
 
-/* A list of DIEs for which we may have to generate
-   DW_AT_{,MIPS_}linkage_name once their DECL_ASSEMBLER_NAMEs are set.  */
-static GTY(()) limbo_die_node *deferred_asm_name;
-
 /* Filenames referenced by this compilation unit.  */
 static GTY((param_is (struct dwarf_file_data))) htab_t file_table;
 
@@ -15394,22 +15390,9 @@ add_linkage_name (dw_die_ref die, tree d
        && TREE_PUBLIC (decl)
        && !DECL_ABSTRACT (decl)
        && !(TREE_CODE (decl) == VAR_DECL && DECL_REGISTER (decl))
-       && die->die_tag != DW_TAG_member)
-    {
-      /* Defer until we have an assembler name set.  */
-      if (!DECL_ASSEMBLER_NAME_SET_P (decl))
-	{
-	  limbo_die_node *asm_name;
-
-	  asm_name = ggc_alloc_cleared_limbo_die_node ();
-	  asm_name->die = die;
-	  asm_name->created_for = decl;
-	  asm_name->next = deferred_asm_name;
-	  deferred_asm_name = asm_name;
-	}
-      else if (DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
-	add_linkage_attr (die, decl);
-    }
+       && die->die_tag != DW_TAG_member
+       && DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
+    add_linkage_attr (die, decl);
 }
 
 /* Add a DW_AT_name attribute and source coordinate attribute for the
@@ -21161,37 +21144,6 @@ htab_ct_eq (const void *of1, const void
                     DWARF_TYPE_SIGNATURE_SIZE));
 }
 
-/* Move a DW_AT_{,MIPS_}linkage_name attribute just added to dw_die_ref
-   to the location it would have been added, should we know its
-   DECL_ASSEMBLER_NAME when we added other attributes.  This will
-   probably improve compactness of debug info, removing equivalent
-   abbrevs, and hide any differences caused by deferring the
-   computation of the assembler name, triggered by e.g. PCH.  */
-
-static inline void
-move_linkage_attr (dw_die_ref die)
-{
-  unsigned ix = VEC_length (dw_attr_node, die->die_attr);
-  dw_attr_node linkage = *VEC_index (dw_attr_node, die->die_attr, ix - 1);
-
-  gcc_assert (linkage.dw_attr == DW_AT_linkage_name
-	      || linkage.dw_attr == DW_AT_MIPS_linkage_name);
-
-  while (--ix > 0)
-    {
-      dw_attr_node *prev = VEC_index (dw_attr_node, die->die_attr, ix - 1);
-
-      if (prev->dw_attr == DW_AT_decl_line || prev->dw_attr == DW_AT_name)
-	break;
-    }
-
-  if (ix != VEC_length (dw_attr_node, die->die_attr) - 1)
-    {
-      VEC_pop (dw_attr_node, die->die_attr);
-      VEC_quick_insert (dw_attr_node, die->die_attr, ix, &linkage);
-    }
-}
-
 /* Helper function for resolve_addr, mark DW_TAG_base_type nodes
    referenced from typed stack ops and count how often they are used.  */
 
@@ -22155,18 +22107,6 @@ dwarf2out_finish (const char *filename)
   resolve_addr (comp_unit_die ());
   move_marked_base_types ();
 
-  for (node = deferred_asm_name; node; node = node->next)
-    {
-      tree decl = node->created_for;
-      if (DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
-	{
-	  add_linkage_attr (node->die, decl);
-	  move_linkage_attr (node->die);
-	}
-    }
-
-  deferred_asm_name = NULL;
-
   /* Walk through the list of incomplete types again, trying once more to
      emit full debugging info for them.  */
   retry_incomplete_types ();

Re: [PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Jason Merrill]

The comment mentions PCH in connection with deferred seting of 
DECL_ASSEMBLER_NAME; off the top of my head it occurs to me that that 
might be connected with anonymous unions, which need to have different 
linkage names in different translation units.

Jason

Re: [PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Richard Guenther]

On Thu, 31 May 2012, Jason Merrill wrote:

> The comment mentions PCH in connection with deferred seting of
> DECL_ASSEMBLER_NAME; off the top of my head it occurs to me that that might be
> connected with anonymous unions, which need to have different linkage names in
> different translation units.

Not sure when PCH generation happens (or when we call 
rest_of_type_compilation), but shouldn't that be way earlier than
the debug output?  Anyway, any suggestion on how to tackle the issue
that we cannot compute new DECL_ASSEMBLER_NAMEs after the frontend
is finished?

Thanks,
Richard.

Re: [PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Jason Merrill]

On 06/01/2012 05:22 AM, Richard Guenther wrote:
> On Thu, 31 May 2012, Jason Merrill wrote:
>
>> The comment mentions PCH in connection with deferred seting of
>> DECL_ASSEMBLER_NAME; off the top of my head it occurs to me that that might be
>> connected with anonymous unions, which need to have different linkage names in
>> different translation units.
>
> Not sure when PCH generation happens (or when we call
> rest_of_type_compilation), but shouldn't that be way earlier than
> the debug output?

PCH generation happens at the beginning of cp_write_global_declarations; 
rest_of_type_compilation, which triggers the debug output, is called 
after each class definition.

> Anyway, any suggestion on how to tackle the issue
> that we cannot compute new DECL_ASSEMBLER_NAMEs after the frontend
> is finished?

Fix up the deferred_asm_name list somewhere between the call to 
c_common_write_pch and the call to free_lang_data.  I guess this would 
mean another debug hook for processing that needs to happen before 
free_lang_data.

Or fix free_lang_data to deal with these types, too.

Or use your first patch, and decide that we don't care about the linkage 
name of unreachable types.  What types are affected by this, anyway?

Jason

Re: [PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Richard Guenther]

On Fri, 1 Jun 2012, Jason Merrill wrote:

> On 06/01/2012 05:22 AM, Richard Guenther wrote:
> > On Thu, 31 May 2012, Jason Merrill wrote:
> >
> > > The comment mentions PCH in connection with deferred seting of
> > > DECL_ASSEMBLER_NAME; off the top of my head it occurs to me that that
> > > might be
> > > connected with anonymous unions, which need to have different linkage
> > > names in
> > > different translation units.
> >
> > Not sure when PCH generation happens (or when we call
> > rest_of_type_compilation), but shouldn't that be way earlier than
> > the debug output?
> 
> PCH generation happens at the beginning of cp_write_global_declarations;
> rest_of_type_compilation, which triggers the debug output, is called after
> each class definition.
> 
> > Anyway, any suggestion on how to tackle the issue
> > that we cannot compute new DECL_ASSEMBLER_NAMEs after the frontend
> > is finished?
> 
> Fix up the deferred_asm_name list somewhere between the call to
> c_common_write_pch and the call to free_lang_data.  I guess this would mean
> another debug hook for processing that needs to happen before free_lang_data.

That's certainly possible - add a finish_compilation_unit debug hook.
I'll think about that for 4.8.

> Or fix free_lang_data to deal with these types, too.

Unfortunately they are not "reachable" from anything but the
deferred_asm_name list.  So it would mean another debug hook.

> Or use your first patch, and decide that we don't care about the linkage name
> of unreachable types.  What types are affected by this, anyway?

Types affected by this are types not referenced from any object/function
that is being output as LTO bytecode (and the type will not be output
as LTO bytecode either, so at LTO time we'd not generate debuginfo for
it either).  It's basically completely "unused" types that are affected.

I suppose that's indeed the easiest fix - all "important" types should
have been assigned an assembler name by free-lang-data.  One issue
is that free-lang-data (and its assembler name assigning code) is not
run if -flto is not enabled ...

So I suppose for the 4.7 branch we'd go with

Index: dwarf2out.c
===================================================================
--- dwarf2out.c (revision 188106)
+++ dwarf2out.c (working copy)
@@ -22158,7 +22158,8 @@ dwarf2out_finish (const char *filename)
   for (node = deferred_asm_name; node; node = node->next)
     {
       tree decl = node->created_for;
-      if (DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
+      if ((!flag_generate_lto || DECL_ASSEMBLER_NAME_SET_P (decl))
+         && DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
        {
          add_linkage_attr (node->die, decl);
          move_linkage_attr (node->die);

and for trunk change two things - make sure the assembler-name assigning
piece of free-lang-data runs unconditionally and a debug hook is added
so we can output debuginfo dependent on langhooks.

I am going to test the above and install it on trunk and the branch
for now to get into 4.7.1.

Thanks,
Richard.

Re: [PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Richard Guenther]

On Mon, 4 Jun 2012, Richard Guenther wrote:

> On Fri, 1 Jun 2012, Jason Merrill wrote:
> 
> > Or use your first patch, and decide that we don't care about the linkage name
> > of unreachable types.  What types are affected by this, anyway?

So like the following.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Thanks,
Richard.

2012-06-04  Richard Guenther  <rguenther@suse.de>

	PR middle-end/53471
	* dwarf2out.c (dwarf2out_finish): If generating LTO do not
	create new assembler names.

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	(revision 188168)
+++ gcc/dwarf2out.c	(working copy)
@@ -22158,7 +22158,8 @@ dwarf2out_finish (const char *filename)
   for (node = deferred_asm_name; node; node = node->next)
     {
       tree decl = node->created_for;
-      if (DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
+      if ((!flag_generate_lto || DECL_ASSEMBLER_NAME_SET_P (decl))
+	  && DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
 	{
 	  add_linkage_attr (node->die, decl);
 	  move_linkage_attr (node->die);

Re: [PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Jason Merrill]

Please add a comment explaining why flag_generate_lto matters there.  OK 
with that change.

Jason

Re: [PATCH] Fix PR53471, remove DECL_ASSEMBLER_NAME deferred compute

[Richard Guenther]

On Mon, 4 Jun 2012, Jason Merrill wrote:

> Please add a comment explaining why flag_generate_lto matters there.  OK with
> that change.

Committed as follows.

Thanks,
Richard.

2012-06-04  Richard Guenther  <rguenther@suse.de>

	PR middle-end/53471
	* dwarf2out.c (dwarf2out_finish): If generating LTO do not
	create new assembler names.

Index: gcc/dwarf2out.c
===================================================================
*** gcc/dwarf2out.c	(revision 188176)
--- gcc/dwarf2out.c	(working copy)
*************** dwarf2out_finish (const char *filename)
*** 22158,22164 ****
    for (node = deferred_asm_name; node; node = node->next)
      {
        tree decl = node->created_for;
!       if (DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
  	{
  	  add_linkage_attr (node->die, decl);
  	  move_linkage_attr (node->die);
--- 22158,22168 ----
    for (node = deferred_asm_name; node; node = node->next)
      {
        tree decl = node->created_for;
!       /* When generating LTO bytecode we can not generate new assembler
!          names at this point and all important decls got theirs via
! 	 free-lang-data.  */
!       if ((!flag_generate_lto || DECL_ASSEMBLER_NAME_SET_P (decl))
! 	  && DECL_ASSEMBLER_NAME (decl) != DECL_NAME (decl))
  	{
  	  add_linkage_attr (node->die, decl);
  	  move_linkage_attr (node->die);