Diamond shaped class structure with a covariant return method causes GCC to fail to adjust base pointers. In the attached program, the output is a huge number instead of the expected 123. Looking at what happens in GDB, it appears that the this pointer in the call of A(const A &other) gets a B reference rather than an A reference. The place where it expects to find the member A::a is actually outside of the object of type F. If I'm reading the output from GDB correctly.
Created attachment 19921 [details] Test case No special flags, just compile with "c++ thunk.cc -o thunk".
Confirmed. Broken since it is supported.
Reduced a bit: extern "C" void abort (); struct A { virtual void dummy() {} }; struct B { virtual B *clone() = 0; }; struct C : public virtual B { }; struct D : public virtual B { }; struct E : public C, public D { virtual E *clone() = 0; }; struct F* fp; struct F : public A, public E { F(int) { fp = this; } virtual E *clone() { if (fp != this) abort (); } }; int main() { F *a = new F(123); B *c = a; c->clone(); }
Created attachment 19944 [details] A different simplification, avoiding the diamond inheritance The diamond inheritance does not seem to be a requirement. Here is a case simplified in a different way, where there is no diamond inheritance any more. (This one does print "33" rather than a huge number. Still wrong, obviously.)
This seems to have been broken by 2003-01-27 Nathan Sidwell <nathan@codesourcery.com> * class.c (update_vtable_entry_for_fn): Add index parameter. Generate vcall thunk for covariant overriding from a virtual primary base. (dfs_modify_vtables): Adjust. Reverting that patch fixes the problem and doesn't break any of the existing covariant tests. Nathan, I couldn't figure out what the comment you added was talking about; I don't suppose you have any idea after all these years?
Subject: Re: Virtual inheritence with covariant return type confuses GCC Jason, > Reverting that patch fixes the problem and doesn't break any of the existing > covariant tests. Nathan, I couldn't figure out what the comment you added was > talking about; I don't suppose you have any idea after all these years? the patch email is at http://gcc.gnu.org/ml/gcc-patches/2003-01/msg02157.html and I see you and I discussed the patch then too, and it seems you took some convincing then too :) Does that email thread help, or would you like me to dig deeper? nathan
That thread does help, thanks. But part of my confusion was that the testcase you added with that patch (abi/covariant1) didn't actually seem to test the bug. When I add a definition of c14::f17, the test starts failing; even 3.4 refers to the no-this-adjustment covariant thunk in the c14 vtable. So I'm not sure what the patch was actually doing.
Hm, I guess I must have flubbed the testcase. After all this time, I don't have a better recollection. Sorry.
Subject: Bug 43120 Author: jason Date: Thu Jul 8 14:00:26 2010 New Revision: 161954 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=161954 Log: PR c++/43120 * class.c (update_vtable_entry_for_fn): Fix handling of dummy virtual bases for covariant thunks. Added: trunk/gcc/testsuite/g++.dg/inherit/covariant17.C Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/class.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/g++.dg/abi/covariant1.C
Subject: Bug 43120 Author: jason Date: Fri Jul 9 19:36:19 2010 New Revision: 162008 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162008 Log: 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. Added: trunk/gcc/testsuite/g++.dg/abi/covariant6.C Modified: trunk/gcc/cp/ChangeLog trunk/gcc/cp/class.c trunk/gcc/cp/cp-tree.h trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/g++.dg/abi/covariant1.C trunk/gcc/testsuite/g++.dg/inherit/covariant17.C trunk/gcc/testsuite/g++.dg/inherit/covariant7.C
Subject: Bug 43120 Author: jason Date: Fri Jul 9 19:40:39 2010 New Revision: 162010 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162010 Log: PR c++/43120 * class.c (update_vtable_entry_for_fn): Fix handling of dummy virtual bases for covariant thunks. Added: branches/gcc-4_5-branch/gcc/testsuite/g++.dg/inherit/covariant17.C Modified: branches/gcc-4_5-branch/gcc/cp/ChangeLog branches/gcc-4_5-branch/gcc/cp/class.c branches/gcc-4_5-branch/gcc/testsuite/ChangeLog branches/gcc-4_5-branch/gcc/testsuite/g++.dg/abi/covariant1.C
Subject: Bug 43120 Author: jason Date: Fri Jul 9 19:45:53 2010 New Revision: 162011 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162011 Log: PR c++/43120 * class.c (update_vtable_entry_for_fn): Fix handling of dummy virtual bases for covariant thunks. Added: branches/gcc-4_4-branch/gcc/testsuite/g++.dg/inherit/covariant17.C Modified: branches/gcc-4_4-branch/gcc/cp/ChangeLog branches/gcc-4_4-branch/gcc/cp/class.c branches/gcc-4_4-branch/gcc/testsuite/ChangeLog branches/gcc-4_4-branch/gcc/testsuite/g++.dg/abi/covariant1.C
Subject: Bug 43120 Author: jason Date: Fri Jul 9 19:50:25 2010 New Revision: 162013 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162013 Log: Revert " PR c++/43120" Removed: branches/gcc-4_5-branch/gcc/testsuite/g++.dg/inherit/covariant17.C Modified: branches/gcc-4_5-branch/gcc/cp/ChangeLog branches/gcc-4_5-branch/gcc/cp/class.c branches/gcc-4_5-branch/gcc/testsuite/ChangeLog branches/gcc-4_5-branch/gcc/testsuite/g++.dg/abi/covariant1.C
Fixed for 4.6. This isn't a regression, and the fix changes the ABI (for code that is badly broken without it), so I'm not comfortable applying it to the release branches. I did apply it, but then reverted it again.