Patchwork C++ PATCH for c++/43120 (wrong-code with covariant thunks)

login
register
mail settings
Submitter Jason Merrill
Date July 9, 2010, 6:51 p.m.
Message ID <4C376FC9.70403@redhat.com>
Download mbox | patch
Permalink /patch/58425/
State New
Headers show

Comments

Jason Merrill - July 9, 2010, 6:51 p.m.
As previously discussed at
http://gcc.gnu.org/ml/gcc-patches/2003-01/msg02157.html
when we have a covariant override of a virtual function from a primary 
virtual base, we need to use the vcall offset for the 'this' adjustment 
even though we know that the base is primary and therefore doesn't need 
any adjustment.

But the code implementing this wasn't quite right; it assumed that the 
non-virtual offset between the binfo being considered and the overrider 
was always what we wanted, which is incorrect if the binfo is a 
secondary base of the overrider.  In fact, the non-virtual offset we 
want is always zero, because we're always looking at a primary virtual 
base of binfo which is necessarily at offset zero.

So that's 43120.patch, which I checked in the other day to fix the 
wrong-code bug.

But I still didn't feel like I had a good handle on the the issue, and 
so I kept digging to figure out a model that satisfied me.  What I 
eventually realized was that the issue was with determining which class 
the vtable slot pertains to, which is the nearest definition that 
doesn't require a covariant thunk; definitions that require a covariant 
thunk in this slot will use a different slot when called through a 
pointer to their class.

This refinement allows us to optimize our vtable layout a bit: in the 
new abi/covariant6.C the C vtable can refer to the (non-virtual) thunk 
for B rather than the (virtual) thunk for A.  And in 
inherit/covariant7.C, we don't need to initialize the vtable slot for 
c1-in-c3-in-c4-in-c6 because it's a lost primary; a call through a c3 
object will use a different vtable slot.

So that's 43120-take2.patch, which I'm going to check in now.  While 
this changes the vtable contents, it does not change the ABI; it doesn't 
change the set of thunks emitted for a function, it just changes some 
slots to refer to a different member of that set, or none.

Tested x86_64-pc-linux-gnu, applied to trunk.
H.J. Lu - Sept. 1, 2010, 2:30 a.m.
On Fri, Jul 9, 2010 at 11:51 AM, Jason Merrill <jason@redhat.com> wrote:
> As previously discussed at
> http://gcc.gnu.org/ml/gcc-patches/2003-01/msg02157.html
> when we have a covariant override of a virtual function from a primary
> virtual base, we need to use the vcall offset for the 'this' adjustment even
> though we know that the base is primary and therefore doesn't need any
> adjustment.
>
> But the code implementing this wasn't quite right; it assumed that the
> non-virtual offset between the binfo being considered and the overrider was
> always what we wanted, which is incorrect if the binfo is a secondary base
> of the overrider.  In fact, the non-virtual offset we want is always zero,
> because we're always looking at a primary virtual base of binfo which is
> necessarily at offset zero.
>
> So that's 43120.patch, which I checked in the other day to fix the
> wrong-code bug.
>
> But I still didn't feel like I had a good handle on the the issue, and so I
> kept digging to figure out a model that satisfied me.  What I eventually
> realized was that the issue was with determining which class the vtable slot
> pertains to, which is the nearest definition that doesn't require a
> covariant thunk; definitions that require a covariant thunk in this slot
> will use a different slot when called through a pointer to their class.
>
> This refinement allows us to optimize our vtable layout a bit: in the new
> abi/covariant6.C the C vtable can refer to the (non-virtual) thunk for B
> rather than the (virtual) thunk for A.  And in inherit/covariant7.C, we
> don't need to initialize the vtable slot for c1-in-c3-in-c4-in-c6 because
> it's a lost primary; a call through a c3 object will use a different vtable
> slot.
>
> So that's 43120-take2.patch, which I'm going to check in now.  While this
> changes the vtable contents, it does not change the ABI; it doesn't change
> the set of thunks emitted for a function, it just changes some slots to
> refer to a different member of that set, or none.
>
> Tested x86_64-pc-linux-gnu, applied to trunk.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45473

Patch

