This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
C++ PATCH for c++/43120 (wrong-code with covariant thunks)
- From: Jason Merrill <jason at redhat dot com>
- To: gcc-patches List <gcc-patches at gcc dot gnu dot org>
- Cc: Nathan Sidwell <nathan at codesourcery dot com>
- Date: Fri, 09 Jul 2010 14:51:53 -0400
- Subject: C++ PATCH for c++/43120 (wrong-code with covariant thunks)
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.
commit 8e5e243fbef76b0ea5652414b79d4ba8b86acfa7
Author: Jason Merrill <jason@redhat.com>
Date: Fri Jul 2 23:14:14 2010 -0400
PR c++/43120
* class.c (update_vtable_entry_for_fn): Fix handling of dummy
virtual bases for covariant thunks.
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index 3c4830e..20b8c12 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -2058,8 +2058,9 @@ get_vcall_index (tree fn, tree type)
}
/* Update an entry in the vtable for BINFO, which is in the hierarchy
- dominated by T. FN has been overridden in BINFO; VIRTUALS points to the
- corresponding position in the BINFO_VIRTUALS list. */
+ dominated by T. FN is the old function; VIRTUALS points to the
+ corresponding position in the new BINFO_VIRTUALS list. IX is the index
+ of that entry in the list. */
static void
update_vtable_entry_for_fn (tree t, tree binfo, tree fn, tree* virtuals,
@@ -2252,9 +2253,11 @@ update_vtable_entry_for_fn (tree t, tree binfo, tree fn, tree* virtuals,
virtual_base = probe;
if (virtual_base)
- /* Even if we find a virtual base, the correct delta is
- between the overrider and the binfo we're building a vtable
- for. */
+ /* 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;
}
@@ -2272,12 +2275,12 @@ 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
BINFO to pointing at the base where the final overrider
appears. */
- virtual_covariant:
delta = size_diffop_loc (input_location,
convert (ssizetype,
BINFO_OFFSET (TREE_VALUE (overrider))),
diff --git a/gcc/testsuite/g++.dg/abi/covariant1.C b/gcc/testsuite/g++.dg/abi/covariant1.C
index 203ec2c..42522c1 100644
--- a/gcc/testsuite/g++.dg/abi/covariant1.C
+++ b/gcc/testsuite/g++.dg/abi/covariant1.C
@@ -16,6 +16,11 @@ struct c12 : c11 { };
struct c14 :
virtual c12,
- virtual c11 { virtual c12* f17(); };
+ virtual c11 { virtual void f(); c12* f17(); };
-// { dg-final { scan-assembler-not "\n_ZTch0_v0_n16_N3c143f17Ev\[: \t\n\]" } }
+void c14::f() { }
+
+// { dg-final { scan-assembler "_ZTcv0_n12_v0_n16_N3c143f17Ev" { target ilp32 } } }
+// { dg-final { scan-assembler-not "_ZTch0_v0_n16_N3c143f17Ev" } }
+// { dg-final { scan-assembler "_ZTcv0_n24_v0_n32_N3c143f17Ev" { target lp64 } } }
+// { dg-final { scan-assembler-not "_ZTch0_v0_n32_N3c143f17Ev" } }
diff --git a/gcc/testsuite/g++.dg/inherit/covariant17.C b/gcc/testsuite/g++.dg/inherit/covariant17.C
new file mode 100644
index 0000000..26031d5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/inherit/covariant17.C
@@ -0,0 +1,40 @@
+// PR c++/43120
+// { dg-do run }
+
+extern "C" void abort ();
+
+struct A {
+ int a;
+
+ A(int a_) : a(a_) {}
+
+ A(const A &other) { }
+
+ virtual void dummy() {}
+};
+
+struct B {
+ virtual B *clone() const = 0;
+};
+
+struct C : public virtual B {
+ virtual B *clone() const = 0;
+};
+
+struct E* ep;
+struct E : public A, public C {
+ E(int a_) : A(a_) { ep = this; }
+
+ virtual E *clone() const {
+ if (this != ep)
+ abort();
+ return new E(*this);
+ }
+};
+
+int main() {
+ E *a = new E(123);
+ B *c = a;
+ B *d = c->clone();
+ return 0;
+}
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" } }