[debug-early] C++ clones and limbo DIEs
diff mbox

Message ID 54B87E5B.1090502@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Jan. 16, 2015, 2:58 a.m. UTC
Hi Jason.  Hi Richard.

While adjusting the limbdo_die_list for early debug I noticed we weren't 
early generating C++ constructors/destructors.  This got me on a detour 
of sorts.

I noticed dwarf2out's gen_member_die() disallows generation of clones 
earlier, by design:

      /* Don't include clones in the member list.  */
      if (DECL_ABSTRACT_ORIGIN (member))
        continue;

I played around trying to disable this "feature", but my approach ran 
into various walls, and I decided instead to attack it from the 
front-end side. The attached patch generates early DIEs for the C++ 
clones in the C++ parser. I'd be (un)happy to revisit the dwarf2out 
approach if it's deemed more appropriate.

Now back to limbdo_die_list... My approach is to flush the limbo list, 
generically, after the front-ends have finished, by adding a new 
"early_finish" debug hook.  This gets rid of any permanence into LTO 
time.  Then I flush it out again, if the middle end (or LTO, etc) has 
added any limbo DIEs.  I wish we didn't need to flush the limbo list 
past the compilation proper, but some things like Cilk+ generate 
function trees way late in the game, so we end up generating limbo DIEs 
past where I'd like to.  Please see the comment in the source code.

All in all, this patch should get rid of any middle-end dependence on 
the early debug generated limbo DIEs.

Are you happy with this?  I'd like to get your input before committing 
to this approach.

Thanks.
Aldy
commit cd20df2a48ce4f0d4e64626d61f8f61a810cf742
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Wed Jan 14 13:33:40 2015 -0800

    	* cp/decl2.c (c_parse_final_cleanups): Emit early debug
    	information for clones.
    	* dbxout.c (dbx_debug_hooks): Add early_finish field.
    	(xcoff_debug_hooks): Same.
    	* sdbout.c (sdb_debug_hooks): Same.
    	* vmsdbgout.c (vmsdbg_debug_hooks): Same.
    	* debug.c (do_nothing_debug_hooks): Same.
    	* debug.h (struct gcc_debug_hooks): Same.
    	* dwarf2out.c (dwarf2_debug_hooks): Same.
    	(dwarf2out_finish): Move limbo list handling to...
    	(dwarf2out_early_finish): ...here.
    	* toplev.c (compile_file): Call debug_hooks->early_finish.

Comments

Jason Merrill Jan. 16, 2015, 3:11 a.m. UTC | #1
On 01/15/2015 09:58 PM, Aldy Hernandez wrote:
> The attached patch generates early DIEs for the C++
> clones in the C++ parser.

This strikes me as an unnecessary abstraction violation.

> +  /* Emit debug information for clones.  */
> +  symtab_node *node;
> +  FOR_EACH_DEFINED_SYMBOL (node)
> +    if (DECL_ABSTRACT_ORIGIN (node->decl))
> +      debug_hooks->early_global_decl (DECL_ABSTRACT_ORIGIN (node->decl));

And I note that you aren't even doing anything with the clone here, 
which suggests even more strongly that this is not what we want to do.

> Now back to limbdo_die_list... My approach is to flush the limbo list,
> generically, after the front-ends have finished, by adding a new
> "early_finish" debug hook.  This gets rid of any permanence into LTO
> time.  Then I flush it out again, if the middle end (or LTO, etc) has
> added any limbo DIEs.

Can you remove the first flush and just do it in the second place?

Jason
Richard Biener Jan. 16, 2015, 10:55 a.m. UTC | #2
On Fri, Jan 16, 2015 at 4:11 AM, Jason Merrill <jason@redhat.com> wrote:
> On 01/15/2015 09:58 PM, Aldy Hernandez wrote:
>>
>> The attached patch generates early DIEs for the C++
>> clones in the C++ parser.
>
>
> This strikes me as an unnecessary abstraction violation.

