diff mbox

Fix debug info of nested inline functions

Message ID 201007280822.00800.ebotcazou@adacore.com
State New
Headers show

Commit Message

Eric Botcazou July 28, 2010, 6:22 a.m. UTC 
Hi,

Compile the attached testcase with -O3 -g -dA (no need to know Ada, it's 
C-like code) and you'll see that GCC generates broken debug info for nested 
inline functions.

More specifically, Iterate_Corresponding_Subcomponents_Rec is inlined into its 
parent Assign_Subcomponents but its child Walk_One_LHS_Component isn't.   The 
first problem is that, in the debug info, the abstract instance of I_C_S_B 
contains the out-of-line instance of W_O_L_C.

Moreover, Iterate_Corresponding_Subcomponents_Rec is also inlined into its 
child Walk_One_LHS_Component.  The out-of-line instance of W_O_L_C correctly 
contains a concrete instance of I_C_S_B but, because of the above, we have a 
cycle in the debug info through the DW_AT_abstract_origin of the latter.

The attached patch is a (partial) attempt at implementing 3.3.8.4 of the DWARF 
standard and avoiding cycles.  It makes it so that abstract instances of 
functions contain a full tree of abstract instances of their child functions; 
it also makes sure that concrete or out-of-line instances aren't contained in 
abstract instances.  I think this implements 3.3.8.4 up to 1., 2. and 3; as 
for 4., it isn't (always) fulfilled but this doesn't seem to be a problem in 
practice for GDB.

Tested on x86-64-suse-linux (GCC and GDB), OK for mainline?


2010-07-28  Eric Botcazou  <ebotcazou@adacore.com>

	* dwarf2out.c (gen_subprogram_die): Emit a definition of nested
	functions within an abstract instance of their parent.
	(gen_inlined_subroutine_die): Return if the origin is to be ignored.
	(function_possibly_abstracted_p): New static function.
	(process_scope_var): Do not emit concrete instances of abstracted
	nested functions from here.
	(gen_decl_die): Emit the abstract instance if the function is possibly
	abstracted and not only possibly inlined.
	(dwarf2out_finish): Find the first non-abstract parent instance and
	attach concrete instances on the limbo list to it.

Comments

