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

[rnk]

lgtm

lib/AST/VTableBuilder.cpp

2795

So this fix (I think I wrote it?) was incorrect. We need to look back at the chain of method overrides in the current vftable, rather than all possible overrides.

2885–2886

I would say something like:

// Once a chain of method overrides adds a return adjusting vftable slot, all subsequent overrides will also use an extra method slot.

Would you agree with that statement?

[timurrrr]

lib/AST/VTableBuilder.cpp

2795

Yep, you wrote it in r198080.
It was pretty much as incorrect as the earlier version that I've written :)
We just had insufficient test coverage to tell if we were right...

2885–2886

Yes, this statement seems to be correct. In my comment I wanted to emphasize that we have to do it even if this is suboptimal vftable-size-wise, but apparently made the main point a bit too complex.

Do you think I should just use your wording or merge the two?

[timurrrr]

OK, I've just used your wording for the comment in r215312.

lib/AST/VTableBuilder.cpp

     /// VFTableIndex - The index in the vftable that this method has.
     const uint64_t VFTableIndex;
 
     /// Shadowed - Indicates if this vftable slot is shadowed by
     /// a slot for a covariant-return override. If so, it shouldn't be printed
     /// 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;
 
   /// MethodInfoMap - The information for all methods in the vftable we're
   /// currently building.
   MethodInfoMapTy MethodInfoMap;
 

       assert(TI.Return.isEmpty() &&
              "Destructor can't have return adjustment!");
       Components.push_back(VTableComponent::MakeDeletingDtor(DD));
     } else {
       Components.push_back(VTableComponent::MakeFunction(MD));
     }
   }
 
-  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,
                   const CXXRecordDecl *LastVBase,
                   BasesSetVectorTy &VisitedBases);
 
   void LayoutVFTable() {
     // FIXME: add support for RTTI when we have proper LLVM support for symbols

     if (MD->isVirtual())
       Groups[J->second].push_back(MD);
   }
 
   for (unsigned I = 0, E = Groups.size(); I != E; ++I)
     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)
       return true;
   }
   return false;
 }
 

 
     FinalOverriders::OverriderInfo Overrider =
         Overriders.getOverrider(MD, Base.getBaseOffset());
     const CXXMethodDecl *OverriderMD = Overrider.Method;
     const CXXMethodDecl *OverriddenMD =
         FindNearestOverriddenMethod(MD, VisitedBases);
 
     ThisAdjustment ThisAdjustmentOffset;
-    bool ReturnAdjustingThunk = false;
+    bool ReturnAdjustingThunk = false, ForceReturnAdjustmentMangling = false;
     CharUnits ThisOffset = ComputeThisOffset(Overrider);
     ThisAdjustmentOffset.NonVirtual =
         (ThisOffset - WhichVFPtr.FullOffsetInMDC).getQuantity();
     if ((OverriddenMD || OverriderMD != MD) &&
         WhichVFPtr.getVBaseWithVPtr())
       CalculateVtordispAdjustment(Overrider, ThisOffset, ThisAdjustmentOffset);
 
     if (OverriddenMD) {
       // If MD overrides anything in this vftable, we need to update the entries.
       MethodInfoMapTy::iterator OverriddenMDIterator =
           MethodInfoMap.find(OverriddenMD);
 
       // If the overridden method went to a different vftable, skip it.
       if (OverriddenMDIterator == MethodInfoMap.end())
         continue;
 
       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.
+      // Interestingly, a new vftable slot is also created if OverriddenMD
+      // itself required an extra slot due to return adjustment.
+      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,
                       OverriddenMethodInfo.VFTableIndex);
         MethodInfoMap.erase(OverriddenMDIterator);
 
         assert(!MethodInfoMap.count(MD) &&
                "Should not have method info for this method yet!");
         MethodInfoMap.insert(std::make_pair(MD, MI));
         continue;
       }
 
       // In case we need a return adjustment, we'll add a new slot for
       // the overrider. Mark the overriden method as shadowed by the new slot.
       OverriddenMethodInfo.Shadowed = true;
 
       // 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()) {
       // Skip methods that don't belong to the vftable of the current class,
       // e.g. each method that wasn't seen in any of the visited sub-bases
       // but overrides multiple methods of other sub-bases.
       continue;
     }
 
     // If we got here, MD is a method not seen in any of the sub-bases or
     // it requires return adjustment. Insert the method info for this method.
     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!");
     MethodInfoMap.insert(std::make_pair(MD, MI));
 
     // Check if this overrider needs a return adjustment.
     // We don't want to do this for pure virtual member functions.
     BaseOffset ReturnAdjustmentOffset;
     ReturnAdjustment ReturnAdjustment;
     if (!OverriderMD->isPure()) {
       ReturnAdjustmentOffset =
           ComputeReturnAdjustmentBaseOffset(Context, OverriderMD, MD);
     }
     if (!ReturnAdjustmentOffset.isEmpty()) {
-      ReturnAdjustingThunk = true;
+      ForceReturnAdjustmentMangling = true;
       ReturnAdjustment.NonVirtual =
           ReturnAdjustmentOffset.NonVirtualOffset.getQuantity();
       if (ReturnAdjustmentOffset.VirtualBase) {
         const ASTRecordLayout &DerivedLayout =
             Context.getASTRecordLayout(ReturnAdjustmentOffset.DerivedClass);
         ReturnAdjustment.Virtual.Microsoft.VBPtrOffset =
             DerivedLayout.getVBPtrOffset().getQuantity();
         ReturnAdjustment.Virtual.Microsoft.VBIndex =
             VTables.getVBTableIndex(ReturnAdjustmentOffset.DerivedClass,
                                     ReturnAdjustmentOffset.VirtualBase);
       }
     }
 
-    AddMethod(OverriderMD, ThunkInfo(ThisAdjustmentOffset, ReturnAdjustment,
-                                     ReturnAdjustingThunk ? MD : nullptr));
+    AddMethod(OverriderMD,
+              ThunkInfo(ThisAdjustmentOffset, ReturnAdjustment,
+                        ForceReturnAdjustmentMangling ? MD : nullptr));
   }
 }
 
 static void PrintBasePath(const VPtrInfo::BasePath &Path, raw_ostream &Out) {
   for (VPtrInfo::BasePath::const_reverse_iterator I = Path.rbegin(),
        E = Path.rend(); I != E; ++I) {
     Out << "'";
     (*I)->printQualifiedName(Out);

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

   // CHECK-NEXT:   -- accessible via vfptr at offset 4 --
   // CHECK-NEXT:   3 | test6::F *test6::X::foo()
 
   virtual F* foo();
 };
 
 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(); }
+}