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

[Martin Jambor <mjambor at suse dot cz>]

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;
+}
+