Jason Merrill Sept. 8, 2010, 9:57 p.m. UTC | #1 
On 07/28/2010 02:22 AM, Eric Botcazou wrote:
> +  int declaration = ((decl != current_function_decl
> +		      && !(DECL_INITIAL (decl)
> +			   && current_function_decl
> +			   && DECL_ABSTRACT (current_function_decl)))

This doesn't seem to depend on decl being nested in 
current_function_decl, so I'm concerned about it getting the wrong 
answer if we somehow call gen_subprogram_die for a non-nested function.

Does it work to check DECL_ABSTRACT (decl) rather than DECL_ABSTRACT 
(current_function_decl)?

> +  /* Do not emit concrete instances of abstracted nested functions without
> +     actual instances.  */
> +  else if (TREE_CODE (decl_or_origin) == FUNCTION_DECL
> +	   && ((!DECL_ABSTRACT (decl_or_origin)
> +		&& function_possibly_abstracted_p (decl_or_origin))
> +	       || (die && get_AT (die, DW_AT_inline))))
> +    ;

This seems wrong.  We do want to emit concrete instances of abstracted 
nested functions, just not within the abstract instance of the enclosing 
function.

>  	  if (origin)
> -	    add_child_die (origin->die_parent, die);
> +	    {
> +	      /* Find the first non-abstract parent instance.  */
> +	      do
> +		origin = origin->die_parent;
> +	      while (origin
> +		     && (origin->die_tag != DW_TAG_subprogram
> +			 || get_AT (origin, DW_AT_inline)));
> +	      if (origin)
> +		add_child_die (origin, die);
> +	      else
> +		add_child_die (comp_unit_die, die);
> +	    }

Here, shouldn't the parent of the concrete instance of the nested 
function be the concrete instance of the enclosing function?

Jason
Eric Botcazou Sept. 10, 2010, 3:54 p.m. UTC | #2 
> > +  int declaration = ((decl != current_function_decl
> > +		      && !(DECL_INITIAL (decl)
> > +			   && current_function_decl
> > +			   && DECL_ABSTRACT (current_function_decl)))
>
> This doesn't seem to depend on decl being nested in
> current_function_decl, so I'm concerned about it getting the wrong
> answer if we somehow call gen_subprogram_die for a non-nested function.
>
> Does it work to check DECL_ABSTRACT (decl) rather than DECL_ABSTRACT
> (current_function_decl)?

We can test the flag on both DECLs to match the comment:

  /* Emit an abstract instance of nested functions within an abstract instance
     of their parent.  */
  int declaration = ((decl != current_function_decl
		      && !(DECL_INITIAL (decl) != NULL_TREE
			   && DECL_ABSTRACT (decl)
			   && current_function_decl
			   && DECL_ABSTRACT (current_function_decl)))
		     || class_or_namespace_scope_p (context_die));

> > +  /* Do not emit concrete instances of abstracted nested functions
> > without +     actual instances.  */
> > +  else if (TREE_CODE (decl_or_origin) == FUNCTION_DECL
> > +	   && ((!DECL_ABSTRACT (decl_or_origin)
> > +		&& function_possibly_abstracted_p (decl_or_origin))
> > +	       || (die && get_AT (die, DW_AT_inline))))
> > +    ;
>
> This seems wrong.  We do want to emit concrete instances of abstracted
> nested functions, just not within the abstract instance of the enclosing
> function.

This hunk is necessary to prevent abstracted nested functions from being 
emitted twice in the enclosing function if they are inlined there, as a 
concrete inline and as a concrete out-of-line instance.  We wait until after 
actual instances, either inline or out-of-line, are encountered and attach 
the DIEs in dwarf2out_finish.

> >  	  if (origin)
> > -	    add_child_die (origin->die_parent, die);
> > +	    {
> > +	      /* Find the first non-abstract parent instance.  */
> > +	      do
> > +		origin = origin->die_parent;
> > +	      while (origin
> > +		     && (origin->die_tag != DW_TAG_subprogram
> > +			 || get_AT (origin, DW_AT_inline)));
> > +	      if (origin)
> > +		add_child_die (origin, die);
> > +	      else
> > +		add_child_die (comp_unit_die, die);
> > +	    }
>
> Here, shouldn't the parent of the concrete instance of the nested
> function be the concrete instance of the enclosing function?

Yes, the not always fulfilled 4. I mentioned in the message.  How would you do 
that?  If you look up the DIE of the enclosing function, you get the abstract 
instance.  The parent of concrete instances doesn't seem to really matter to 
GDB as long as it isn't abstract.
Jason Merrill Sept. 10, 2010, 5:36 p.m. UTC | #3 
On 09/10/2010 11:54 AM, Eric Botcazou wrote:
> We can test the flag on both DECLs to match the comment:
>
>    /* Emit an abstract instance of nested functions within an abstract instance
>       of their parent.  */
>    int declaration = ((decl != current_function_decl
> 		&&  !(DECL_INITIAL (decl) != NULL_TREE
> 			&&  DECL_ABSTRACT (decl)
> 			&&  current_function_decl
> 			&&  DECL_ABSTRACT (current_function_decl)))
> 		     || class_or_namespace_scope_p (context_die));

OK.  I would expect DECL_ABSTRACT (current_function_decl) to be set 
whenever DECL_ABSTRACT (decl) is set, but I guess it doesn't hurt to 
check both.

>>> +  /* Do not emit concrete instances of abstracted nested functions without
>>> +     actual instances.  */
>>> +  else if (TREE_CODE (decl_or_origin) == FUNCTION_DECL
>>> +       && ((!DECL_ABSTRACT (decl_or_origin)
>>> +        && function_possibly_abstracted_p (decl_or_origin))
>>> +           || (die && get_AT (die, DW_AT_inline))))
>>> +    ;
>>
>> This seems wrong.  We do want to emit concrete instances of abstracted
>> nested functions, just not within the abstract instance of the enclosing
>> function.
>
> This hunk is necessary to prevent abstracted nested functions from being
> emitted twice in the enclosing function if they are inlined there, as a
> concrete inline and as a concrete out-of-line instance.  We wait until after
> actual instances, either inline or out-of-line, are encountered and attach
> the DIEs in dwarf2out_finish.

OK, I see.

>>>   	if (origin)
>>> -	    add_child_die (origin->die_parent, die);
>>> +	    {
>>> +	      /* Find the first non-abstract parent instance.  */
>>> +	      do
>>> +		origin = origin->die_parent;
>>> +	      while (origin
>>> +		&&  (origin->die_tag != DW_TAG_subprogram
>>> +			 || get_AT (origin, DW_AT_inline)));
>>> +	      if (origin)
>>> +		add_child_die (origin, die);
>>> +	      else
>>> +		add_child_die (comp_unit_die, die);
>>> +	    }
>>
>> Here, shouldn't the parent of the concrete instance of the nested
>> function be the concrete instance of the enclosing function?
>
> Yes, the not always fulfilled 4. I mentioned in the message.  How would you do
> that?  If you look up the DIE of the enclosing function, you get the abstract
> instance.  The parent of concrete instances doesn't seem to really matter to
> GDB as long as it isn't abstract.

It seems like the later block that checks DECL_CONTEXT 
(node->created_for) should handle the case of a concrete non-inlined 
instance.

Jason
diff mbox

Patch

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 162566)
+++ dwarf2out.c	(working copy)
@@ -18570,7 +18570,12 @@  gen_subprogram_die (tree decl, dw_die_re
   tree fn_arg_types;
   tree outer_scope;
   dw_die_ref old_die = lookup_decl_die (decl);
-  int declaration = (current_function_decl != decl
+  /* Emit an abstract instance of nested functions within an abstract instance
+     of their parent.  */
+  int declaration = ((decl != current_function_decl
+		      && !(DECL_INITIAL (decl)
+			   && current_function_decl
+			   && DECL_ABSTRACT (current_function_decl)))
 		     || class_or_namespace_scope_p (context_die));
 
   premark_used_types ();
@@ -19352,6 +19357,8 @@  gen_inlined_subroutine_die (tree stmt, d
   gcc_assert (! BLOCK_ABSTRACT (stmt));
 
   decl = block_ultimate_origin (stmt);
+  if (DECL_IGNORED_P (decl))
+    return;
 
   /* Emit info for the abstract instance first, if we haven't yet.  We
      must emit this even if the block is abstract, otherwise when we
@@ -20128,8 +20135,25 @@  gen_block_die (tree stmt, dw_die_ref con
     decls_for_scope (stmt, context_die, depth);
 }
 
+/* Return true if an abstract instance of function DECL can be generated in
+   the debug information.  */
+
+static bool
+function_possibly_abstracted_p (tree decl)
+{
+  while (decl)
+    {
+      if (cgraph_function_possibly_inlined_p (decl))
+	return true;
+      decl = decl_function_context (decl);
+    }
+
+  return false;
+}
+
 /* Process variable DECL (or variable with origin ORIGIN) within
    block STMT and add it to CONTEXT_DIE.  */
+
 static void
 process_scope_var (tree stmt, tree decl, tree origin, dw_die_ref context_die)
 {
@@ -20147,8 +20171,16 @@  process_scope_var (tree stmt, tree decl,
   if (die != NULL && die->die_parent == NULL)
     add_child_die (context_die, die);
   else if (TREE_CODE (decl_or_origin) == IMPORTED_DECL)
-    dwarf2out_imported_module_or_decl_1 (decl_or_origin, DECL_NAME (decl_or_origin),
+    dwarf2out_imported_module_or_decl_1 (decl_or_origin,
+					 DECL_NAME (decl_or_origin),
 					 stmt, context_die);
+  /* Do not emit concrete instances of abstracted nested functions without
+     actual instances.  */
+  else if (TREE_CODE (decl_or_origin) == FUNCTION_DECL
+	   && ((!DECL_ABSTRACT (decl_or_origin)
+		&& function_possibly_abstracted_p (decl_or_origin))
+	       || (die && get_AT (die, DW_AT_inline))))
+    ;
   else
     gen_decl_die (decl, origin, context_die);
 }
@@ -20486,11 +20518,11 @@  gen_decl_die (tree decl, tree origin, dw
 				     ? DECL_ORIGIN (origin)
 				     : DECL_ABSTRACT_ORIGIN (decl));
 
-      /* If we're emitting an out-of-line copy of an inline function,
+      /* If we're emitting an out-of-line copy of an abstracted function,
 	 emit info for the abstract instance and set up to refer to it.  */
-      else if (cgraph_function_possibly_inlined_p (decl)
-	       && ! DECL_ABSTRACT (decl)
-	       && ! class_or_namespace_scope_p (context_die)
+      else if (!DECL_ABSTRACT (decl)
+	       && function_possibly_abstracted_p (decl)
+	       && !class_or_namespace_scope_p (context_die)
 	       /* dwarf2out_abstract_function won't emit a die if this is just
 		  a declaration.  We must avoid setting DECL_ABSTRACT_ORIGIN in
 		  that case, because that works only if we have a die.  */
@@ -22253,7 +22285,18 @@  dwarf2out_finish (const char *filename)
 	  dw_die_ref origin = get_AT_ref (die, DW_AT_abstract_origin);
 
 	  if (origin)
-	    add_child_die (origin->die_parent, die);
+	    {
+	      /* Find the first non-abstract parent instance.  */
+	      do
+		origin = origin->die_parent;
+	      while (origin
+		     && (origin->die_tag != DW_TAG_subprogram
+			 || get_AT (origin, DW_AT_inline)));
+	      if (origin)
+		add_child_die (origin, die);
+	      else
+		add_child_die (comp_unit_die, die);
+	    }
 	  else if (die == comp_unit_die)
 	    ;
 	  else if (seen_error ())