diff mbox

[debug-early] reuse variable DIEs and fix their context

Message ID 53FD45A7.4000804@redhat.com
State New
Headers show

Commit Message

Aldy Hernandez Aug. 27, 2014, 2:42 a.m. UTC 
This patch fixes a bunch of guality failures.  With it I get 144 
guality.exp failures vs. 163 for "make check-gcc 
RUNTESTFLAGS=guality.exp".  A lot better than 100% fail rate ;-).

Variable DIEs were not being reused.  Instead, variable DIEs even had 
the wrong context (unilaterally the compilation unit).  The attached 
patch reuses variable DIEs that have been outputted earlier.  It also 
fixes the context by correcting the context on the second round.

I have also added a bit field to the DIE structure to record if a DIE 
has been generated early.

Again, this is all a rough draft, but feel free to comment.

Committed to branch.
Aldy
commit 71d99991672200b6bcfe258ae0bfe92ea59af3ea
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Tue Aug 26 19:12:17 2014 -0700

    	* dwarf2out.c (struct die_struct): Add dumped_early field.
    	(reparent_child): New.
    	(splice_child_die): Use reparent_child.
    	(gen_subprogram_die): Do not regenerate parameters if previously
    	dumped.
    	(gen_variable_die): Fix parent of decls that have been dumped
    	early to reflect correct context.
    	Do not regenerate decls if previously dumped.
    	(dwarf2out_decl): Add return value.
    	(dwarf2out_early_decl): Set dumped_early bit.

Comments

Richard Biener Aug. 28, 2014, 1:58 p.m. UTC | #1 
On Wed, Aug 27, 2014 at 4:42 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> This patch fixes a bunch of guality failures.  With it I get 144 guality.exp
> failures vs. 163 for "make check-gcc RUNTESTFLAGS=guality.exp".  A lot
> better than 100% fail rate ;-).
>
> Variable DIEs were not being reused.  Instead, variable DIEs even had the
> wrong context (unilaterally the compilation unit).  The attached patch
> reuses variable DIEs that have been outputted earlier.  It also fixes the
> context by correcting the context on the second round.
>
> I have also added a bit field to the DIE structure to record if a DIE has
> been generated early.
>
> Again, this is all a rough draft, but feel free to comment.

I wonder if we can't not force a proper context die (ISTR dwarf2out.c
lazily handles some contexts in some circumstances).  All parent
"trees" should be readily available and we should be able to create
DIEs for them.

Richard.

> Committed to branch.
> Aldy
Aldy Hernandez Aug. 28, 2014, 5:34 p.m. UTC | #2 
On 08/28/14 06:58, Richard Biener wrote:
> On Wed, Aug 27, 2014 at 4:42 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
>> This patch fixes a bunch of guality failures.  With it I get 144 guality.exp
>> failures vs. 163 for "make check-gcc RUNTESTFLAGS=guality.exp".  A lot
>> better than 100% fail rate ;-).
>>
>> Variable DIEs were not being reused.  Instead, variable DIEs even had the
>> wrong context (unilaterally the compilation unit).  The attached patch
>> reuses variable DIEs that have been outputted earlier.  It also fixes the
>> context by correcting the context on the second round.
>>
>> I have also added a bit field to the DIE structure to record if a DIE has
>> been generated early.
>>
>> Again, this is all a rough draft, but feel free to comment.
>
> I wonder if we can't not force a proper context die (ISTR dwarf2out.c
> lazily handles some contexts in some circumstances).  All parent

Hmmm, I don't see any of this lazy context setting you speak of, but...

> "trees" should be readily available and we should be able to create
> DIEs for them.

I wonder if instead of early dumping of all the DECLs, we could only 
dump the toplevel scoped DECLs, and let inheritance set the proper contexts.

We could start with calling dwarf2out_early_decl() for each function 
decl, and then for every global.  This is analogous to what we currently 
do for late dwarf2out.

see final.c for the functions:
       if (!DECL_IGNORED_P (current_function_decl))
         debug_hooks->function_decl (current_function_decl);