I'd hope that in the very distant future all early DIEs would be "created"
by the frontends (that is, dwarf2out.c wouldn't walk into parents/siblings
so much).

>> +  /* Emit debug information for clones.  */
>> +  symtab_node *node;
>> +  FOR_EACH_DEFINED_SYMBOL (node)
>> +    if (DECL_ABSTRACT_ORIGIN (node->decl))
>> +      debug_hooks->early_global_decl (DECL_ABSTRACT_ORIGIN (node->decl));
>
>
> And I note that you aren't even doing anything with the clone here, which
> suggests even more strongly that this is not what we want to do.

There must be a better place in the frontend to call early_global_decl
for those?  Maybe the point where we create the clone?

>> Now back to limbdo_die_list... My approach is to flush the limbo list,
>> generically, after the front-ends have finished, by adding a new
>> "early_finish" debug hook.  This gets rid of any permanence into LTO
>> time.  Then I flush it out again, if the middle end (or LTO, etc) has
>> added any limbo DIEs.
>
>
> Can you remove the first flush and just do it in the second place?

I hoped we wouldn't need the limbo list at all ... that is, parent DIEs
are always present when we create children.  I think that should
work in principle if the frontends would create DIEs while parsing.

Note that dwarf2out forces parent DIE creation in some cases
but not in some others - it would be interesting to sort out which
parent DIEs it thinks it cannot create when we create the DIE
for a sibling.  Maybe it's just poor ordering of early_global_decl
calls?

Richard.

> Jason
>
Jason Merrill Jan. 16, 2015, 4:31 p.m. UTC | #3
On 01/16/2015 05:55 AM, Richard Biener wrote:
> I'd hope that in the very distant future all early DIEs would be "created"
> by the frontends (that is, dwarf2out.c wouldn't walk into parents/siblings
> so much).

Are you thinking that the front end would immediately call a debug hook 
for every block, local variable and such, or just for higher level entities?

> I hoped we wouldn't need the limbo list at all ... that is, parent DIEs
> are always present when we create children.  I think that should
> work in principle if the frontends would create DIEs while parsing.

So create the function DIE as soon as we see the declaration?  That 
seems reasonable.  Then that would be the point of early debug, not 
later at EOF.

> Note that dwarf2out forces parent DIE creation in some cases
> but not in some others - it would be interesting to sort out which
> parent DIEs it thinks it cannot create when we create the DIE
> for a sibling.  Maybe it's just poor ordering of early_global_decl
> calls?

Agreed.

Jason
Aldy Hernandez Jan. 16, 2015, 5:50 p.m. UTC | #4
On 01/15/2015 07:11 PM, Jason Merrill wrote:
> On 01/15/2015 09:58 PM, Aldy Hernandez wrote:

>> Now back to limbdo_die_list... My approach is to flush the limbo list,
>> generically, after the front-ends have finished, by adding a new
>> "early_finish" debug hook.  This gets rid of any permanence into LTO
>> time.  Then I flush it out again, if the middle end (or LTO, etc) has
>> added any limbo DIEs.
>
> Can you remove the first flush and just do it in the second place?

If I only flush the limbo list in the second place, that's basically 
what mainline does, albeit abstracted into a function.  I thought the 
whole point was to get rid of the limbo list, or at least keep it from 
being a structure that has to go through LTO streaming.

Aldy
Aldy Hernandez Jan. 16, 2015, 5:59 p.m. UTC | #5
On 01/16/2015 08:31 AM, Jason Merrill wrote:
> On 01/16/2015 05:55 AM, Richard Biener wrote:
>> I'd hope that in the very distant future all early DIEs would be
>> "created"
>> by the frontends (that is, dwarf2out.c wouldn't walk into
>> parents/siblings
>> so much).
>
> Are you thinking that the front end would immediately call a debug hook
> for every block, local variable and such, or just for higher level
> entities?

