diff mbox

Fix debug info of nested inline functions

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

Commit Message

Eric Botcazou March 2, 2012, 8:29 p.m. UTC 
Hi Jason,

you may remember a patch I posted and over which we exchanged a few messages:
  first message: http://gcc.gnu.org/ml/gcc-patches/2010-07/msg02143.html
  last message: http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01289.html
I eventually dropped the ball and nothing was installed, although we had almost 
reached an agreement.

The problem is still present as of today on the mainline, so I think now is a 
good time to solve it once for all.  We were disagreeing on the last hunk of 
the latest revision of the patch:
  http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01286.html
and you suggested to iterate over DECL_CONTEXT instead of die_parent to find an 
appropriate parent in order to attach the DIE on the limbo list to.

I confirm that we need to iterate, as the immediate DECL_CONTEXT is an abtract 
instance for the testcase.  The attached patch implements this and generates 
exactly the same debug info for the testcase as the original patch.

Tested on x86_64-suse-linux, OK for the mainline?


2012-03-02  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): Skip an abtract parent instance and iterate over
	the context to find the first non-abstract parent instance to attach
	concrete instances on the limbo list to it.

Comments

Jason Merrill May 12, 2012, 2:03 a.m. UTC | #1 
On 03/02/2012 03:29 PM, Eric Botcazou wrote:

I notice that D.7 seems to suggest that if the nested function is not 
inlinable and shared between all instances of the containing function 
that we put a normal (non-abstract/concrete) instance of the nested 
function inside the abstract function for the containing function.  But 
I agree that it's cleaner your way: put an abstract instance there 
instead so that the abstract instance of the containing function is all 
abstract.

> +  /* 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));

Hmm, why isn't current_function_decl == decl when we're trying to emit 
the abstract instance of a nested function?

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

Should "without actual instances" be something like "within concrete 
instances of containing functions"?

> -	  if (origin && origin->die_parent)
> +	  if (origin
> +	      && origin->die_parent
> +	      /* Skip an abtract parent instance.  */
> +	      && !(origin->die_parent->die_tag == DW_TAG_subprogram
> +		   && get_AT (origin->die_parent, DW_AT_inline)))
>  	    add_child_die (origin->die_parent, die);

What if the immediate parent is a DW_TAG_lexical_block or some other 
thing nested inside an abstract subprogram?

> and you suggested to iterate over DECL_CONTEXT instead of die_parent to find an
> appropriate parent in order to attach the DIE on the limbo list to.

In my earlier comments I seem to have been wrong about the behavior of 
gen_subprogram_die; now I see that if there is an abstract instance the 
concrete out-of-line instance is not associated with the decl number. 
So I guess your earlier limbo handling code was fine apart from the 
lexical_block issue above.

Jason
Eric Botcazou May 14, 2012, 3:54 p.m. UTC | #2 
> I notice that D.7 seems to suggest that if the nested function is not
> inlinable and shared between all instances of the containing function
> that we put a normal (non-abstract/concrete) instance of the nested
> function inside the abstract function for the containing function.  But
> I agree that it's cleaner your way: put an abstract instance there
> instead so that the abstract instance of the containing function is all
> abstract.

Hum, yes, D.7 even tends to show that what GCC currently generates is as 
expected.  The problem arises when the containing function is itself inlined 
into the nested function: in this case, we have the cycle in the debug info:

  |->  AI (containing) -> OL (nested) -> CI (containing) --|
  |                                                        |
  ------------------DW_AT_abstract_origin ------------------

that is problematic for GDB, and Joel said at the time that there was no easy 
way to break the recursion in GDB.

AI: Abstract Instance
OL: Out-of-Line
CI: Concrete Instance

But if you think the patch is nevertheless reasonable, then let's go ahead.

> > +  /* 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));
>
> Hmm, why isn't current_function_decl == decl when we're trying to emit
> the abstract instance of a nested function?

Because it is emitted when the first instance of the parent function is seen, 
and in this case current_function_decl == parent_decl.

> > +  /* Do not emit concrete instances of abstracted nested functions
> > without +     actual instances.  */
> > +  else if (TREE_CODE (decl_or_origin) == FUNCTION_DECL
> > +	   && die
> > +	   && get_AT (die, DW_AT_inline))
> > +    ;
>
> Should "without actual instances" be something like "within concrete
> instances of containing functions"?

Yes, that's probably clearer.

> > -	  if (origin && origin->die_parent)
> > +	  if (origin
> > +	      && origin->die_parent
> > +	      /* Skip an abtract parent instance.  */
> > +	      && !(origin->die_parent->die_tag == DW_TAG_subprogram
> > +		   && get_AT (origin->die_parent, DW_AT_inline)))
> >  	    add_child_die (origin->die_parent, die);
>
> What if the immediate parent is a DW_TAG_lexical_block or some other
> thing nested inside an abstract subprogram?

Indeed, there was a loop in the last revised version of the original thread:
  http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01286.html

Not clear why I dropped it...

> In my earlier comments I seem to have been wrong about the behavior of
> gen_subprogram_die; now I see that if there is an abstract instance the
> concrete out-of-line instance is not associated with the decl number.
> So I guess your earlier limbo handling code was fine apart from the
> lexical_block issue above.

