Fix PR19408 -- Missing static this adjustment in a vtordisp thunk

[rnk]

lgtm

lib/AST/VTableBuilder.cpp

2749

I never know what 'ThisOffset' refers to in this code. Is there a better name we could use?

[timurrrr]

I went forward with the test cases, uncovered a couple of more bugs and fixed them.
The change to the code is pretty straightforward so I hope you can review post-commit.

I'll revisit the ThisOffset naming as a separate commit.

[timurrrr]

r206457.

[timurrrr]

See the follow-up at http://reviews.llvm.org/D3410

It looks like in fact the value ThisOffset was wrong :)
So after D3410 fix "ThisOffset" remains as described in the ComputeThisOffset comment.

lib/AST/VTableBuilder.cpp

     FinalOverriders::OverriderInfo Overrider, CharUnits ThisOffset,
     ThisAdjustment &TA) {
   const ASTRecordLayout::VBaseOffsetsMapTy &VBaseMap =
       MostDerivedClassLayout.getVBaseOffsetsMap();
   const ASTRecordLayout::VBaseOffsetsMapTy::const_iterator &VBaseMapEntry =
       VBaseMap.find(WhichVFPtr.getVBaseWithVPtr());
   assert(VBaseMapEntry != VBaseMap.end());
 
-  // Check if we need a vtordisp adjustment at all.
+  // If there's no vtordisp, we don't need any vtordisp adjustment.
   if (!VBaseMapEntry->second.hasVtorDisp())
     return;
 
-  CharUnits VFPtrVBaseOffset = VBaseMapEntry->second.VBaseOffset;
+  const CXXRecordDecl *OverriderRD = Overrider.Method->getParent();
+  const CXXRecordDecl *OverriderVBase = 0;
+  if (OverriderRD != MostDerivedClass) {
+    OverriderVBase =
+        ComputeBaseOffset(Context, OverriderRD, MostDerivedClass).VirtualBase;
+  }
+
+  // If the final overrider is defined in the same vbase as the initial
+  // declaration, we don't need a vtordisp thunk at all.
+  if (OverriderVBase == WhichVFPtr.getVBaseWithVPtr())
+    return;
+
+  // 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;
 
-  // If the final overrider is defined in either:
-  // - the most derived class or its non-virtual base or
-  // - the same vbase as the initial declaration,
-  // a simple vtordisp thunk will suffice.
-  const CXXRecordDecl *OverriderRD = Overrider.Method->getParent();
+  // 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;
 
-  const CXXRecordDecl *OverriderVBase =
-      ComputeBaseOffset(Context, OverriderRD, MostDerivedClass).VirtualBase;
-  if (!OverriderVBase || OverriderVBase == WhichVFPtr.getVBaseWithVPtr())
+  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();
     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-vtordisps.cpp

struct C : virtual V4 {
   virtual ~C();
   // MANGLING-DAG: @"\01??_EC@simple@@$4PPPPPPPA@M@AEPAXI@Z"
   // MANGLING-DAG: @"\01??_EC@simple@@$4PPPPPPPE@7AEPAXI@Z"
   // MANGLING-DAG: @"\01??_EC@simple@@$4PPPPPPPM@A@AEPAXI@Z"
 };
 
 C c;
 void use(C *obj) { obj->f(); }
+
+class D : B {
+  // CHECK-LABEL: VFTable for 'V2' in 'V3' in 'simple::B' in 'simple::D' (2 entries).
+  // CHECK-NEXT: 0 | void simple::B::f()
+  // CHECK-NEXT:     [this adjustment: vtordisp at -12, -4 non-virtual]
+  // CHECK-NEXT: 1 | simple::D::~D() [scalar deleting]
+  // CHECK-NEXT:     [this adjustment: vtordisp at -12, -8 non-virtual]
+  D();
+  int z;
+
+  // MANGLING-DAG: @"\01?f@B@simple@@$4PPPPPPPE@3AEXXZ"
+};
+
+D::D() {}
+
+struct E : V3 {
+  virtual void f();
+};
+
+struct F : virtual E {
+  // CHECK-LABEL: VFTable for 'Z' in 'V3' in 'simple::E' in 'simple::F' (2 entries).
+  // CHECK-NEXT:   0 | void simple::F::g()
+  // CHECK-NEXT:       [this adjustment: vtordisp at -4, 0 non-virtual]
+  // CHECK-NEXT:   1 | simple::F::~F() [scalar deleting]
+  // CHECK-NEXT:       [this adjustment: vtordisp at -4, 0 non-virtual]
+
+  // CHECK-LABEL: VFTable for 'V2' in 'V3' in 'simple::E' in 'simple::F' (2 entries).
+  // CHECK-NEXT:   0 | void simple::E::f()
+  // CHECK-NEXT:   1 | simple::F::~F() [scalar deleting]
+  // CHECK-NEXT:       [this adjustment: vtordisp at -12, -8 non-virtual]
+
+  F();
+  virtual void g();  // Force a vtordisp.
+  int f;
+
+  // MANGLING-DAG: @"\01?g@F@simple@@$4PPPPPPPM@A@AEXXZ"{{.*}}??_EF@simple@@$4PPPPPPPM@A@AEPAXI@Z
+  // MANGLING-DAG: ?f@E@simple@@UAEXXZ{{.*}}??_EF@simple@@$4PPPPPPPE@7AEPAXI@Z
+};
+
+F::F() {}
+
+struct G : F {
+  // CHECK-LABEL: VFTable for 'Z' in 'V3' in 'simple::E' in 'simple::F' in 'simple::G' (2 entries).
+  // CHECK-NEXT:   0 | void simple::F::g()
+  // CHECK-NEXT:       [this adjustment: vtordisp at -4, -4 non-virtual]
+  // CHECK-NEXT:   1 | simple::G::~G() [scalar deleting]
+  // CHECK-NEXT:       [this adjustment: vtordisp at -4, 0 non-virtual]
+
+  // CHECK-LABEL: VFTable for 'V2' in 'V3' in 'simple::E' in 'simple::F' in 'simple::G' (2 entries).
+  // CHECK-NEXT:   0 | void simple::E::f()
+  // CHECK-NEXT:   1 | simple::G::~G() [scalar deleting]
+  // CHECK-NEXT:       [this adjustment: vtordisp at -12, -8 non-virtual]
+
+  G();
+  int g;
+
+  // MANGLING-DAG: @"\01?g@F@simple@@$4PPPPPPPM@3AEXXZ"{{.*}}@"\01??_EG@simple@@$4PPPPPPPM@A@AEPAXI@Z"
+  // MANGLING-DAG: @"\01?f@E@simple@@UAEXXZ"{{.*}}@"\01??_EG@simple@@$4PPPPPPPE@7AEPAXI@Z"
+};
+
+G::G() {}
 }
 
 namespace extended {
 // If a virtual function requires vtordisp adjustment and the final overrider
 // is defined in another vitual base of the most derived class,
 // we need to know two vbase offsets.
 // In this case, we should use the extended form of vtordisp thunks, called
 // vtordispex thunks.
struct A : virtual simple::B {
   int a;
   virtual ~A();
 };
 
 A a;
 void use(A *obj) { delete obj; }
 }
 
+namespace pr19408 {
+// In this test, the vptr used to vcall D::f() is located in the A vbase.
+// The offset of A in different in C and D, so the D vtordisp thunk should
+// adjust "this" so C::f gets the right value.
+struct A {
+  A();
+  virtual void f();
+  int a;
+};
+
+struct B : virtual A {
+  B();
+  int b;
+};
+
+struct C : B {
+  C();
+  virtual void f();
+  int c;
+};
+
+struct D : C {
+  // CHECK-LABEL: VFTable for 'pr19408::A' in 'pr19408::B' in 'pr19408::C' in 'pr19408::D' (1 entry).
+  // CHECK-NEXT:   0 | void pr19408::C::f()
+  // CHECK-NEXT:       [this adjustment: vtordisp at -4, -4 non-virtual]
+
+  // MANGLING-DAG: @"\01?f@C@pr19408@@$4PPPPPPPM@3AEXXZ"
+  D();
+  int d;
+};
+
+D::D() {}
+}
+
 namespace access {
 struct A {
   virtual ~A();
 protected:
   virtual void prot();
 private:
   virtual void priv();
 };