see c/c-decl.c for the globals:
       FOR_EACH_VEC_ELT (*all_translation_units, i, t)
	c_write_global_declarations_2 (BLOCK_VARS (DECL_INITIAL (t)));
       c_write_global_declarations_2 (BLOCK_VARS (ext_block));

The problem being that to calculate `ext_block' above, we need intimate 
knowledge of scopes and such, only available in the FE.  Is there a 
generic way of determining if a DECL is in global scope?  If, so we 
could do:

	foreach decl in fld.decls
		if (is_global_scope(decl))
			dwarf2out_decl (decl)

...and contexts will magically be set.

Is there such a way, or am I going about this the wrong way?

Aldy
Jason Merrill Aug. 28, 2014, 6:01 p.m. UTC | #3 
On 08/28/2014 01:34 PM, Aldy Hernandez wrote:
> I wonder if instead of early dumping of all the DECLs, we could only
> dump the toplevel scoped DECLs, and let inheritance set the proper
> contexts.

Yes, I think this makes a lot more sense; do it at a well-defined point 
in compilation rather than as part of free_lang_data.

> We could start with calling dwarf2out_early_decl() for each function
> decl, and then for every global.  This is analogous to what we currently
> do for late dwarf2out.
>
> see final.c for the functions:
>        if (!DECL_IGNORED_P (current_function_decl))
>          debug_hooks->function_decl (current_function_decl);
>
> see c/c-decl.c for the globals:
>        FOR_EACH_VEC_ELT (*all_translation_units, i, t)
>      c_write_global_declarations_2 (BLOCK_VARS (DECL_INITIAL (t)));
>        c_write_global_declarations_2 (BLOCK_VARS (ext_block));

> The problem being that to calculate `ext_block' above, we need intimate
> knowledge of scopes and such, only available in the FE.  Is there a
> generic way of determining if a DECL is in global scope?

Why not do it in the FE, i.e. *_write_global_declarations?