So the above last revised version is OK with you, modulo the comment in 
process_scope_var?
Jason Merrill May 14, 2012, 4:49 p.m. UTC | #3 
On 05/14/2012 11:54 AM, Eric Botcazou wrote:
>> Hmm, why isn't current_function_decl == decl when we're trying to emit
>> the abstract instance of a nested function?
>
> Because it is emitted when the first instance of the parent function is seen,
> and in this case current_function_decl == parent_decl.

Our normal procedure is to generate a declaration when we see a function 
in its enclosing context, and then fix it up later when we see the 
definition.  Why not handle this similarly?

Jason
Jason Merrill May 14, 2012, 4:52 p.m. UTC | #4 
On 05/14/2012 12:49 PM, Jason Merrill wrote:
> On 05/14/2012 11:54 AM, Eric Botcazou wrote:
>>> Hmm, why isn't current_function_decl == decl when we're trying to emit
>>> the abstract instance of a nested function?
>>
>> Because it is emitted when the first instance of the parent function
>> is seen,
>> and in this case current_function_decl == parent_decl.
>
> Our normal procedure is to generate a declaration when we see a function
> in its enclosing context, and then fix it up later when we see the
> definition. Why not handle this similarly?

I suppose the way we handle nested functions, we generate debug info for 
the nested function before that for the enclosing function, but then we 
should attach the (abstract) nested function to the enclosing function 
in process_scope_var.

Jason
Eric Botcazou May 14, 2012, 8:17 p.m. UTC | #5 
> Our normal procedure is to generate a declaration when we see a function
> in its enclosing context, and then fix it up later when we see the
> definition.  Why not handle this similarly?

Because we want to generate an abstract instance of the nested function within 
the abstract instance of the parent function.  If we wait for the definition of 
the nested function, and it's out-of-line, we attach the out-of-line instance 
to the abstract parent, which is the source of the problem.
Jason Merrill May 14, 2012, 9:04 p.m. UTC | #6 
On 05/14/2012 04:17 PM, Eric Botcazou wrote:
>> Our normal procedure is to generate a declaration when we see a function
>> in its enclosing context, and then fix it up later when we see the
>> definition.  Why not handle this similarly?
>
> Because we want to generate an abstract instance of the nested function within
> the abstract instance of the parent function.  If we wait for the definition of
> the nested function, and it's out-of-line, we attach the out-of-line instance
> to the abstract parent, which is the source of the problem.

Right, and that's why we want your change to split the nested function 
into abstract and concrete instances.  But then it should be fine to 
attach the abstract instance to the abstract parent normally, I would think.

Jason
diff mbox

Patch

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 184668)
+++ dwarf2out.c	(working copy)
@@ -17173,7 +17173,13 @@  gen_subprogram_die (tree decl, dw_die_re
   dw_die_ref subr_die;
   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) != NULL_TREE
+			   && DECL_ABSTRACT (decl)
+			   && current_function_decl
+			   && DECL_ABSTRACT (current_function_decl)))
 		     || class_or_namespace_scope_p (context_die));
 
   premark_used_types ();
@@ -18198,6 +18204,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
@@ -19158,8 +19166,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)
 {
@@ -19177,8 +19202,15 @@  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
+	   && die
+	   && get_AT (die, DW_AT_inline))
+    ;
   else
     gen_decl_die (decl, origin, context_die);
 }
@@ -19525,11 +19557,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.  */
@@ -22526,7 +22558,11 @@  dwarf2out_finish (const char *filename)
 	{
 	  dw_die_ref origin = get_AT_ref (die, DW_AT_abstract_origin);
 
-	  if (origin && origin->die_parent)
+	  if (origin
+	      && origin->die_parent
+	      /* Skip an abtract parent instance.  */
+	      && !(origin->die_parent->die_tag == DW_TAG_subprogram
+		   && get_AT (origin->die_parent, DW_AT_inline)))
 	    add_child_die (origin->die_parent, die);
 	  else if (is_cu_die (die))
 	    ;
@@ -22545,16 +22581,22 @@  dwarf2out_finish (const char *filename)
 		 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.  */
-	      tree context = NULL_TREE;
-
-	      gcc_assert (node->created_for);
+	      tree context = node->created_for;
+	      gcc_assert (context);
 
-	      if (DECL_P (node->created_for))
-		context = DECL_CONTEXT (node->created_for);
-	      else if (TYPE_P (node->created_for))
-		context = TYPE_CONTEXT (node->created_for);
+	      /* Find the first non-abstract parent instance.  */
+	      do {
+		if (DECL_P (context))
+		  context = DECL_CONTEXT (context);
+		else if (TYPE_P (context))
+		  context = TYPE_CONTEXT (context);
+		else
+		  context = NULL_TREE;
+		origin = get_context_die (context);
+	      } while (origin
+		       && origin->die_tag == DW_TAG_subprogram
+		       && get_AT (origin, DW_AT_inline));
 
-	      origin = get_context_die (context);
 	      add_child_die (origin, die);
 	    }
 	}