In the very distant future, as in, when Aldy is retired and living in a 
tropical island somewhere? :).

>
>> I hoped we wouldn't need the limbo list at all ... that is, parent DIEs
>> are always present when we create children.  I think that should
>> work in principle if the frontends would create DIEs while parsing.
>
> So create the function DIE as soon as we see the declaration?  That
> seems reasonable.  Then that would be the point of early debug, not
> later at EOF.

I'm certainly game to exploring this option, though I think we should be 
able to get this working at EOF.  The reason why I didn't take this 
approach originally is because it seemed like a lot more hassle, 
especially since we have to do the same thing for all other front-ends.

>
>> Note that dwarf2out forces parent DIE creation in some cases
>> but not in some others - it would be interesting to sort out which
>> parent DIEs it thinks it cannot create when we create the DIE
>> for a sibling.  Maybe it's just poor ordering of early_global_decl
>> calls?
>
> Agreed.

Regrettably, I have to agree as well.  I can investigate why some DIEs 
end up orphaned and report back.

Thanks.
Aldy
Richard Biener Jan. 16, 2015, 6:41 p.m. UTC | #6
On January 16, 2015 5:31:49 PM CET, Jason Merrill <jason@redhat.com> wrote:
>On 01/16/2015 05:55 AM, Richard Biener wrote:
>> I'd hope that in the very distant future all early DIEs would be
>"created"
>> by the frontends (that is, dwarf2out.c wouldn't walk into
>parents/siblings
>> so much).
>
>Are you thinking that the front end would immediately call a debug hook
>
>for every block, local variable and such, or just for higher level
>entities?

For every block, local variable and such.
Yes.  The FE then also has the chance to append whatever FE specific attributes without langhooks in dwarf2out.

>> I hoped we wouldn't need the limbo list at all ... that is, parent
>DIEs
>> are always present when we create children.  I think that should
>> work in principle if the frontends would create DIEs while parsing.
>
>So create the function DIE as soon as we see the declaration?  That 
>seems reasonable.  Then that would be the point of early debug, not 
>later at EOF.

Yes.

Richard.

>> Note that dwarf2out forces parent DIE creation in some cases
>> but not in some others - it would be interesting to sort out which
>> parent DIEs it thinks it cannot create when we create the DIE
>> for a sibling.  Maybe it's just poor ordering of early_global_decl
>> calls?
>
>Agreed.
>
>Jason
Jason Merrill Jan. 16, 2015, 8:45 p.m. UTC | #7
On 01/16/2015 12:50 PM, Aldy Hernandez wrote:
>> Can you remove the first flush and just do it in the second place?
>
> If I only flush the limbo list in the second place, that's basically
> what mainline does, albeit abstracted into a function.  I thought the
> whole point was to get rid of the limbo list, or at least keep it from
> being a structure that has to go through LTO streaming.

It would expect it to be before free_lang_data and LTO streaming.

Jason
Aldy Hernandez Jan. 28, 2015, 6:33 p.m. UTC | #8
On 01/16/2015 02:55 AM, Richard Biener wrote:
> On Fri, Jan 16, 2015 at 4:11 AM, Jason Merrill <jason@redhat.com> wrote:
>> On 01/15/2015 09:58 PM, Aldy Hernandez wrote:

> I hoped we wouldn't need the limbo list at all ... that is, parent DIEs
> are always present when we create children.  I think that should
> work in principle if the frontends would create DIEs while parsing.

By the way, don't think I ignored this suggestion.  I played around with 
this approach for the C++ front-end, and gave up after having to keep 
track of a few too many things to get the parenthood right.  It's 
totally doable; it was just easier to flush out the limbo DIE list as 
with my current patch.  Ok, call me lazy :).

> Note that dwarf2out forces parent DIE creation in some cases
> but not in some others - it would be interesting to sort out which
> parent DIEs it thinks it cannot create when we create the DIE
> for a sibling.  Maybe it's just poor ordering of early_global_decl
> calls?

