Follow-up to r206457 -- fix static adjustments for some subtle virtual inheritance cases

[rnk]

I think the vftable layout code is getting pretty complicated. What do you think about rewriting it with a more complete understanding of what vftables need?

I sat down with David Majnemer to try to figure out what all goes into a given vftable slot, and we came up with five offset-like things:

• The adjustment that the method will do in the prologue. This is the negation of the vfptr offset in the class defining the method.
• The static this adjustment done in the thunk. This is the difference between the offset of the vfptr containing the thunk and the adjustment done in the prologue.
• The vtordisp offset. This is the offset from the vfptr to the containing vbase minus four.
• The containing vbase, which is only present for vtordispex thunks. This is used for vbtable lookup. Equivalently, this is the vbtable index or offset.
• The offset from the vbase containing the vfptr to the vbptr in the complete type. This is used to power the vbtable lookup.

We don't capture this with our current data structures, but I imagine we could simplify the code a lot if we did.

test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp

459

Going forward, I'd really prefer it if our inheritance hierarchy tests could all be standalone. See the microsoft-abi-vbtables.cpp test cases, where the contents of any namespace can be cut-and-paste into a standalone .cpp file and compiled.

In this test, I have to go lookup ::A and ::B in the right namespace to figure out what kind of record this is going to be. It's worth a few lines of duplicated test code to avoid that.

[rnk]

lgtm

I haven't been able to fully convince myself that the math is correct, but it does simplify the code and eliminate special cases. I still think we have a rewrite in the very near future, but this will be an easier reference to work from.

[timurrrr]

r206504, thanks!

We might indeed consider to refactor the code when we have better test coverage and better understanding of the corner cases.

I'll consider simplifying the tests in a separate commit.

[timurrrr]

2014-04-18 1:38 GMT+04:00 Reid Kleckner <[e-mail]>:

Comment at: test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp:459
@@ +458,3 @@
+namespace Test12 {
+struct X : B, A { };

+

Going forward, I'd really prefer it if our inheritance hierarchy tests could all be standalone. See the microsoft-abi-vbtables.cpp test cases, where the contents of any namespace can be cut-and-paste into a standalone .cpp file and compiled.

In this test, I have to go lookup ::A and ::B in the right namespace to figure out what kind of record this is going to be. It's worth a few lines of duplicated test code to avoid that.

Like r206614?

lib/AST/VTableBuilder.cpp

 
   // This will hold the smallest this offset among overridees of MD.
   // This implies that an offset of a non-virtual base will dominate an offset
   // of a virtual base to potentially reduce the number of thunks required
   // in the derived classes that inherit this method.
   CharUnits Ret;
   bool First = true;
 
+  const ASTRecordLayout &OverriderRDLayout =
+      Context.getASTRecordLayout(Overrider.Method->getParent());
   for (CXXBasePaths::paths_iterator I = Paths.begin(), E = Paths.end();
        I != E; ++I) {
     const CXXBasePath &Path = (*I);
     CharUnits ThisOffset = Overrider.Offset;
     CharUnits LastVBaseOffset;
 
     // For each path from the overrider to the parents of the overridden methods,
     // traverse the path, calculating the this offset in the most derived class.
     for (int J = 0, F = Path.size(); J != F; ++J) {
       const CXXBasePathElement &Element = Path[J];
       QualType CurTy = Element.Base->getType();
       const CXXRecordDecl *PrevRD = Element.Class,
                           *CurRD = CurTy->getAsCXXRecordDecl();
       const ASTRecordLayout &Layout = Context.getASTRecordLayout(PrevRD);
 
       if (Element.Base->isVirtual()) {
-        LastVBaseOffset = MostDerivedClassLayout.getVBaseClassOffset(CurRD);
-        if (Overrider.Method->getParent() == PrevRD) {
-          // This one's interesting. If the final overrider is in a vbase B of the
-          // most derived class and it overrides a method of the B's own vbase A,
-          // it uses A* as "this". In its prologue, it can cast A* to B* with
-          // a static offset. This offset is used regardless of the actual
-          // offset of A from B in the most derived class, requiring an
-          // this-adjusting thunk in the vftable if A and B are laid out
-          // differently in the most derived class.
-          ThisOffset += Layout.getVBaseClassOffset(CurRD);
-        } else {
-          ThisOffset = LastVBaseOffset;
-        }
+        // The interesting things begin when you have virtual inheritance.
+        // The final overrider will use a static adjustment equal to the offset
+        // of the vbase in the final overrider class.
+        // For example, if the final overrider is in a vbase B of the most
+        // derived class and it overrides a method of the B's own vbase A,
+        // it uses A* as "this".  In its prologue, it can cast A* to B* with
+        // a static offset.  This offset is used regardless of the actual
+        // offset of A from B in the most derived class, requiring an
+        // this-adjusting thunk in the vftable if A and B are laid out
+        // differently in the most derived class.
+        LastVBaseOffset = ThisOffset =
+            Overrider.Offset + OverriderRDLayout.getVBaseClassOffset(CurRD);
       } else {
         ThisOffset += Layout.getBaseClassOffset(CurRD);
       }
     }
 
     if (isa<CXXDestructorDecl>(Overrider.Method)) {
       if (LastVBaseOffset.isZero()) {
         // If a "Base" class has at least one non-virtual base with a virtual

   // OK, now we know we need to use a vtordisp thunk.
   // The implicit vtordisp field is located right before the vbase.
   CharUnits VFPtrVBaseOffset = VBaseMapEntry->second.VBaseOffset;
   TA.Virtual.Microsoft.VtordispOffset =
       (VFPtrVBaseOffset - WhichVFPtr.FullOffsetInMDC).getQuantity() - 4;
 
   // A simple vtordisp thunk will suffice if the final overrider is defined
   // in either the most derived class or its non-virtual base.
-  if (OverriderRD == MostDerivedClass)
-    return;
-
-  if (!OverriderVBase) {
-    MethodVFTableLocation ML = VTables.getMethodVFTableLocation(Overrider.Method);
-    assert(ML.VBase && "why would we need a vtordisp if we can call the method "
-                       "without a vfptr of a vbase?");
-    // We need to offset the this parameter if the offset of the vbase is
-    // different between the overrider class and the most derived class.
-    const ASTRecordLayout &OverriderRDLayout =
-        Context.getASTRecordLayout(OverriderRD);
-    TA.NonVirtual += (OverriderRDLayout.getVBaseClassOffset(ML.VBase) +
-                     ML.VFPtrOffset - ThisOffset).getQuantity();
+  if (OverriderRD == MostDerivedClass || !OverriderVBase)
     return;
-  }
 
   // Otherwise, we need to do use the dynamic offset of the final overrider
   // in order to get "this" adjustment right.
   TA.Virtual.Microsoft.VBPtrOffset =
       (VFPtrVBaseOffset + WhichVFPtr.NonVirtualOffset -
        MostDerivedClassLayout.getVBPtrOffset()).getQuantity();
   TA.Virtual.Microsoft.VBOffsetOffset =
       Context.getTypeSizeInChars(Context.IntTy).getQuantity() *

test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp

 Z z;
 
 struct W : virtual X, A {};
 
 // Used to crash, PR17748.
 W w;
 }
 
+namespace Test12 {
+struct X : B, A { };
+
+struct Y : X {
+  virtual void f();  // Overrides A::f.
+};
+
+struct Z : virtual Y {
+  // CHECK-LABEL: VFTable for 'A' in 'Test12::X' in 'Test12::Y' in 'Test12::Z' (2 entries).
+  // CHECK-NEXT:   0 | void Test12::Y::f()
+  // CHECK-NEXT:   1 | void A::z()
+
+  int z;
+  // MANGLING-DAG: @"\01??_7Z@Test12@@6BA@@@" = {{.*}}@"\01?f@Y@Test12@@UAEXXZ"
+};
+
+struct W : Z {
+  // CHECK-LABEL: VFTable for 'A' in 'Test12::X' in 'Test12::Y' in 'Test12::Z' in 'Test12::W' (2 entries).
+  // CHECK-NEXT:   0 | void Test12::Y::f()
+  // CHECK-NEXT:   1 | void A::z()
+  W();
+
+  int w;
+  // MANGLING-DAG: @"\01??_7W@Test12@@6BA@@@" = {{.*}}@"\01?f@Y@Test12@@UAEXXZ"
+};
+
+W::W() {}
+}
+
 namespace vdtors {
 struct X {
   virtual ~X();
   virtual void zzz();
 };
 
 struct Y : virtual X {
   // CHECK-LABEL: VFTable for 'vdtors::X' in 'vdtors::Y' (2 entries).

 
 // Each MDC only has one vftable.
 
 // MANGLING-DAG: @"\01??_7D@pr19240@@6B@"
 // MANGLING-DAG: @"\01??_7A@pr19240@@6B@"
 // MANGLING-DAG: @"\01??_7B@pr19240@@6B@"
 
 }
+
+namespace pr19408 {
+// This test is a non-vtordisp version of the reproducer for PR19408.
+struct X : virtual A {
+  int x;
+};
+
+struct Y : X {
+  virtual void f();
+  int y;
+};
+
+struct Z : Y {
+  // CHECK-LABEL: VFTable for 'A' in 'pr19408::X' in 'pr19408::Y' in 'pr19408::Z' (2 entries).
+  // CHECK-NEXT:   0 | void pr19408::Y::f()
+  // CHECK-NEXT:       [this adjustment: -4 non-virtual]
+  // CHECK-NEXT:   1 | void A::z()
+
+  Z();
+  int z;
+  // MANGLING-DAG: @"\01??_7Z@pr19408@@6B@" = {{.*}}@"\01?f@Y@pr19408@@W3AEXXZ"
+};
+
+Z::Z() {}
+
+struct W : B, Y {
+  // CHECK-LABEL: VFTable for 'A' in 'pr19408::X' in 'pr19408::Y' in 'pr19408::W' (2 entries).
+  // CHECK-NEXT:   0 | void pr19408::Y::f()
+  // CHECK-NEXT:       [this adjustment: -4 non-virtual]
+  // CHECK-NEXT:   1 | void A::z()
+
+  W();
+  int w;
+  // MANGLING-DAG: @"\01??_7W@pr19408@@6BY@1@@" = {{.*}}@"\01?f@Y@pr19408@@W3AEXXZ"
+};
+
+W::W() {}
+}