commit d01da10464311453e51ff634550235b640af5d90
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Jul 9 14:08:39 2010 -0400

    	PR c++/43120
    	* cp-tree.h (BV_LOST_PRIMARY): New macro.
    	* class.c (update_vtable_entry_for_fn): Fix covariant thunk logic.
    	Set BV_LOST_PRIMARY.
    	(build_vtbl_initializer): Check BV_LOST_PRIMARY.

diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 20b8c12..c3b5822 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -2205,6 +2205,40 @@  update_vtable_entry_for_fn (tree t, tree binfo, tree fn, tree* virtuals,
     gcc_assert (DECL_INVALID_OVERRIDER_P (overrider_target) ||
 		!DECL_THUNK_P (fn));
 
+  /* If we need a covariant thunk, then we may need to adjust first_defn.
+     The ABI specifies that the thunks emitted with a function are
+     determined by which bases the function overrides, so we need to be
+     sure that we're using a thunk for some overridden base; even if we
+     know that the necessary this adjustment is zero, there may not be an
+     appropriate zero-this-adjusment thunk for us to use since thunks for
+     overriding virtual bases always use the vcall offset.
+
+     Furthermore, just choosing any base that overrides this function isn't
+     quite right, as this slot won't be used for calls through a type that
+     puts a covariant thunk here.  Calling the function through such a type
+     will use a different slot, and that slot is the one that determines
+     the thunk emitted for that base.
+
+     So, keep looking until we find the base that we're really overriding
+     in this slot: the nearest primary base that doesn't use a covariant
+     thunk in this slot.  */
+  if (overrider_target != overrider_fn)
+    {
+      if (BINFO_TYPE (b) == DECL_CONTEXT (overrider_target))
+	/* Skip past the overrider.  */
+	b = get_primary_binfo (b);
+      for (; ; b = get_primary_binfo (b))
+	{
+	  tree main_binfo = TYPE_BINFO (BINFO_TYPE (b));
+	  tree bv = chain_index (ix, BINFO_VIRTUALS (main_binfo));
+	  if (BINFO_LOST_PRIMARY_P (b))
+	    lost = true;
+	  if (!DECL_THUNK_P (TREE_VALUE (bv)))
+	    break;
+	}
+      first_defn = b;
+    }
+
   /* Assume that we will produce a thunk that convert all the way to
      the final overrider, and not to an intermediate virtual base.  */
   virtual_base = NULL_TREE;
@@ -2229,38 +2263,6 @@  update_vtable_entry_for_fn (tree t, tree binfo, tree fn, tree* virtuals,
 	}
     }
 
-  if (overrider_fn != overrider_target && !virtual_base)
-    {
-      /* The ABI specifies that a covariant thunk includes a mangling
-	 for a this pointer adjustment.  This-adjusting thunks that
-	 override a function from a virtual base have a vcall
-	 adjustment.  When the virtual base in question is a primary
-	 virtual base, we know the adjustments are zero, (and in the
-	 non-covariant case, we would not use the thunk).
-	 Unfortunately we didn't notice this could happen, when
-	 designing the ABI and so never mandated that such a covariant
-	 thunk should be emitted.  Because we must use the ABI mandated
-	 name, we must continue searching from the binfo where we
-	 found the most recent definition of the function, towards the
-	 primary binfo which first introduced the function into the
-	 vtable.  If that enters a virtual base, we must use a vcall
-	 this-adjusting thunk.  Bleah! */
-      tree probe = first_defn;
-
-      while ((probe = get_primary_binfo (probe))
-	     && (unsigned) list_length (BINFO_VIRTUALS (probe)) > ix)
-	if (BINFO_VIRTUAL_P (probe))
-	  virtual_base = probe;
-
-      if (virtual_base)
-	/* OK, first_defn got this function from a (possibly lost) primary
-	   virtual base, so we're going to use the vcall offset for that
-	   primary virtual base.  But the caller is passing a first_defn*,
-	   not a virtual_base*, so the correct delta is the delta between
-	   first_defn* and itself, i.e. zero.  */
-	goto virtual_covariant;
-    }
-
   /* Compute the constant adjustment to the `this' pointer.  The
      `this' pointer, when this function is called, will point at BINFO
      (or one of its primary bases, which are at the same offset).  */
