Fix PR20444 -- wrong number of vftable slots created when return adjustment thunks are needed

Reviewed at http://reviews.llvm.org/D4822

llvm-svn: 215312

Author: timurrrr

clang/lib/AST/VTableBuilder.cpp

@@ -2464,11 +2464,18 @@ class VFTableBuilder {
     /// or used for vcalls in the most derived class.
     bool Shadowed;
 
-    MethodInfo(uint64_t VBTableIndex, uint64_t VFTableIndex)
+    /// UsesExtraSlot - Indicates if this vftable slot was created because
+    /// any of the overridden slots required a return adjusting thunk.
+    bool UsesExtraSlot;
+
+    MethodInfo(uint64_t VBTableIndex, uint64_t VFTableIndex,
+               bool UsesExtraSlot = false)
         : VBTableIndex(VBTableIndex), VFTableIndex(VFTableIndex),
-          Shadowed(false) {}
+          Shadowed(false), UsesExtraSlot(UsesExtraSlot) {}
 
-    MethodInfo() : VBTableIndex(0), VFTableIndex(0), Shadowed(false) {}
+    MethodInfo()
+        : VBTableIndex(0), VFTableIndex(0), Shadowed(false),
+          UsesExtraSlot(false) {}
   };
 
   typedef llvm::DenseMap<const CXXMethodDecl *, MethodInfo> MethodInfoMapTy;
@@ -2525,8 +2532,6 @@ class VFTableBuilder {
     }
   }
 
