Re: [PATCH, PR 45699] Devirtualize to thunks

Re: [PATCH, PR 45699] Devirtualize to thunks

[Benjamin Redelings I]

Jan Hubicka wrote:
> >  Hi,
> >
> >  folding of OBJ_TYPE_REFs just takes the function declaration in BINFOs
> >  and puts into the call statement.  Unfortunately BINFOs do not put the
> >  declaration of the proper thunk there and so we might ending up not
> >  adjusting the this pointer like in the testcase below.  On the other
> >  hand, BINFOs do contain the deltas and so the folding code can look up
> >  the right thunk in the call graph if need be.  This is what the patch
> >  below does.
Thank you!  That's great.  I can now test gcc 4.6 at -O and -O2... O3 
still gives segfaults.

> >
> >  Bootstrapped and tested on x86_64-linux without any issues.  OK for
> >  trunk?
>
> I guess we should also add an folder that transforms calls to thunk to call to
> the function so inlining and other IPA stuff work?
> At the moment i think both ipa-prop and inliner will get direct calls to thunks wrong.
Should I submit a bugzilla bug for this, as is, or do you need a testcase?
(I'm sure I can get one by whittling down my code, but last time it took 
about 4 hours to do this, so if there's a faster way ... :-P)

-BenRI

Re: [PATCH, PR 45699] Devirtualize to thunks

[Martin Jambor]

Hi,

On Tue, Oct 12, 2010 at 04:22:15PM -0400, Benjamin Redelings I wrote:
> Jan Hubicka wrote:
> >>  Hi,
> >>
> >>  folding of OBJ_TYPE_REFs just takes the function declaration in BINFOs
> >>  and puts into the call statement.  Unfortunately BINFOs do not put the
> >>  declaration of the proper thunk there and so we might ending up not
> >>  adjusting the this pointer like in the testcase below.  On the other
> >>  hand, BINFOs do contain the deltas and so the folding code can look up
> >>  the right thunk in the call graph if need be.  This is what the patch
> >>  below does.
> Thank you!  That's great.  I can now test gcc 4.6 at -O and -O2...
> O3 still gives segfaults.

No worries.  Thanks for testing.

> 
> >>
> >>  Bootstrapped and tested on x86_64-linux without any issues.  OK for
> >>  trunk?
> >
> >I guess we should also add an folder that transforms calls to thunk to call to
> >the function so inlining and other IPA stuff work?
> >At the moment i think both ipa-prop and inliner will get direct calls to thunks wrong.
> Should I submit a bugzilla bug for this, as is, or do you need a testcase?
> (I'm sure I can get one by whittling down my code, but last time it
> took about 4 hours to do this, so if there's a faster way ... :-P)

Do you mean the segfaults you are experiencing?  Yes, please file a
bug for them.  If not, then we will remember to revisit this
ourselves.

Thanks,

Martin

pure virtual method called

[Benjamin Redelings]

Hi Jan,

     I hope to report a bug soon, but in the meantime I wanted to let 
you know that for the last month or so, the 4.9 branch has (I think) a 
bug at O3, where my program gets:

pure virtual method called
terminate called without an active exception
Aborted

4.8 works fine.

     I am guessing this is related to your devirt work.  I haven't been 
able to produce a reduce testcase yet (sorry!), but here is some code 
that illustrates the C++ type for the object whose virtual table is (?) 
messed up.  (Note: this code compiles but does NOT crash.)

     Again, sorry for not having a testcase.  I'll make one soon. 
Hopefully it is helpful to know that bugs still exist.

-BenRI

#include <vector>

class Object
{
   virtual Object* clone() const =0;
};

template <typename T>
class Box: public Object, public T
{
public:
   Box<T>* clone() const {return new Box<T>(*this);}
};

template <typename T>
using Vector = Box<std::vector<T>>;

int main()
{
   Vector<int> v;
   v.clone();
}

-BenRI

P.S. The bug exists in debian gcc snapshots taken on 2014-02-12 and 
2014-01-22, Linux AMD64.

Re: pure virtual method called

[Jan Hubicka]

Hi,
the testcase would be wonderful - those bugs are hard to catch. I fixed some issues
recently, so you may try recent snapshot if you didn't.
You may try -fno-devirtualize to see if the bug goes away (likely it will) and you
may try to look in -fdump-tree-all -fdump-ipa-all dumps where cxa_pure_virtual call
appears in the program and send me some context.

Honza
> Hi Jan,
> 
>     I hope to report a bug soon, but in the meantime I wanted to let
> you know that for the last month or so, the 4.9 branch has (I think)
> a bug at O3, where my program gets:
> 
> pure virtual method called
> terminate called without an active exception
> Aborted
> 
> 4.8 works fine.
> 
>     I am guessing this is related to your devirt work.  I haven't
> been able to produce a reduce testcase yet (sorry!), but here is
> some code that illustrates the C++ type for the object whose virtual
> table is (?) messed up.  (Note: this code compiles but does NOT
> crash.)
> 
>     Again, sorry for not having a testcase.  I'll make one soon.
> Hopefully it is helpful to know that bugs still exist.
> 
> -BenRI
> 
> #include <vector>
> 
> class Object
> {
>   virtual Object* clone() const =0;
> };
> 
> template <typename T>
> class Box: public Object, public T
> {
> public:
>   Box<T>* clone() const {return new Box<T>(*this);}
> };
> 
> template <typename T>
> using Vector = Box<std::vector<T>>;
> 
> int main()
> {
>   Vector<int> v;
>   v.clone();
> }
> 
> -BenRI
> 
> P.S. The bug exists in debian gcc snapshots taken on 2014-02-12 and
> 2014-01-22, Linux AMD64.

Re: pure virtual method called

[Benjamin Redelings]

On 02/14/2014 03:21 PM, Jan Hubicka wrote:
> Hi,
> the testcase would be wonderful - those bugs are hard to catch.
Yeah - hope to soon.
>   I fixed some issues
> recently, so you may try recent snapshot if you didn't.
> You may try -fno-devirtualize to see if the bug goes away (likely it will) and you
> may try to look in -fdump-tree-all -fdump-ipa-all dumps where cxa_pure_virtual call
> appears in the program and send me some context.
Thanks!
-BenRI

Re: [PATCH, PR 45699] Devirtualize to thunks

[Jan Hubicka]

> Hi,
> 
> folding of OBJ_TYPE_REFs just takes the function declaration in BINFOs
> and puts into the call statement.  Unfortunately BINFOs do not put the
> declaration of the proper thunk there and so we might ending up not
> adjusting the this pointer like in the testcase below.  On the other
> hand, BINFOs do contain the deltas and so the folding code can look up
> the right thunk in the call graph if need be.  This is what the patch
> below does.
> 
> Bootstrapped and tested on x86_64-linux without any issues.  OK for
> trunk?

I guess we should also add an folder that transforms calls to thunk to call to
the function so inlining and other IPA stuff work?
At the moment i think both ipa-prop and inliner will get direct calls to thunks wrong.

Honza

Re: [PATCH, PR 45699] Devirtualize to thunks

[Richard Guenther]

On Mon, 11 Oct 2010, Martin Jambor wrote:

> Hi,
> 
> folding of OBJ_TYPE_REFs just takes the function declaration in BINFOs
> and puts into the call statement.  Unfortunately BINFOs do not put the
> declaration of the proper thunk there and so we might ending up not
> adjusting the this pointer like in the testcase below.  On the other
> hand, BINFOs do contain the deltas and so the folding code can look up
> the right thunk in the call graph if need be.  This is what the patch
> below does.
> 
> Bootstrapped and tested on x86_64-linux without any issues.  OK for
> trunk?

Ok.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 2010-10-08  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR middle-end/45699
> 	* gimple-fold.c (gimple_fold_obj_type_ref_known_binfo): Choose among
> 	thunks.
> 
> 	* testsuite/g++.dg/torture/pr45699.C: New test.
> 	* testsuite/g++.dg/otr-fold-1.C: Adjusted.
> 	* testsuite/g++.dg/otr-fold-1.C: Likewise.
> 
> 
> Index: icln/gcc/gimple-fold.c
> ===================================================================
> --- icln.orig/gcc/gimple-fold.c
> +++ icln/gcc/gimple-fold.c
> @@ -1463,7 +1463,7 @@ tree
>  gimple_fold_obj_type_ref_known_binfo (HOST_WIDE_INT token, tree known_binfo)
>  {
>    HOST_WIDE_INT i;
> -  tree v, fndecl;
> +  tree v, fndecl, delta;
>  
>    v = BINFO_VIRTUALS (known_binfo);
>    i = 0;
> @@ -1475,6 +1475,25 @@ gimple_fold_obj_type_ref_known_binfo (HO
>      }
>  
>    fndecl = TREE_VALUE (v);
> +  delta = TREE_PURPOSE (v);
> +  gcc_assert (host_integerp (delta, 0));
> +
> +  if (integer_nonzerop (delta))
> +    {
> +      struct cgraph_node *node = cgraph_get_node (fndecl);
> +      HOST_WIDE_INT off = tree_low_cst (delta, 0);
> +
> +      if (!node)
> +        return NULL;
> +      for (node = node->same_body; node; node = node->next)
> +        if (node->thunk.thunk_p && off == node->thunk.fixed_offset)
> +          break;
> +      if (node)
> +        fndecl = node->decl;
> +      else
> +        return NULL;
> +     }
> +
>    /* When cgraph node is missing and function is not public, we cannot
>       devirtualize.  This can happen in WHOPR when the actual method
>       ends up in other partition, because we found devirtualization
> Index: icln/gcc/testsuite/g++.dg/otr-fold-1.C
> ===================================================================
> --- icln.orig/gcc/testsuite/g++.dg/otr-fold-1.C
> +++ icln/gcc/testsuite/g++.dg/otr-fold-1.C
> @@ -72,5 +72,5 @@ int main (int argc, char *argv[])
>    return 0;
>  }
>  
> -/* { dg-final { scan-tree-dump "= B::foo"  "optimized"  } } */
> +/* { dg-final { scan-tree-dump "= B::.*foo"  "optimized"  } } */
>  /* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: icln/gcc/testsuite/g++.dg/otr-fold-2.C
> ===================================================================
> --- icln.orig/gcc/testsuite/g++.dg/otr-fold-2.C
> +++ icln/gcc/testsuite/g++.dg/otr-fold-2.C
> @@ -84,5 +84,5 @@ int main (int argc, char *argv[])
>    return 0;
>  }
>  
> -/* { dg-final { scan-tree-dump "= B::foo"  "optimized"  } } */
> +/* { dg-final { scan-tree-dump "= B::.*foo"  "optimized"  } } */
>  /* { dg-final { cleanup-tree-dump "optimized" } } */
> Index: icln/gcc/testsuite/g++.dg/torture/pr45699.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/torture/pr45699.C
> @@ -0,0 +1,61 @@
> +// { dg-do run }
> +
> +extern "C" void abort ();
> +
> +class A
> +{
> +public:
> +  virtual void foo () {abort();}
> +};
> +
> +class B : public A
> +{
> +public:
> +  int z;
> +  virtual void foo () {abort();}
> +};
> +
> +class C : public A
> +{
> +public:
> +  void *a[32];
> +  unsigned long b;
> +  long c[32];
> +
> +  virtual void foo () {abort();}
> +};
> +
> +class D : public C, public B
> +{
> +public:
> +  D () : C(), B()
> +  {
> +    int i;
> +    for (i = 0; i < 32; i++)
> +      {
> +	a[i] = (void *) 0;
> +	c[i] = 0;
> +      }
> +    b = 0xaaaa;
> +  }
> +
> +  virtual void foo ();
> +};
> +
> +void D::foo()
> +{
> +  if (b != 0xaaaa)
> +    abort();
> +}
> +
> +static inline void bar (B &b)
> +{
> +  b.foo ();
> +}
> +
> +int main()
> +{
> +  D d;
> +  bar (d);
> +  return 0;
> +}
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

[PATCH, PR 45699] Devirtualize to thunks

[Martin Jambor]

Hi,

folding of OBJ_TYPE_REFs just takes the function declaration in BINFOs
and puts into the call statement.  Unfortunately BINFOs do not put the
declaration of the proper thunk there and so we might ending up not
adjusting the this pointer like in the testcase below.  On the other
hand, BINFOs do contain the deltas and so the folding code can look up
the right thunk in the call graph if need be.  This is what the patch
below does.

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

Thanks,

Martin


2010-10-08  Martin Jambor  <mjambor@suse.cz>

	PR middle-end/45699
	* gimple-fold.c (gimple_fold_obj_type_ref_known_binfo): Choose among
	thunks.

	* testsuite/g++.dg/torture/pr45699.C: New test.
	* testsuite/g++.dg/otr-fold-1.C: Adjusted.
	* testsuite/g++.dg/otr-fold-1.C: Likewise.


Index: icln/gcc/gimple-fold.c
===================================================================
--- icln.orig/gcc/gimple-fold.c
+++ icln/gcc/gimple-fold.c
@@ -1463,7 +1463,7 @@ tree
 gimple_fold_obj_type_ref_known_binfo (HOST_WIDE_INT token, tree known_binfo)
 {
   HOST_WIDE_INT i;
-  tree v, fndecl;
+  tree v, fndecl, delta;
 
   v = BINFO_VIRTUALS (known_binfo);
   i = 0;
@@ -1475,6 +1475,25 @@ gimple_fold_obj_type_ref_known_binfo (HO
     }
 
   fndecl = TREE_VALUE (v);
+  delta = TREE_PURPOSE (v);
+  gcc_assert (host_integerp (delta, 0));
+
+  if (integer_nonzerop (delta))
+    {
+      struct cgraph_node *node = cgraph_get_node (fndecl);
+      HOST_WIDE_INT off = tree_low_cst (delta, 0);
+
+      if (!node)
+        return NULL;
+      for (node = node->same_body; node; node = node->next)
+        if (node->thunk.thunk_p && off == node->thunk.fixed_offset)
+          break;
+      if (node)
+        fndecl = node->decl;
+      else
+        return NULL;
+     }
+
   /* When cgraph node is missing and function is not public, we cannot
      devirtualize.  This can happen in WHOPR when the actual method
      ends up in other partition, because we found devirtualization
Index: icln/gcc/testsuite/g++.dg/otr-fold-1.C
===================================================================
--- icln.orig/gcc/testsuite/g++.dg/otr-fold-1.C
+++ icln/gcc/testsuite/g++.dg/otr-fold-1.C
@@ -72,5 +72,5 @@ int main (int argc, char *argv[])
   return 0;
 }
 
-/* { dg-final { scan-tree-dump "= B::foo"  "optimized"  } } */
+/* { dg-final { scan-tree-dump "= B::.*foo"  "optimized"  } } */
 /* { dg-final { cleanup-tree-dump "optimized" } } */
Index: icln/gcc/testsuite/g++.dg/otr-fold-2.C
===================================================================
--- icln.orig/gcc/testsuite/g++.dg/otr-fold-2.C
+++ icln/gcc/testsuite/g++.dg/otr-fold-2.C
@@ -84,5 +84,5 @@ int main (int argc, char *argv[])
   return 0;
 }
 
-/* { dg-final { scan-tree-dump "= B::foo"  "optimized"  } } */
+/* { dg-final { scan-tree-dump "= B::.*foo"  "optimized"  } } */
 /* { dg-final { cleanup-tree-dump "optimized" } } */
Index: icln/gcc/testsuite/g++.dg/torture/pr45699.C
===================================================================
--- /dev/null
+++ icln/gcc/testsuite/g++.dg/torture/pr45699.C
@@ -0,0 +1,61 @@
+// { dg-do run }
+
+extern "C" void abort ();
+
+class A
+{
+public:
+  virtual void foo () {abort();}
+};
+
+class B : public A
+{
+public:
+  int z;
+  virtual void foo () {abort();}
+};
+
+class C : public A
+{
+public:
+  void *a[32];
+  unsigned long b;
+  long c[32];
+
+  virtual void foo () {abort();}
+};
+
+class D : public C, public B
+{
+public:
+  D () : C(), B()
+  {
+    int i;
+    for (i = 0; i < 32; i++)
+      {
+	a[i] = (void *) 0;
+	c[i] = 0;
+      }
+    b = 0xaaaa;
+  }
+
+  virtual void foo ();
+};
+
+void D::foo()
+{
+  if (b != 0xaaaa)
+    abort();
+}
+
+static inline void bar (B &b)
+{
+  b.foo ();
+}
+
+int main()
+{
+  D d;
+  bar (d);
+  return 0;
+}