Patchwork Fix debug info of nested inline functions

login
register
mail settings
Submitter Eric Botcazou
Date May 16, 2012, 9:29 p.m.
Message ID <201205162329.04339.ebotcazou@adacore.com>
Download mbox | patch
Permalink /patch/159762/
State New
Headers show

Comments

Eric Botcazou - May 16, 2012, 9:29 p.m.
> 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.

Indeed, this works, but I need to use function_possibly_abstracted_p instead of 
cgraph_function_possibly_inlined_p in gen_subprogram_die to get DW_AT_inline.

Revised patch attached.  It still generates the same (fixed) debug info for the 
reduced testcase.  I'll do a full testing cycle if you're happy with it.


	* dwarf2out.c (function_possibly_abstracted_p): New static function.
	(gen_subprogram_die): Use it function_possibly_abstracted_p in lieu of
	cgraph_function_possibly_inlined_p.
	(gen_inlined_subroutine_die): Return if the origin is to be ignored.
	(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.
Jason Merrill - May 18, 2012, 8:01 p.m.
On 05/16/2012 05:29 PM, Eric Botcazou wrote:
> -	  if (cgraph_function_possibly_inlined_p (decl))
> +	  if (function_possibly_abstracted_p (decl))
>   	    add_AT_unsigned (subr_die, DW_AT_inline, DW_INL_declared_inlined);
>   	  else
>   	    add_AT_unsigned (subr_die, DW_AT_inline, DW_INL_declared_not_inlined);
>   	}
>         else
>   	{
> -	  if (cgraph_function_possibly_inlined_p (decl))
> +	  if (function_possibly_abstracted_p (decl))

Why do you need this change?  As long as we're setting DW_AT_inline, it 
shouldn't matter what its value is.

> -	  if (origin && origin->die_parent)
> -	    add_child_die (origin->die_parent, die);
> +	  if (origin)
> +	    {
> +	      /* 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);
> +	    }

If we are looking at the DIE for something from a function in non-unit 
scope, this will return comp_unit_die() where previously it would have 
returned the immediate scope of the function, which might be something 
like a namespace/module or type.

Jason
Eric Botcazou - May 18, 2012, 8:48 p.m.
> Why do you need this change?  As long as we're setting DW_AT_inline, it
> shouldn't matter what its value is.

It's 0 for the nested function without it.

> If we are looking at the DIE for something from a function in non-unit
> scope, this will return comp_unit_die() where previously it would have
> returned the immediate scope of the function, which might be something
> like a namespace/module or type.

Does it really matter for concrete instances of inline functions?
Jason Merrill - May 18, 2012, 9:34 p.m.
On 05/18/2012 04:48 PM, Eric Botcazou wrote:
>> Why do you need this change?  As long as we're setting DW_AT_inline, it
>> shouldn't matter what its value is.
>
> It's 0 for the nested function without it.

Ah, I thought that having DW_AT_inline of DW_INL_not_inlined was enough 
to mark it as an abstract instance, but it seems I was wrong.

>> If we are looking at the DIE for something from a function in non-unit
>> scope, this will return comp_unit_die() where previously it would have
>> returned the immediate scope of the function, which might be something
>> like a namespace/module or type.
>
> Does it really matter for concrete instances of inline functions?

Nope.  Are those the only things with an abstract origin that will end 
up on limbo?  If we're always going to attach them to comp_unit_die () 
anyway, we might as well do that without the loop.

Jason
Eric Botcazou - May 19, 2012, 8:40 a.m.
> > Does it really matter for concrete instances of inline functions?
>
> Nope.  Are those the only things with an abstract origin that will end
> up on limbo?

According to the head comment of the block, yes, but I can try and see what 
happens in real life.

> If we're always going to attach them to comp_unit_die () anyway, we might as
> well do that without the loop. 

We attach them to the first non-abstract parent function with the loop.  That's 
important for Ada because we rely on the static nesting of functions in the 
debug info to support up-level references in GDB (i.e. access to variables of 
parent functions from within nested functions).
Jason Merrill - May 19, 2012, 3:22 p.m.
On 05/19/2012 04:40 AM, Eric Botcazou wrote:
> We attach them to the first non-abstract parent function with the loop.  That's
> important for Ada because we rely on the static nesting of functions in the
> debug info to support up-level references in GDB (i.e. access to variables of
> parent functions from within nested functions).

Aha.  The patch is OK.

Jason
H.J. Lu - July 5, 2012, 2:02 p.m.
On Wed, May 16, 2012 at 2:29 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> 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.
>
> Indeed, this works, but I need to use function_possibly_abstracted_p instead of
> cgraph_function_possibly_inlined_p in gen_subprogram_die to get DW_AT_inline.
>
> Revised patch attached.  It still generates the same (fixed) debug info for the
> reduced testcase.  I'll do a full testing cycle if you're happy with it.
>
>
>         * dwarf2out.c (function_possibly_abstracted_p): New static function.
>         (gen_subprogram_die): Use it function_possibly_abstracted_p in lieu of
>         cgraph_function_possibly_inlined_p.
>         (gen_inlined_subroutine_die): Return if the origin is to be ignored.
>         (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.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53860
Eric Botcazou - July 6, 2012, 7:41 a.m.
> > Revised patch attached.  It still generates the same (fixed) debug info
> > for the reduced testcase.  I'll do a full testing cycle if you're happy
> > with it.
> >
> >
> >         * dwarf2out.c (function_possibly_abstracted_p): New static
> > function. (gen_subprogram_die): Use it function_possibly_abstracted_p in
> > lieu of cgraph_function_possibly_inlined_p.
> >         (gen_inlined_subroutine_die): Return if the origin is to be
> > ignored. (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.
>
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53860

I've reverted the patch, as I can imagine that it will cause other problems.

Patch

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 187533)
+++ dwarf2out.c	(working copy)
@@ -16586,6 +16586,22 @@  gen_call_site_die (tree decl, dw_die_ref
   return die;
 }
 
+/* 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;
+}
+
 /* Generate a DIE to represent a declared function (either file-scope or
    block-local).  */
 
@@ -16738,14 +16754,14 @@  gen_subprogram_die (tree decl, dw_die_re
     {
       if (DECL_DECLARED_INLINE_P (decl))
 	{
-	  if (cgraph_function_possibly_inlined_p (decl))
+	  if (function_possibly_abstracted_p (decl))
 	    add_AT_unsigned (subr_die, DW_AT_inline, DW_INL_declared_inlined);
 	  else
 	    add_AT_unsigned (subr_die, DW_AT_inline, DW_INL_declared_not_inlined);
 	}
       else
 	{
-	  if (cgraph_function_possibly_inlined_p (decl))
+	  if (function_possibly_abstracted_p (decl))
 	    add_AT_unsigned (subr_die, DW_AT_inline, DW_INL_inlined);
 	  else
 	    add_AT_unsigned (subr_die, DW_AT_inline, DW_INL_not_inlined);
@@ -17674,6 +17690,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
@@ -18615,6 +18633,7 @@  gen_block_die (tree stmt, dw_die_ref con
 
 /* 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)
 {
@@ -18632,8 +18651,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 within
+     concrete instances of parent functions.  */
+  else if (TREE_CODE (decl_or_origin) == FUNCTION_DECL
+	   && die
+	   && get_AT (die, DW_AT_inline))
+    ;
   else
     gen_decl_die (decl, origin, context_die);
 }
@@ -18980,11 +19006,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.  */
@@ -21982,8 +22008,19 @@  dwarf2out_finish (const char *filename)
 	{
 	  dw_die_ref origin = get_AT_ref (die, DW_AT_abstract_origin);
 
-	  if (origin && origin->die_parent)
-	    add_child_die (origin->die_parent, die);
+	  if (origin)
+	    {
+	      /* 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 (is_cu_die (die))
 	    ;
 	  else if (seen_error ())