From patchwork Fri Jul 9 18:51:53 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Merrill X-Patchwork-Id: 58425 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 56385B6EF3 for ; Sat, 10 Jul 2010 04:52:23 +1000 (EST) Received: (qmail 5762 invoked by alias); 9 Jul 2010 18:52:22 -0000 Received: (qmail 5742 invoked by uid 22791); 9 Jul 2010 18:52:18 -0000 X-SWARE-Spam-Status: No, hits=-5.0 required=5.0 tests=AWL, BAYES_20, RCVD_IN_DNSWL_HI, SPF_HELO_PASS, TW_VP, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 09 Jul 2010 18:52:11 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o69Ipt0Q031712 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 9 Jul 2010 14:51:55 -0400 Received: from [IPv6:::1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o69Ipsg8031612; Fri, 9 Jul 2010 14:51:54 -0400 Message-ID: <4C376FC9.70403@redhat.com> Date: Fri, 09 Jul 2010 14:51:53 -0400 From: Jason Merrill User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.10) Gecko/20100619 Lightning/1.0b1 Shredder/3.0.6pre MIME-Version: 1.0 To: gcc-patches List CC: Nathan Sidwell Subject: C++ PATCH for c++/43120 (wrong-code with covariant thunks) Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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 d01da10464311453e51ff634550235b640af5d90 Author: Jason Merrill 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 @@ -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" } }