Jason
Richard Biener Aug. 28, 2014, 7:13 p.m. UTC | #4 
On August 28, 2014 8:01:05 PM CEST, Jason Merrill <jason@redhat.com> wrote:
>On 08/28/2014 01:34 PM, Aldy Hernandez wrote:
>> I wonder if instead of early dumping of all the DECLs, we could only
>> dump the toplevel scoped DECLs, and let inheritance set the proper
>> contexts.
>
>Yes, I think this makes a lot more sense; do it at a well-defined point
>
>in compilation rather than as part of free_lang_data.
>
>> We could start with calling dwarf2out_early_decl() for each function
>> decl, and then for every global.  This is analogous to what we
>currently
>> do for late dwarf2out.
>>
>> see final.c for the functions:
>>        if (!DECL_IGNORED_P (current_function_decl))
>>          debug_hooks->function_decl (current_function_decl);
>>
>> see c/c-decl.c for the globals:
>>        FOR_EACH_VEC_ELT (*all_translation_units, i, t)
>>      c_write_global_declarations_2 (BLOCK_VARS (DECL_INITIAL (t)));
>>        c_write_global_declarations_2 (BLOCK_VARS (ext_block));
>
>> The problem being that to calculate `ext_block' above, we need
>intimate
>> knowledge of scopes and such, only available in the FE.  Is there a
>> generic way of determining if a DECL is in global scope?

Via DECL_CONTEXT and the global scope macro predicate.  Eventually not enough to detect class scope statics.

>Why not do it in the FE, i.e. *_write_global_declarations?

Yeah, ultimatively I'd like the front ends to do all required dwarf2out calls but free lang data seemed a convenient place to do things.

There is no reason we can't walk its array in a more sensible order.

Richard.
>
>Jason
Jason Merrill Aug. 28, 2014, 8:14 p.m. UTC | #5 
On 08/28/2014 03:13 PM, Richard Biener wrote:
>>> knowledge of scopes and such, only available in the FE.  Is there a
>>> generic way of determining if a DECL is in global scope?
>
> Via DECL_CONTEXT and the global scope macro predicate.  Eventually not enough to detect class scope statics.

!decl_function_context should do the trick.

Jason
Richard Biener Aug. 29, 2014, 9:09 a.m. UTC | #6 
On Thu, Aug 28, 2014 at 10:14 PM, Jason Merrill <jason@redhat.com> wrote:
> On 08/28/2014 03:13 PM, Richard Biener wrote:
>>>>
>>>> knowledge of scopes and such, only available in the FE.  Is there a
>>>> generic way of determining if a DECL is in global scope?
>>
>>
>> Via DECL_CONTEXT and the global scope macro predicate.  Eventually not
>> enough to detect class scope statics.
>
>
> !decl_function_context should do the trick.

Yeah, that might work.  But I suppose we want proper ordering even
for a namespace hierarchy, that is, output dwarf for the namespace
which will output its siblings.

That is, dwarf2out.c currently kind-of supports both, first create
DIEs for the context which will usually create DIEs for its siblings
or first create siblings which will either end up in the limbo-list
or get a context DIE via force_decl/type_die (see get_context_die).

IMHO the best would be to create DIEs top-down controlled
by frontends and also populate context DIEs from the frontends
and not by walking some sibling list from dwarf2out.c.

Frontends already "announce" some types/decls to dwarf2out.c
by means of calling rest_of_decl_compilation or rest_of_type_compilation
(only for TYPE_DECLs).  So it's mostly a matter of adding similar calls
for scopes and global decls.  The calls should be debug_hook calls
of course, not these weird rest_of_*_compilation stuff.

Richard.

> Jason
>
diff mbox

Patch

diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 0aa4456..32c17ce 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -102,7 +102,7 @@  along with GCC; see the file COPYING3.  If not see
 static void dwarf2out_source_line (unsigned int, const char *, int, bool);
 static rtx_insn *last_var_location_insn;
 static rtx_insn *cached_next_real_insn;
-static void dwarf2out_decl (tree);
+static dw_die_ref dwarf2out_decl (tree);
 
 #ifdef VMS_DEBUGGING_INFO
 int vms_file_stats_name (const char *, long long *, long *, char *, int *);
@@ -2608,6 +2608,8 @@  typedef struct GTY((chain_circular ("%h.die_sib"))) die_struct {
   /* Die is used and must not be pruned as unused.  */
   BOOL_BITFIELD die_perennial_p : 1;
   BOOL_BITFIELD comdat_type_p : 1; /* DIE has a type signature */
+  /* Die was generated early via dwarf2out_early_decl.  */
+  BOOL_BITFIELD dumped_early : 1;
   /* Lots of spare bits.  */
 }
 die_node;
@@ -4808,6 +4810,21 @@  add_child_die (dw_die_ref die, dw_die_ref child_die)
   die->die_child = child_die;
 }
 
+/* Unassociate CHILD from its parent, and make its parent be
+   NEW_PARENT.  */
+
+static void
+reparent_child (dw_die_ref child, dw_die_ref new_parent)
+{
+  for (dw_die_ref p = child->die_parent->die_child; ; p = p->die_sib)
+    if (p->die_sib == child)
+      {
+	remove_child_with_prev (child, p);
+	break;
+      }
+  add_child_die (new_parent, child);
+}
+
 /* Move CHILD, which must be a child of PARENT or the DIE for which PARENT
    is the specification, to the end of PARENT's list of children.
    This is done by removing and re-adding it.  */
@@ -4815,8 +4832,6 @@  add_child_die (dw_die_ref die, dw_die_ref child_die)
 static void
 splice_child_die (dw_die_ref parent, dw_die_ref child)
 {
-  dw_die_ref p;
-
   /* We want the declaration DIE from inside the class, not the
      specification DIE at toplevel.  */
   if (child->die_parent != parent)
@@ -4831,14 +4846,7 @@  splice_child_die (dw_die_ref parent, dw_die_ref child)
 	      || (child->die_parent
 		  == get_AT_ref (parent, DW_AT_specification)));
 
-  for (p = child->die_parent->die_child; ; p = p->die_sib)
-    if (p->die_sib == child)
-      {
-	remove_child_with_prev (child, p);
-	break;
-      }
-
-  add_child_die (parent, child);
+  reparent_child (child, parent);
 }
 
 /* Return a pointer to a newly created DIE node.  */
@@ -18288,16 +18296,17 @@  gen_subprogram_die (tree decl, dw_die_ref context_die)
 	  /* ??? Hmmm, early dwarf generation happened earlier, so no
 	     sense in removing the parameters.  Let's keep them and
 	     augment them with location information later.  */
-#if 0
-	  /* Clear out the declaration attribute and the formal parameters.
-	     Do not remove all children, because it is possible that this
-	     declaration die was forced using force_decl_die(). In such
-	     cases die that forced declaration die (e.g. TAG_imported_module)
-	     is one of the children that we do not want to remove.  */
-	  remove_AT (subr_die, DW_AT_declaration);
-	  remove_AT (subr_die, DW_AT_object_pointer);
-	  remove_child_TAG (subr_die, DW_TAG_formal_parameter);
-#endif
+	  if (!old_die->dumped_early)
+	    {
+	      /* Clear out the declaration attribute and the formal parameters.
+		 Do not remove all children, because it is possible that this
+		 declaration die was forced using force_decl_die(). In such
+		 cases die that forced declaration die (e.g. TAG_imported_module)
+		 is one of the children that we do not want to remove.  */
+	      remove_AT (subr_die, DW_AT_declaration);
+	      remove_AT (subr_die, DW_AT_object_pointer);
+	      remove_child_TAG (subr_die, DW_TAG_formal_parameter);
+	    }
 	}
       else
 	{
@@ -18998,6 +19007,18 @@  gen_variable_die (tree decl, tree origin, dw_die_ref context_die)
   if (old_die && !declaration && !local_scope_p (context_die))
     return;
 
+  /* When DIEs are created early, the context is the compilation unit.
+     Adjust the context when we know what it is the second time
+     around.  */
+  if (old_die && old_die->dumped_early)
+    {
+      if (old_die->die_parent != context_die)
+	reparent_child (old_die, context_die);
+      var_die = old_die;
+      old_die = NULL;
+      goto gen_variable_die_location;
+    }
+
   /* For static data members, the declaration in the class is supposed
      to have DW_TAG_member tag; the specification should still be
      DW_TAG_variable referencing the DW_TAG_member DIE.  */
@@ -19081,6 +19102,7 @@  gen_variable_die (tree decl, tree origin, dw_die_ref context_die)
   if (decl && (DECL_ABSTRACT (decl) || declaration || old_die == NULL))
     equate_decl_number_to_die (decl, var_die);
 
+ gen_variable_die_location:
   if (! declaration
       && (! DECL_ABSTRACT (decl_or_origin)
 	  /* Local static vars are shared between all clones/inlines,
@@ -20988,9 +21010,9 @@  gen_namelist_decl (tree name, dw_die_ref scope_die, tree item_decls)
 }
 
 
-/* Write the debugging output for DECL.  */
+/* Write the debugging output for DECL and return the DIE.  */
 
-static void
+static dw_die_ref
 dwarf2out_decl (tree decl)
 {
   dw_die_ref context_die = comp_unit_die ();
@@ -20998,7 +21020,7 @@  dwarf2out_decl (tree decl)
   switch (TREE_CODE (decl))
     {
     case ERROR_MARK:
-      return;
+      return NULL;
 
     case FUNCTION_DECL:
       /* What we would really like to do here is to filter out all mere
@@ -21035,7 +21057,7 @@  dwarf2out_decl (tree decl)
 	 or not at all.  */
       if (DECL_INITIAL (decl) == NULL_TREE
 	  && ! DECL_ABSTRACT (decl))
-	return;
+	return NULL;
 
       /* If we're a nested function, initially use a parent of NULL; if we're
 	 a plain function, this will be fixed up in decls_for_scope.  If
@@ -21056,7 +21078,7 @@  dwarf2out_decl (tree decl)
 	 would screw-up the debugger's name lookup mechanism and cause it to
 	 miss things which really ought to be in scope at a given point.  */
       if (DECL_EXTERNAL (decl) && !TREE_USED (decl))
-	return;
+	return NULL;
 
       /* For local statics lookup proper context die.  */
       if (TREE_STATIC (decl)
@@ -21067,14 +21089,14 @@  dwarf2out_decl (tree decl)
       /* If we are in terse mode, don't generate any DIEs to represent any
 	 variable declarations or definitions.  */
       if (debug_info_level <= DINFO_LEVEL_TERSE)
-	return;
+	return NULL;
       break;
 
     case CONST_DECL:
       if (debug_info_level <= DINFO_LEVEL_TERSE)
-	return;
+	return NULL;
       if (!is_fortran () && !is_ada ())
-	return;
+	return NULL;
       if (TREE_STATIC (decl) && decl_function_context (decl))
 	context_die = lookup_decl_die (DECL_CONTEXT (decl));
       break;
@@ -21082,24 +21104,24 @@  dwarf2out_decl (tree decl)
     case NAMESPACE_DECL:
     case IMPORTED_DECL:
       if (debug_info_level <= DINFO_LEVEL_TERSE)
-	return;
+	return NULL;
       if (lookup_decl_die (decl) != NULL)
-	return;
+	return NULL;
       break;
 
     case TYPE_DECL:
       /* Don't emit stubs for types unless they are needed by other DIEs.  */
       if (TYPE_DECL_SUPPRESS_DEBUG (decl))
-	return;
+	return NULL;
 
       /* Don't bother trying to generate any DIEs to represent any of the
 	 normal built-in types for the language we are compiling.  */
       if (DECL_IS_BUILTIN (decl))
-	return;
+	return NULL;
 
       /* If we are in terse mode, don't generate any DIEs for types.  */
       if (debug_info_level <= DINFO_LEVEL_TERSE)
-	return;
+	return NULL;
 
       /* If we're a function-scope tag, initially use a parent of NULL;
 	 this will be fixed up in decls_for_scope.  */
@@ -21112,7 +21134,7 @@  dwarf2out_decl (tree decl)
       break;
 
     default:
-      return;
+      return NULL;
     }
 
   gen_decl_die (decl, NULL, context_die);
@@ -21120,6 +21142,7 @@  dwarf2out_decl (tree decl)
   dw_die_ref die = lookup_decl_die (decl);
   if (die)
     check_die (die, 0);
+  return die;
 }
 
 /* Early dumping of DECLs before we lose language data.  */
@@ -21135,8 +21158,8 @@  dwarf2out_early_decl (tree decl)
      cgraph information, causing cgraph_function_possibly_inlined_p()
      to return true.  Trick cgraph_function_possibly_inlined_p()
      while we generate dwarf early.  */
-  bool save = cgraph_global_info_ready;
-  cgraph_global_info_ready = true;
+  bool save = symtab->global_info_ready;
+  symtab->global_info_ready = true;
 
   /* We don't handle TYPE_DECLs.  If required, they'll be reached via
      other DECLs and they can point to template types or other things
@@ -21154,7 +21177,9 @@  dwarf2out_early_decl (tree decl)
 	  push_cfun (DECL_STRUCT_FUNCTION (decl));
 	  current_function_decl = decl;
 	}
-      dwarf2out_decl (decl);
+      dw_die_ref die = dwarf2out_decl (decl);
+      if (die)
+	die->dumped_early = true;
       if (TREE_CODE (decl) == FUNCTION_DECL)
 	{
 	  pop_cfun ();
@@ -21162,7 +21187,7 @@  dwarf2out_early_decl (tree decl)
 	}
     }
  early_decl_exit:
-  cgraph_global_info_ready = save;
+  symtab->global_info_ready = save;
   return;
 }