In my current patch, I added an ICE to make sure we're not creating 
limbo destined DIEs past early dwarf dumping.  The only exception to 
this (temporarily) is that the ltrans stage is adding things late, but 
that's only because we're not streaming dwarf as the ultimate plan is. 
I've documented all this.

Aldy

Patch
diff mbox

diff --git a/gcc/cp/decl2.c b/gcc/cp/decl2.c
index 691688b..b9a407a 100644
--- a/gcc/cp/decl2.c
+++ b/gcc/cp/decl2.c
@@ -4737,6 +4737,13 @@  c_parse_final_cleanups (void)
      generate initial debug information.  */
   timevar_stop (TV_PHASE_PARSING);
   timevar_start (TV_PHASE_DBGINFO);
+
+  /* Emit debug information for clones.  */
+  symtab_node *node;
+  FOR_EACH_DEFINED_SYMBOL (node)
+    if (DECL_ABSTRACT_ORIGIN (node->decl))
+      debug_hooks->early_global_decl (DECL_ABSTRACT_ORIGIN (node->decl));
+
   walk_namespaces (emit_debug_for_namespace, 0);
   if (vec_safe_length (pending_statics) != 0)
     {
diff --git a/gcc/dbxout.c b/gcc/dbxout.c
index 430a2eb..202ef8a 100644
--- a/gcc/dbxout.c
+++ b/gcc/dbxout.c
@@ -359,6 +359,7 @@  const struct gcc_debug_hooks dbx_debug_hooks =
   dbxout_init,
   dbxout_finish,
   debug_nothing_void,
+  debug_nothing_void,
   debug_nothing_int_charstar,
   debug_nothing_int_charstar,
   dbxout_start_source_file,
@@ -400,6 +401,7 @@  const struct gcc_debug_hooks xcoff_debug_hooks =
   dbxout_init,
   dbxout_finish,
   debug_nothing_void,
+  debug_nothing_void,
   debug_nothing_int_charstar,
   debug_nothing_int_charstar,
   dbxout_start_source_file,
diff --git a/gcc/debug.c b/gcc/debug.c
index 449d3a1..d0e00c0 100644
--- a/gcc/debug.c
+++ b/gcc/debug.c
@@ -27,6 +27,7 @@  const struct gcc_debug_hooks do_nothing_debug_hooks =
 {
   debug_nothing_charstar,
   debug_nothing_charstar,
+  debug_nothing_void,			/* early_finish */
   debug_nothing_void,
   debug_nothing_int_charstar,
   debug_nothing_int_charstar,
diff --git a/gcc/debug.h b/gcc/debug.h
index f9485bc..a8d3f23 100644
--- a/gcc/debug.h
+++ b/gcc/debug.h
@@ -30,6 +30,9 @@  struct gcc_debug_hooks
   /* Output debug symbols.  */
   void (* finish) (const char *main_filename);
 
+  /* Run cleanups necessary after early debug generation.  */
+  void (* early_finish) (void);
+
   /* Called from cgraph_optimize before starting to assemble
      functions/variables/toplevel asms.  */
   void (* assembly_start) (void);
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index e3ccda2..80d43af 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -2424,6 +2424,7 @@  build_cfa_aligned_loc (dw_cfa_location *cfa,
 
 static void dwarf2out_init (const char *);
 static void dwarf2out_finish (const char *);
+static void dwarf2out_early_finish (void);
 static void dwarf2out_assembly_start (void);
 static void dwarf2out_define (unsigned int, const char *);
 static void dwarf2out_undef (unsigned int, const char *);
@@ -2451,6 +2452,7 @@  const struct gcc_debug_hooks dwarf2_debug_hooks =
 {
   dwarf2out_init,
   dwarf2out_finish,
+  dwarf2out_early_finish,
   dwarf2out_assembly_start,
   dwarf2out_define,
   dwarf2out_undef,
@@ -24739,10 +24741,27 @@  optimize_location_lists (dw_die_ref die)
 static void
 dwarf2out_finish (const char *filename)
 {
-  limbo_die_node *node, *next_node;
   comdat_type_node *ctnode;
   dw_die_ref main_comp_unit_die;
 
+  /* Technically limbo_die_list should either be empty because
+     dwarf2out_early_finish took care of everything, or should _only_
+     have inline instances that will be created late (which is OK,
+     because they'll be pointing to the early generated abstract).
+
+     However, functions can be created after the parsers are done
+     (e.g. create_cilk_helper_decl), which will populate the
+     limbo_die_list again.  Eventually all late function creation
+     should be banned, or these new functions should be created with a
+     proper context_die, thus avoiding limbo.  For now, it doesn't
+     hurt, since late creation should have all the location
+     information needed (and if it doesn't, we don't care-- the code
+     is undebuggable).
+
+     If there was anything created late, flush it out.  */
+  if (limbo_die_list)
+    dwarf2out_early_finish ();
+
   /* PCH might result in DW_AT_producer string being restored from the
      header compilation, so always fill it with empty string initially
      and overwrite only here.  */
@@ -24767,55 +24786,6 @@  dwarf2out_finish (const char *filename)
 	add_comp_dir_attribute (comp_unit_die ());
     }
 
-  /* Traverse the limbo die list, and add parent/child links.  The only
-     dies without parents that should be here are concrete instances of
-     inline functions, and the comp_unit_die.  We can ignore the comp_unit_die.
-     For concrete instances, we can get the parent die from the abstract
-     instance.  */
-  for (node = limbo_die_list; node; node = next_node)
-    {
-      dw_die_ref die = node->die;
-      next_node = node->next;
-
-      if (die->die_parent == NULL)
-	{
-	  dw_die_ref origin = get_AT_ref (die, DW_AT_abstract_origin);
-
-	  if (origin && origin->die_parent)
-	    add_child_die (origin->die_parent, die);
-	  else if (is_cu_die (die))
-	    ;
-	  else if (seen_error ())
-	    /* It's OK to be confused by errors in the input.  */
-	    add_child_die (comp_unit_die (), die);
-	  else
-	    {
-	      /* In certain situations, the lexical block containing a
-		 nested function can be optimized away, which results
-		 in the nested function die being orphaned.  Likewise
-		 with the return type of that nested function.  Force
-		 this to be a child of the containing function.
-
-		 It may happen that even the containing function got fully
-		 inlined and optimized out.  In that case we are lost and
-		 assign the empty child.  This should not be big issue as
-		 the function is likely unreachable too.  */
-	      gcc_assert (node->created_for);
-
-	      if (DECL_P (node->created_for))
-		origin = get_context_die (DECL_CONTEXT (node->created_for));
-	      else if (TYPE_P (node->created_for))
-		origin = scope_die_for (node->created_for, comp_unit_die ());
-	      else
-		origin = comp_unit_die ();
-
-	      add_child_die (origin, die);
-	    }
-	}
-    }
-
-  limbo_die_list = NULL;
-
 #if ENABLE_ASSERT_CHECKING
   {
     dw_die_ref die = comp_unit_die (), c;
@@ -24860,9 +24830,10 @@  dwarf2out_finish (const char *filename)
   if (flag_eliminate_dwarf2_dups)
     break_out_includes (comp_unit_die ());
 
-  /* Traverse the DIE's and add add sibling attributes to those DIE's
+  /* Traverse the DIE's and add sibling attributes to those DIE's
      that have children.  */
   add_sibling_attributes (comp_unit_die ());
+  limbo_die_node *node;
   for (node = limbo_die_list; node; node = node->next)
     add_sibling_attributes (node->die);
   for (ctnode = comdat_type_list; ctnode != NULL; ctnode = ctnode->next)
@@ -25124,6 +25095,66 @@  dwarf2out_finish (const char *filename)
     output_indirect_strings ();
 }
 
+/* Perform any cleanups needed after the early debug generation pass
+   has run.  */
+
+static void
+dwarf2out_early_finish (void)
+{
+  /* Traverse the limbo die list, and add parent/child links.  The only
+     dies without parents that should be here are concrete instances of
+     inline functions, and the comp_unit_die.  We can ignore the comp_unit_die.
+     For concrete instances, we can get the parent die from the abstract
+     instance.
+
+     The point here is to flush out the limbo list so that it is empty
+     and we don't need to stream it for LTO.  */
+  limbo_die_node *node, *next_node;
+  for (node = limbo_die_list; node; node = next_node)
+    {
+      dw_die_ref die = node->die;
+      next_node = node->next;
+
+      if (die->die_parent == NULL)
+	{
+	  dw_die_ref origin = get_AT_ref (die, DW_AT_abstract_origin);
+
+	  if (origin && origin->die_parent)
+	    add_child_die (origin->die_parent, die);
+	  else if (is_cu_die (die))
+	    ;
+	  else if (seen_error ())
+	    /* It's OK to be confused by errors in the input.  */
+	    add_child_die (comp_unit_die (), die);
+	  else
+	    {
+	      /* In certain situations, the lexical block containing a
+		 nested function can be optimized away, which results
+		 in the nested function die being orphaned.  Likewise
+		 with the return type of that nested function.  Force
+		 this to be a child of the containing function.
+
+		 It may happen that even the containing function got fully
+		 inlined and optimized out.  In that case we are lost and
+		 assign the empty child.  This should not be big issue as
+		 the function is likely unreachable too.  */
+	      gcc_assert (node->created_for);
+
+	      if (DECL_P (node->created_for))
+		origin = get_context_die (DECL_CONTEXT (node->created_for));
+	      else if (TYPE_P (node->created_for))
+		origin = scope_die_for (node->created_for, comp_unit_die ());
+	      else
+		origin = comp_unit_die ();
+
+	      add_child_die (origin, die);
+	    }
+	}
+    }
+
+  limbo_die_list = NULL;
+}
+
 /* Reset all state within dwarf2out.c so that we can rerun the compiler
    within the same process.  For use by toplev::finalize.  */
 
diff --git a/gcc/sdbout.c b/gcc/sdbout.c
index d7b2d6b..43b8cf2 100644
--- a/gcc/sdbout.c
+++ b/gcc/sdbout.c
@@ -279,6 +279,7 @@  const struct gcc_debug_hooks sdb_debug_hooks =
 {
   sdbout_init,			         /* init */
   sdbout_finish,		         /* finish */
+  debug_nothing_void,			 /* early_finish */
   debug_nothing_void,			 /* assembly_start */
   debug_nothing_int_charstar,	         /* define */
   debug_nothing_int_charstar,	         /* undef */
diff --git a/gcc/toplev.c b/gcc/toplev.c
index 42a2cdc..12fa509 100644
--- a/gcc/toplev.c
+++ b/gcc/toplev.c
@@ -586,6 +586,10 @@  compile_file (void)
   if (flag_syntax_only || flag_wpa)
     return;
 
+  /* Clean up anything that needs cleaning up after initial debug
+     generation.  */
+  (*debug_hooks->early_finish) ();
+
   ggc_protect_identifiers = false;
 
   /* Run the actual compilation process.  */
diff --git a/gcc/vmsdbgout.c b/gcc/vmsdbgout.c
index 5cb66bc..6da48eb 100644
--- a/gcc/vmsdbgout.c
+++ b/gcc/vmsdbgout.c
@@ -179,6 +179,7 @@  static void vmsdbgout_abstract_function (tree);
 const struct gcc_debug_hooks vmsdbg_debug_hooks
 = {vmsdbgout_init,
    vmsdbgout_finish,
+   debug_nothing_void,
    vmsdbgout_assembly_start,
    vmsdbgout_define,
    vmsdbgout_undef,