Semi-reduced test case: $ cat t.cpp struct A { virtual int foo() { return a; } int a; }; struct B : virtual A { B() : b(1) {} virtual int bar() { return b; } int b; }; struct C : virtual A { C() : c(2) {} virtual int foo() { return c; } int c; }; struct D : B, C { D() : d(3) {} virtual int bar() { return d; } int d; }; int main() { D d; return d.foo(); } $ clang-cl -GR- t.cpp && ./t.exe ; echo $? 3 This should return 2, not 3. It's accessing 'd' instead of 'c'.
This was reduced from your best friend and mine, RenderWidgetHostView in Chromium.
We're missing an adjusting thunk for foo in the the vftable for A in D.
This isn't going to be an easy fix. Timur, can you take a look at it? I don't have a patch yet, and I'll work on vftable name mangling tomorrow, since David wanted me to look at that. What I've learned so far is that we iterate over the paths from the MDC to vftables, and in this instance, we get: vftable for B in D vftable for A in B in D // vftable for A in C in D is suppressed due to shared vbase Because the final override of 'foo' is in C, we don't observe it. We don't observe it because we have code like: const CXXMethodDecl *OverriddenMD = FindNearestOverriddenMethod(MD, VisitedBases); MethodInfoMapTy::iterator OverriddenMDIterator; if (OverriddenMD) { ... CharUnits TI = ComputeThisOffset(MD, Base, Overrider); if (TI != WhichVFPtr.VFPtrFullOffset) { ... Meaning, we always say the this offset is zero when we can't see the final overrider along this inheritance path. If C were D's primary base, things would work.
I've started looking at this but got distracted by other duties. If you feel like this is blocking you – feel free to take over, otherwise I'll look into it tomorrow.
We're looking into emitting an error on this so we can fallback. I'm not going to try to do a full fix for now.
In your example, I believe you said it was significant that D inherits from a *non-primary* base (C) that overrides methods from a virtual base (A). Your example returns the correct value if the inheritance order is reversed, i.e. struct D : C, B I think I've found a different way to make it fail. If I change C to this, and look at the value of d.bar() instead: struct D : B, C { D() : d(3) {} virtual int bar() { return C::foo(); } int d; }; D.bar() will now return 3 independently of in which order it inherits from B and C. (Reduced from Chromium's RenderViewImpl.)
I've made Clang error out on this case in r202331. We should revert that when we fix the issue.
Here's another example of a (possibly related) bad vftable layout in the presence of virtual inheritance: struct A { virtual void f() {} }; struct B : virtual A {}; struct C : virtual B, A {}; C c; This test case is from PR17748. We generate an empty vftable for the non-virtual A in C: VFTable for 'A' in 'B' in 'C' (1 entries). 0 | void A::f() VFTable for 'A' in 'C' (0 entries). Which gives this IR: @"\01??_7C@@6BA@@@" = linkonce_odr unnamed_addr constant [0 x i8*] zeroinitializer @"\01??_7C@@6BB@@@" = linkonce_odr unnamed_addr constant [1 x i8*] [i8* bitcast (void (%struct.A*)* @"\01?f@A@@UAEXXZ" to i8*)] Which crashes the COFF writer as reported in PR18993.
I think the latter one is unrelated and should be fixed separately -- filed PR19066.
r203222
The test from Comment 6 still fails: struct A { virtual int foo() { return a; } int a; }; struct B : virtual A { B() : b(1) {} virtual int bar() { return b; } int b; }; struct C : virtual A { C() : c(2) {} virtual int foo() { return c; } int c; }; struct D : B, C { D() : d(3) {} virtual int bar() { return C::foo(); } int d; }; int main() { D d; printf("%d\n", d.bar()); return 0; }
Hm, interesting. I've looked at this code and the vftables are correct on this code. Looks like it's the "C::foo()" call that we miscompile. Filed PR19104 to investigate this separately.