[PATCH] Proper use of decl_function_context in dwar2out.c

[PATCH] Proper use of decl_function_context in dwar2out.c

[Martin Jambor]

Hi,

the following patch fixes an ICE for me when LTO building Firefox at
-O3 -g.  The problem is that at one spot we use decl_function_context
as a predicate whether to use TREE_CONTEXT rather than using it's
result which can be determined in a much more elaborate way.  In my
particular case TREE_CONTEXT is a type, not a function decl and
lookup_decl_die chokes on that, when it is fed the function result,
all seems fine.

Bootstrapped and tested on x86_64-linux.  OK for trunk?

Also, I have not actually tried it but I suppose we'll be hitting this
problem with 4.7 as well.  OK for the branch once 4.7.0 is out?

Thanks,

Martin
 

2012-03-07  Martin Jambor  <mjambor@suse.cz>

	* dwarf2out.c (dwarf2out_decl): Use result of decl_function_context
	rather than DECL_CONTEXT.

Index: src/gcc/dwarf2out.c
===================================================================
--- src.orig/gcc/dwarf2out.c
+++ src/gcc/dwarf2out.c
@@ -19830,6 +19830,7 @@ void
 dwarf2out_decl (tree decl)
 {
   dw_die_ref context_die = comp_unit_die ();
+  tree ctx_fndecl;
 
   switch (TREE_CODE (decl))
     {
@@ -19889,8 +19890,9 @@ dwarf2out_decl (tree decl)
 	return;
 
       /* For local statics lookup proper context die.  */
-      if (TREE_STATIC (decl) && decl_function_context (decl))
-	context_die = lookup_decl_die (DECL_CONTEXT (decl));
+      if (TREE_STATIC (decl) &&
+	  (ctx_fndecl = decl_function_context (decl)) != NULL_TREE)
+	context_die = lookup_decl_die (ctx_fndecl);
 
       /* If we are in terse mode, don't generate any DIEs to represent any
 	 variable declarations or definitions.  */

Re: [PATCH] Proper use of decl_function_context in dwar2out.c

[Jakub Jelinek]

On Thu, Mar 08, 2012 at 12:06:46PM +0100, Martin Jambor wrote:
>        /* For local statics lookup proper context die.  */
> -      if (TREE_STATIC (decl) && decl_function_context (decl))
> -	context_die = lookup_decl_die (DECL_CONTEXT (decl));
> +      if (TREE_STATIC (decl) &&
> +	  (ctx_fndecl = decl_function_context (decl)) != NULL_TREE)
> +	context_die = lookup_decl_die (ctx_fndecl);

The formatting is wrong, && shouldn't be at the end of line.
For the rest I'll defer to Jason, not sure what exactly we want to do there.
This hunk has been added by Honza:
2005-06-09  Jan Hubicka  <jh@suse.cz>

	* cgraphunit.c (cgraph_create_edges): Do not walk BLOCK; finalize
	local statics when doing unit-at-a-time.
	(cgraph_varpool_assemble_pending_decls): Output debug info.
	* dwarf2out.c (decls_for_scope): Skip local statics.
	(dwarf2out_decl): Handle local statics.
	* passes.c (rest_of_decl_compilation): Do not differentiate
	local and global statics in unit-at-a-time.
	* tree-inline.c (remap_decls): Put local static into
	unexpanded_vars_list rather than introducing duplicated VAR_DECL
	node.

	Jakub

Re: [PATCH] Proper use of decl_function_context in dwar2out.c

[Richard Guenther]

On Thu, Mar 8, 2012 at 12:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Mar 08, 2012 at 12:06:46PM +0100, Martin Jambor wrote:
>>        /* For local statics lookup proper context die.  */
>> -      if (TREE_STATIC (decl) && decl_function_context (decl))
>> -     context_die = lookup_decl_die (DECL_CONTEXT (decl));
>> +      if (TREE_STATIC (decl) &&
>> +       (ctx_fndecl = decl_function_context (decl)) != NULL_TREE)
>> +     context_die = lookup_decl_die (ctx_fndecl);
>
> The formatting is wrong, && shouldn't be at the end of line.
> For the rest I'll defer to Jason, not sure what exactly we want to do there.
> This hunk has been added by Honza:

I don't think the patch is right.  Suppose you have a function-local
class declaration with a static member.  Surely the context DIE
you want to use is still the class one, not the function one.

So, I'm not sure why we should not be able to create a testcase
that fails even without LTO.  I think using get_context_die here
would be more appropriate.  Or restrict this to DECL_CONTEXT (decl)
== FUNCTION_DECL.

Richard.

> 2005-06-09  Jan Hubicka  <jh@suse.cz>
>
>        * cgraphunit.c (cgraph_create_edges): Do not walk BLOCK; finalize
>        local statics when doing unit-at-a-time.
>        (cgraph_varpool_assemble_pending_decls): Output debug info.
>        * dwarf2out.c (decls_for_scope): Skip local statics.
>        (dwarf2out_decl): Handle local statics.
>        * passes.c (rest_of_decl_compilation): Do not differentiate
>        local and global statics in unit-at-a-time.
>        * tree-inline.c (remap_decls): Put local static into
>        unexpanded_vars_list rather than introducing duplicated VAR_DECL
>        node.
>
>        Jakub

Re: [PATCH] Proper use of decl_function_context in dwar2out.c

[Martin Jambor]

Hi,

On Mon, Mar 12, 2012 at 11:51:05AM +0100, Richard Guenther wrote:
> On Thu, Mar 8, 2012 at 12:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Thu, Mar 08, 2012 at 12:06:46PM +0100, Martin Jambor wrote:
> >>        /* For local statics lookup proper context die.  */
> >> -      if (TREE_STATIC (decl) && decl_function_context (decl))
> >> -     context_die = lookup_decl_die (DECL_CONTEXT (decl));
> >> +      if (TREE_STATIC (decl) &&
> >> +       (ctx_fndecl = decl_function_context (decl)) != NULL_TREE)
> >> +     context_die = lookup_decl_die (ctx_fndecl);
> >
> > The formatting is wrong, && shouldn't be at the end of line.
> > For the rest I'll defer to Jason, not sure what exactly we want to do there.
> > This hunk has been added by Honza:
> 
> I don't think the patch is right.  Suppose you have a function-local
> class declaration with a static member.  Surely the context DIE
> you want to use is still the class one, not the function one.

That is not what happens, though.  The problem is that we attempt to
generate debug info for VMTs of classes defined within functions.  I
have filed PR 52605 with a small testcase to track the issue.

I have not yet had a look at how this code treats (or should treat)
static members of classes defined within a function.

Thanks,

Martin

Re: [PATCH] Proper use of decl_function_context in dwar2out.c

[Martin Jambor]

Hi,

On Fri, Mar 16, 2012 at 05:14:38PM +0100, Martin Jambor wrote:
> On Mon, Mar 12, 2012 at 11:51:05AM +0100, Richard Guenther wrote:
> > On Thu, Mar 8, 2012 at 12:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > > On Thu, Mar 08, 2012 at 12:06:46PM +0100, Martin Jambor wrote:
> > >>        /* For local statics lookup proper context die.  */
> > >> -      if (TREE_STATIC (decl) && decl_function_context (decl))
> > >> -     context_die = lookup_decl_die (DECL_CONTEXT (decl));
> > >> +      if (TREE_STATIC (decl) &&
> > >> +       (ctx_fndecl = decl_function_context (decl)) != NULL_TREE)
> > >> +     context_die = lookup_decl_die (ctx_fndecl);
> > >
> > > The formatting is wrong, && shouldn't be at the end of line.
> > > For the rest I'll defer to Jason, not sure what exactly we want to do there.
> > > This hunk has been added by Honza:
> > 
> > I don't think the patch is right.  Suppose you have a function-local
> > class declaration with a static member.  Surely the context DIE
> > you want to use is still the class one, not the function one.
> 
> That is not what happens, though.  The problem is that we attempt to
> generate debug info for VMTs of classes defined within functions.  I
> have filed PR 52605 with a small testcase to track the issue.
> 
> I have not yet had a look at how this code treats (or should treat)
> static members of classes defined within a function.
> 

It turs out that local classes are not allowed to have static data
members.  GCC will compile such class with -fpermissive but then the
linker fails with an undefined reference because the data member is
not defined anywhere and I did not manage to figure out or google how
to define it.  Therefore I think this is not an issue.

It seems to me that dwarf2out_decl was intended to supply either
comp_unit_die or a function decl die as the context to gen_decl_die
(which detects memberr-ness by decl_class_context) and thus I still
tend to think my patch (with adjusted formatting) is a correct and
simple fix.  Nevertheless I'll be happy to learn more if I am wrong.

Martin

Re: [PATCH] Proper use of decl_function_context in dwar2out.c

[Martin Jambor]

Hi,

I'd like to drag some attention to this bug again, it is the only ICE
when LTO building Firefox with debug info and the problem also happens
with the 4.7 so it would be nice to have this fixed for 4.7.1.

On Mon, Mar 12, 2012 at 11:51:05AM +0100, Richard Guenther wrote:
> On Thu, Mar 8, 2012 at 12:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Thu, Mar 08, 2012 at 12:06:46PM +0100, Martin Jambor wrote:
> >>        /* For local statics lookup proper context die.  */
> >> -      if (TREE_STATIC (decl) && decl_function_context (decl))
> >> -     context_die = lookup_decl_die (DECL_CONTEXT (decl));
> >> +      if (TREE_STATIC (decl) &&
> >> +       (ctx_fndecl = decl_function_context (decl)) != NULL_TREE)
> >> +     context_die = lookup_decl_die (ctx_fndecl);
> >
> > The formatting is wrong, && shouldn't be at the end of line.
> > For the rest I'll defer to Jason, not sure what exactly we want to do there.
> > This hunk has been added by Honza:
> 
> I don't think the patch is right.  Suppose you have a function-local
> class declaration with a static member.  Surely the context DIE
> you want to use is still the class one, not the function one.
> 

Let me just briefly re-iterate what I have already written before.
The above cannot happen because function-local classes are not allowed
to have static members.  Also, the actual problem happens when we
attempt to generate debug info for a VMT of a class defined within a
function.  I don not think it really matters whether or what context
it gets...

> So, I'm not sure why we should not be able to create a testcase
> that fails even without LTO.  I think using get_context_die here
> would be more appropriate.  Or restrict this to DECL_CONTEXT (decl)
> == FUNCTION_DECL.

...and thus I am fine with this as well, which is what the patch below
does.  It now also has a testcase, LTO builds the testcase and I am
currently bootstrapping it and LTOing Firefox with it.

But I would be equally happy with my original patch, feeding
lookup_decl_die with the result of decl_function_context.

OK for trunk and the 4.7 branch?

Thanks,

Martin


2012-04-27  Martin Jambor  <mjambor@suse.cz>

	PR lto/53138
	* dwarf2out.c (dwarf2out_decl): Only lookup die representing context
	of a variable when the contect is a function.

	* gcc/testsuite/g++.dg/lto/pr52605_0.C: New test.


Index: src/gcc/dwarf2out.c
===================================================================
--- src.orig/gcc/dwarf2out.c
+++ src/gcc/dwarf2out.c
@@ -19880,7 +19880,9 @@ dwarf2out_decl (tree decl)
 	return;
 
       /* For local statics lookup proper context die.  */
-      if (TREE_STATIC (decl) && decl_function_context (decl))
+      if (TREE_STATIC (decl)
+	  && DECL_CONTEXT (decl)
+	  && TREE_CODE (DECL_CONTEXT (decl)) == FUNCTION_DECL)
 	context_die = lookup_decl_die (DECL_CONTEXT (decl));
 
       /* If we are in terse mode, don't generate any DIEs to represent any
Index: src/gcc/testsuite/g++.dg/lto/pr52605_0.C
===================================================================
--- /dev/null
+++ src/gcc/testsuite/g++.dg/lto/pr52605_0.C
@@ -0,0 +1,39 @@
+// { dg-lto-do link }
+// { dg-lto-options {{-flto -g}} }
+
+extern "C" void abort (void);
+
+class A
+{
+public:
+  virtual int foo (int i);
+};
+
+int A::foo (int i)
+{
+  return i + 1;
+}
+
+int __attribute__ ((noinline,noclone)) get_input(void)
+{
+  return 1;
+}
+
+int main (int argc, char *argv[])
+{
+
+  class B : public A
+  {
+  public:
+    int bar (int i)
+    {
+      return foo (i) + 2;
+    }
+  };
+  class B b;
+
+  if (b.bar (get_input ()) != 4)
+    abort ();
+  return 0;
+}
+

Re: [PATCH] Proper use of decl_function_context in dwar2out.c

[Jason Merrill]

On 04/27/2012 07:56 AM, Martin Jambor wrote:
> 	PR lto/53138
> 	* dwarf2out.c (dwarf2out_decl): Only lookup die representing context
> 	of a variable when the contect is a function.

OK.

Jason