@@ -2275,7 +2277,6 @@  update_vtable_entry_for_fn (tree t, tree binfo, tree fn, tree* virtuals,
        entry in our vtable.  Except possibly in a constructor vtable,
        if we happen to get our primary back.  In that case, the offset
        will be zero, as it will be a primary base.  */
-   virtual_covariant:
     delta = size_zero_node;
   else
     /* The `this' pointer needs to be adjusted from pointing to
@@ -2293,6 +2294,9 @@  update_vtable_entry_for_fn (tree t, tree binfo, tree fn, tree* virtuals,
       = get_vcall_index (overrider_target, BINFO_TYPE (virtual_base));
   else
     BV_VCALL_INDEX (*virtuals) = NULL_TREE;
+
+  if (lost)
+    BV_LOST_PRIMARY (*virtuals) = true;
 }
 
 /* Called from modify_all_vtables via dfs_walk.  */
@@ -7648,7 +7652,7 @@  build_vtbl_initializer (tree binfo,
 			int* non_fn_entries_p,
 			VEC(constructor_elt,gc) **inits)
 {
-  tree v, b;
+  tree v;
   vtbl_init_data vid;
   unsigned ix, jx;
   tree vbinfo;
@@ -7762,20 +7766,8 @@  build_vtbl_initializer (tree binfo,
 	 zero out unused slots in ctor vtables, rather than filling them
 	 with erroneous values (though harmless, apart from relocation
 	 costs).  */
-      for (b = binfo; ; b = get_primary_binfo (b))
-	{
-	  /* We found a defn before a lost primary; go ahead as normal.  */
-	  if (look_for_overrides_here (BINFO_TYPE (b), fn_original))
-	    break;
-
-	  /* The nearest definition is from a lost primary; clear the
-	     slot.  */
-	  if (BINFO_LOST_PRIMARY_P (b))
-	    {
-	      init = size_zero_node;
-	      break;
-	    }
-	}
+      if (BV_LOST_PRIMARY (v))
+	init = size_zero_node;
 
       if (! init)
 	{
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 56e86be..08398aa 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -168,6 +168,9 @@  c-common.h, not after.
 
      The BV_FN is the declaration for the virtual function itself.
 
+     If BV_LOST_PRIMARY is set, it means that this entry is for a lost
+     primary virtual base and can be left null in the vtable.
+
    BINFO_VTABLE
      This is an expression with POINTER_TYPE that gives the value
      to which the vptr should be initialized.  Use get_vtbl_decl_for_binfo
@@ -1767,6 +1770,8 @@  struct GTY((variable_size)) lang_type {
 /* The function to call.  */
 #define BV_FN(NODE) (TREE_VALUE (NODE))
 
+/* Whether or not this entry is for a lost primary virtual base.  */
+#define BV_LOST_PRIMARY(NODE) (TREE_LANG_FLAG_0 (NODE))
 
 /* For FUNCTION_TYPE or METHOD_TYPE, a list of the exceptions that
    this type can raise.  Each TREE_VALUE is a _TYPE.  The TREE_VALUE
diff --git a/gcc/testsuite/g++.dg/abi/covariant1.C b/gcc/testsuite/g++.dg/abi/covariant1.C
index 42522c1..ae8c5e6 100644
--- a/gcc/testsuite/g++.dg/abi/covariant1.C
+++ b/gcc/testsuite/g++.dg/abi/covariant1.C
@@ -1,8 +1,8 @@ 
 // { dg-do compile }
 // { dg-options "-w" }
 
-// We don't want to use a covariant thunk to have a virtual
-// primary base
+// If a covariant thunk is overriding a virtual primary base, we have to
+// use the vcall offset even though we know it will be 0.
 
 struct c4 {};
 
diff --git a/gcc/testsuite/g++.dg/abi/covariant6.C b/gcc/testsuite/g++.dg/abi/covariant6.C
new file mode 100644
index 0000000..9dfc5ba
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/covariant6.C
@@ -0,0 +1,34 @@ 
+struct A
+{
+  virtual A* f();
+};
+
+struct B: virtual A
+{
+  virtual A* f();
+};
+
+struct C: B
+{
+  virtual C* f();
+};
+
+C* C::f() { return 0; }
+
+// When we emit C::f, we should emit both thunks: one for B and one for A.
+// { dg-final { scan-assembler "_ZTch0_v0_n16_N1C1fEv" { target ilp32 } } }
+// { dg-final { scan-assembler "_ZTch0_v0_n32_N1C1fEv" { target lp64 } } }
+// { dg-final { scan-assembler "_ZTcv0_n12_v0_n16_N1C1fEv" { target ilp32 } } }
+// { dg-final { scan-assembler "_ZTcv0_n24_v0_n32_N1C1fEv" { target lp64 } } }
+
+struct D: B
+{
+  virtual void dummy ();
+  virtual D* f();
+};
+
+void D::dummy() { }
+
+// When we emit the D vtable, it should refer to the thunk for B.
+// { dg-final { scan-assembler "_ZTch0_v0_n16_N1D1fEv" { target ilp32 } } }
+// { dg-final { scan-assembler "_ZTch0_v0_n32_N1D1fEv" { target lp64 } } }
diff --git a/gcc/testsuite/g++.dg/inherit/covariant17.C b/gcc/testsuite/g++.dg/inherit/covariant17.C
index 26031d5..b2de15f 100644
--- a/gcc/testsuite/g++.dg/inherit/covariant17.C
+++ b/gcc/testsuite/g++.dg/inherit/covariant17.C
@@ -18,7 +18,7 @@  struct B {
 };
 
 struct C : public virtual B {
-  virtual B *clone() const = 0;
+  virtual C *clone() const = 0;
 };
 
 struct E* ep;
@@ -28,13 +28,16 @@  struct E : public A, public C {
   virtual E *clone() const {
     if (this != ep)
       abort();
-    return new E(*this);
+    return 0;
   }
 };
 
 int main() {
   E *a = new E(123);
-  B *c = a;
-  B *d = c->clone();
+  C *c = a;
+  B *b = a;
+  c->clone();
+  b->clone();
+  delete a;
   return 0;
 }
diff --git a/gcc/testsuite/g++.dg/inherit/covariant7.C b/gcc/testsuite/g++.dg/inherit/covariant7.C
index cbd58bb..4d519ed 100644
--- a/gcc/testsuite/g++.dg/inherit/covariant7.C
+++ b/gcc/testsuite/g++.dg/inherit/covariant7.C
@@ -1,4 +1,6 @@ 
 // { dg-do compile }
+// { dg-prune-output "direct base" }
+// { dg-options "-fdump-class-hierarchy" }
 
 // Copyright (C) 2002 Free Software Foundation, Inc.
 // Contributed by Nathan Sidwell 27 Dec 2002 <nathan@codesourcery.com>
@@ -27,7 +29,23 @@  struct c4 : virtual c3, virtual c0, virtual c1
   int m;
 };
 
-struct c6 : c0, c3, c4		// { dg-warning "direct base" "" }
+struct c6 : c0, c3, c4
 {
   virtual c1 &f2() volatile;
 };
+
+// f2 appears four times in the c6 vtables:
+// once in c1-in-c3-in-c6 - covariant, virtual base, uses c1 vcall offset and c0 vbase offset
+// { dg-final { scan-tree-dump "24    c6::_ZTcv0_n16_v0_n12_NV2c62f2Ev" "class" { target ilp32 } } }
+// { dg-final { scan-tree-dump "48    c6::_ZTcv0_n32_v0_n24_NV2c62f2Ev" "class" { target lp64 } } }
+// once in c3-in-c6 - non-covariant, non-virtual base, calls f2 directly
+// { dg-final { scan-tree-dump "28    c6::f2" "class" { target ilp32 } } }
+// { dg-final { scan-tree-dump "56    c6::f2" "class" { target lp64 } } }
+// once in c1-in-c3-in-c4-in-c6 - lost primary
+// { dg-final { scan-tree-dump "80    0u" "class" { target ilp32 } } }
+// { dg-final { scan-tree-dump "160   0u" "class" { target lp64 } } }
+// once in c3-in-c4-in-c6 - c3 vcall offset
+// { dg-final { scan-tree-dump "84    c6::_ZTv0_n16_NV2c62f2Ev" "class" { target ilp32 } } }
+// { dg-final { scan-tree-dump "168   c6::_ZTv0_n32_NV2c62f2Ev" "class" { target lp64 } } }
+
+// { dg-final { cleanup-tree-dump "class" } }