-  bool NeedsReturnAdjustingThunk(const CXXMethodDecl *MD);
-
   /// AddMethods - Add the methods of this base subobject and the relevant
   /// subbases to the vftable we're currently laying out.
   void AddMethods(BaseSubobject Base, unsigned BaseDepth,
@@ -2789,24 +2794,6 @@ static void GroupNewVirtualOverloads(
     VirtualMethods.append(Groups[I].rbegin(), Groups[I].rend());
 }
 
-/// We need a return adjusting thunk for this method if its return type is
-/// not trivially convertible to the return type of any of its overridden
-/// methods.
-bool VFTableBuilder::NeedsReturnAdjustingThunk(const CXXMethodDecl *MD) {
-  OverriddenMethodsSetTy OverriddenMethods;
-  ComputeAllOverriddenMethods(MD, OverriddenMethods);
-  for (OverriddenMethodsSetTy::iterator I = OverriddenMethods.begin(),
-                                        E = OverriddenMethods.end();
-       I != E; ++I) {
-    const CXXMethodDecl *OverriddenMD = *I;
-    BaseOffset Adjustment =
-        ComputeReturnAdjustmentBaseOffset(Context, MD, OverriddenMD);
-    if (!Adjustment.isEmpty())
-      return true;
-  }
-  return false;
-}
-
 static bool isDirectVBase(const CXXRecordDecl *Base, const CXXRecordDecl *RD) {
   for (const auto &B : RD->bases()) {
     if (B.isVirtual() && B.getType()->getAsCXXRecordDecl() == Base)
@@ -2873,7 +2860,7 @@ void VFTableBuilder::AddMethods(BaseSubobject Base, unsigned BaseDepth,
         FindNearestOverriddenMethod(MD, VisitedBases);
 
     ThisAdjustment ThisAdjustmentOffset;
-    bool ReturnAdjustingThunk = false;
+    bool ReturnAdjustingThunk = false, ForceReturnAdjustmentMangling = false;
     CharUnits ThisOffset = ComputeThisOffset(Overrider);
     ThisAdjustmentOffset.NonVirtual =
         (ThisOffset - WhichVFPtr.FullOffsetInMDC).getQuantity();
@@ -2892,7 +2879,16 @@ void VFTableBuilder::AddMethods(BaseSubobject Base, unsigned BaseDepth,
 
       MethodInfo &OverriddenMethodInfo = OverriddenMDIterator->second;
 
-      if (!NeedsReturnAdjustingThunk(MD)) {
+      // Let's check if the overrider requires any return adjustments.
+      // We must create a new slot if the MD's return type is not trivially
+      // convertible to the OverriddenMD's one.
+      // Once a chain of method overrides adds a return adjusting vftable slot,
+      // all subsequent overrides will also use an extra method slot.
+      ReturnAdjustingThunk = !ComputeReturnAdjustmentBaseOffset(
+                                  Context, MD, OverriddenMD).isEmpty() ||
+                             OverriddenMethodInfo.UsesExtraSlot;
+
+      if (!ReturnAdjustingThunk) {
         // No return adjustment needed - just replace the overridden method info
         // with the current info.
         MethodInfo MI(OverriddenMethodInfo.VBTableIndex,
@@ -2911,7 +2907,7 @@ void VFTableBuilder::AddMethods(BaseSubobject Base, unsigned BaseDepth,
 
       // Force a special name mangling for a return-adjusting thunk
       // unless the method is the final overrider without this adjustment.
-      ReturnAdjustingThunk =
+      ForceReturnAdjustmentMangling =
           !(MD == OverriderMD && ThisAdjustmentOffset.isEmpty());
     } else if (Base.getBaseOffset() != WhichVFPtr.FullOffsetInMDC ||
                MD->size_overridden_methods()) {
@@ -2926,7 +2922,8 @@ void VFTableBuilder::AddMethods(BaseSubobject Base, unsigned BaseDepth,
     unsigned VBIndex =
         LastVBase ? VTables.getVBTableIndex(MostDerivedClass, LastVBase) : 0;
     MethodInfo MI(VBIndex,
-                  HasRTTIComponent ? Components.size() - 1 : Components.size());
+                  HasRTTIComponent ? Components.size() - 1 : Components.size(),
+                  ReturnAdjustingThunk);
 
     assert(!MethodInfoMap.count(MD) &&
            "Should not have method info for this method yet!");
@@ -2941,7 +2938,7 @@ void VFTableBuilder::AddMethods(BaseSubobject Base, unsigned BaseDepth,
           ComputeReturnAdjustmentBaseOffset(Context, OverriderMD, MD);
     }
     if (!ReturnAdjustmentOffset.isEmpty()) {
-      ReturnAdjustingThunk = true;
+      ForceReturnAdjustmentMangling = true;
       ReturnAdjustment.NonVirtual =
           ReturnAdjustmentOffset.NonVirtualOffset.getQuantity();
       if (ReturnAdjustmentOffset.VirtualBase) {
@@ -2955,8 +2952,9 @@ void VFTableBuilder::AddMethods(BaseSubobject Base, unsigned BaseDepth,
       }
     }
 
-    AddMethod(OverriderMD, ThunkInfo(ThisAdjustmentOffset, ReturnAdjustment,
-                                     ReturnAdjustingThunk ? MD : nullptr));
+    AddMethod(OverriderMD,
+              ThunkInfo(ThisAdjustmentOffset, ReturnAdjustment,
+                        ForceReturnAdjustmentMangling ? MD : nullptr));
   }
 }
 

clang/test/CodeGenCXX/microsoft-abi-vtables-multiple-nonvirtual-inheritance-return-adjustment.cpp

@@ -295,3 +295,46 @@ struct X : E {
 
 void build_vftable(X *obj) { obj->foo(); }
 }
+
+namespace pr20444 {
+struct A {
+  virtual A* f();
+};
+struct B {
+  virtual B* f();
+};
+struct C : A, B {
+  virtual C* f();
+  // CHECK-LABEL: VFTable for 'pr20444::A' in 'pr20444::C' (1 entry).
+  // CHECK-NEXT:   0 | pr20444::C *pr20444::C::f()
+
+  // CHECK-LABEL: VFTable for 'pr20444::B' in 'pr20444::C' (2 entries).
+  // CHECK-NEXT:   0 | pr20444::C *pr20444::C::f()
+  // CHECK-NEXT:       [return adjustment (to type 'struct pr20444::B *'): 4 non-virtual]
+  // CHECK-NEXT:       [this adjustment: -4 non-virtual]
+  // CHECK-NEXT:   1 | pr20444::C *pr20444::C::f()
+  // CHECK-NEXT:       [return adjustment (to type 'struct pr20444::C *'): 0 non-virtual]
+  // CHECK-NEXT:       [this adjustment: -4 non-virtual]
+};
+
+void build_vftable(C *obj) { obj->f(); }
+
+struct D : C {
+  virtual D* f();
+  // CHECK-LABEL: VFTable for 'pr20444::A' in 'pr20444::C' in 'pr20444::D' (1 entry).
+  // CHECK-NEXT:   0 | pr20444::D *pr20444::D::f()
+
+  // CHECK-LABEL: VFTable for 'pr20444::B' in 'pr20444::C' in 'pr20444::D' (3 entries).
+  // CHECK-NEXT:   0 | pr20444::D *pr20444::D::f()
+  // CHECK-NEXT:       [return adjustment (to type 'struct pr20444::B *'): 4 non-virtual]
+  // CHECK-NEXT:       [this adjustment: -4 non-virtual]
+  // CHECK-NEXT:   1 | pr20444::D *pr20444::D::f()
+  // CHECK-NEXT:       [return adjustment (to type 'struct pr20444::C *'): 0 non-virtual]
+  // CHECK-NEXT:       [this adjustment: -4 non-virtual]
+  // CHECK-NEXT:   2 | pr20444::D *pr20444::D::f()
+  // CHECK-NEXT:       [return adjustment (to type 'struct pr20444::D *'): 0 non-virtual]
+  // CHECK-NEXT:       [this adjustment: -4 non-virtual]
+};
+
+void build_vftable(D *obj) { obj->f(); }
+}