From d950f15ee527009d702853f2d60fe6b4d5344977 Mon Sep 17 00:00:00 2001 From: David Majnemer Date: Thu, 30 Apr 2015 17:15:48 +0000 Subject: [PATCH] [MS ABI] Correctly make paths through covariant virtual bases There can be multiple virtual bases which are on the path to a vfptr when one vbase virtually inherits from another. We should prefer the most derived virtual base which covariantly overrides a method in the vfptr class; if we do not lengthen the path this way, we will end up with too few vftable entries. This fixes PR21073. llvm-svn: 236239 --- clang/lib/AST/VTableBuilder.cpp | 122 ++++++++++++++---- .../microsoft-abi-vtables-return-thunks.cpp | 27 ++++ ...rosoft-abi-vtables-virtual-inheritance.cpp | 2 +- 3 files changed, 124 insertions(+), 27 deletions(-) diff --git a/clang/lib/AST/VTableBuilder.cpp b/clang/lib/AST/VTableBuilder.cpp index 089520debffd..e4ae6f86ee12 100644 --- a/clang/lib/AST/VTableBuilder.cpp +++ b/clang/lib/AST/VTableBuilder.cpp @@ -3451,11 +3451,11 @@ MicrosoftVTableContext::~MicrosoftVTableContext() { /// struct B : virtual A { virtual void f(); }; /// struct C : virtual A, B { virtual void f(); }; /// The path to A's vftable in C should be 'C, B, A', not 'C, A'. -static bool -findPathForVPtr(ASTContext &Context, const ASTRecordLayout &MostDerivedLayout, - const CXXRecordDecl *RD, CharUnits Offset, - llvm::SmallPtrSetImpl &VBasesSeen, - VPtrInfo::BasePath &FullPath, VPtrInfo *Info) { +static bool findPathForVPtr(ASTContext &Context, + const ASTRecordLayout &MostDerivedLayout, + const CXXRecordDecl *RD, CharUnits Offset, + FinalOverriders &Overriders, + VPtrInfo::BasePath &FullPath, VPtrInfo *Info) { if (RD == Info->BaseWithVPtr && Offset == Info->FullOffsetInMDC) { Info->PathToBaseWithVPtr = FullPath; return true; @@ -3463,28 +3463,98 @@ findPathForVPtr(ASTContext &Context, const ASTRecordLayout &MostDerivedLayout, const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD); - // Sort direct bases into non-virtual bases followed by virtual bases. This - // approximates layout order with the exception of classes that do not contain - // vptrs, and those don't affect our results. - SmallVector Bases(RD->bases_begin(), RD->bases_end()); - std::stable_partition(Bases.begin(), Bases.end(), - [](CXXBaseSpecifier bs) { return !bs.isVirtual(); }); - - for (const auto &B : Bases) { - const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl(); - CharUnits NewOffset; - if (!B.isVirtual()) - NewOffset = Offset + Layout.getBaseClassOffset(Base); - else { - if (!VBasesSeen.insert(Base).second) - continue; - NewOffset = MostDerivedLayout.getVBaseClassOffset(Base); - } + auto Recurse = [&](const CXXRecordDecl *Base, CharUnits NewOffset) { FullPath.push_back(Base); - if (findPathForVPtr(Context, MostDerivedLayout, Base, NewOffset, VBasesSeen, + if (findPathForVPtr(Context, MostDerivedLayout, Base, NewOffset, Overriders, FullPath, Info)) return true; + // Adding 'Base' didn't get us to the BaseWithVPtr, pop it off the stack so + // that we can try another. FullPath.pop_back(); + return false; + }; + + // Start with the non-virtual bases so that we correctly create paths to + // virtual bases *through* non-virtual bases. + for (const auto &B : RD->bases()) { + if (B.isVirtual()) + continue; + const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl(); + CharUnits NewOffset = Offset + Layout.getBaseClassOffset(Base); + if (Recurse(Base, NewOffset)) + return true; + } + // None of non-virtual bases got us to the BaseWithVPtr, we need to try the + // virtual bases. + std::set> VBases; + for (const auto &B : RD->bases()) { + if (!B.isVirtual()) + continue; + const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl(); + CharUnits NewOffset = MostDerivedLayout.getVBaseClassOffset(Base); + VBases.insert(std::make_pair(Base, NewOffset)); + } + // No virtual bases, bail out early. + if (VBases.empty()) + return false; + + CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/false, + /*DetectVirtual=*/true); + // All virtual bases which are on the path to the BaseWithVPtr are not equal. + // Specifically, virtual paths which introduce additional covariant thunks + // must be preferred over paths which do not introduce such thunks. + for (const auto *MD : Info->BaseWithVPtr->methods()) { + if (!MD->isVirtual()) + continue; + MD = MD->getCanonicalDecl(); + // Let's find overriders for the BaseWithVPtr where the method is overriden + // with a covariant method. + FinalOverriders::OverriderInfo Overrider = + Overriders.getOverrider(MD, Info->FullOffsetInMDC); + BaseOffset BO = + ComputeReturnAdjustmentBaseOffset(Context, Overrider.Method, MD); + // Skip any overriders which are not return adjusting. + if (BO.isEmpty() || !BO.VirtualBase) + continue; + + // Ok, let's iterate through our virtual bases looking for a base which + // provides a return adjusting overrider for this method. + const CXXRecordDecl *Base = nullptr; + CharUnits NewOffset; + for (auto VBasePair : VBases) { + const CXXRecordDecl *VBase = VBasePair.first; + // There might be a vbase which derives from a vbase which provides a + // covariant override for the method *and* provides its own covariant + // override. + // Because of this, we want to keep climbing up the inheritance lattice + // looking for the most derived virtual base which provides a covariant + // override for the method. + Paths.clear(); + if (!VBase->isDerivedFrom(Base ? Base : Info->BaseWithVPtr, Paths) || + !Paths.getDetectedVirtual()) + continue; + const CXXMethodDecl *VBaseMD = MD->getCorrespondingMethodInClass(VBase); + CharUnits VBaseNewOffset = VBasePair.second; + Overrider = Overriders.getOverrider(VBaseMD, VBaseNewOffset); + BO = ComputeReturnAdjustmentBaseOffset(Context, Overrider.Method, MD); + // Skip any overriders which are not return adjusting. + if (BO.isEmpty() || !BO.VirtualBase) + continue; + Base = VBase; + NewOffset = VBaseNewOffset; + } + + if (Base && Recurse(Base, NewOffset)) + return true; + } + // There are no virtual bases listed as a base specifier which provides a + // covariant override. However, we may still need to go through the virtual + // base to get to BaseWithVPtr. + for (const auto &B : VBases) { + const CXXRecordDecl *Base = B.first; + CharUnits NewOffset = B.second; + if (Recurse(Base, NewOffset)) + return true; } return false; } @@ -3492,13 +3562,13 @@ findPathForVPtr(ASTContext &Context, const ASTRecordLayout &MostDerivedLayout, static void computeFullPathsForVFTables(ASTContext &Context, const CXXRecordDecl *RD, VPtrInfoVector &Paths) { - llvm::SmallPtrSet VBasesSeen; const ASTRecordLayout &MostDerivedLayout = Context.getASTRecordLayout(RD); VPtrInfo::BasePath FullPath; + FinalOverriders Overriders(RD, CharUnits(), RD); + Overriders.dump(); for (VPtrInfo *Info : Paths) { findPathForVPtr(Context, MostDerivedLayout, RD, CharUnits::Zero(), - VBasesSeen, FullPath, Info); - VBasesSeen.clear(); + Overriders, FullPath, Info); FullPath.clear(); } } diff --git a/clang/test/CodeGenCXX/microsoft-abi-vtables-return-thunks.cpp b/clang/test/CodeGenCXX/microsoft-abi-vtables-return-thunks.cpp index 072cb956035f..36c6434d549a 100644 --- a/clang/test/CodeGenCXX/microsoft-abi-vtables-return-thunks.cpp +++ b/clang/test/CodeGenCXX/microsoft-abi-vtables-return-thunks.cpp @@ -128,3 +128,30 @@ C::C() {} // GLOBALS: @"\01?f@B@pr20479@@QAEPAUA@2@XZ" // GLOBALS: @"\01?f@B@pr20479@@UAEPAU12@XZ" } + +namespace pr21073 { +struct A { + virtual A *f(); +}; + +struct B : virtual A { + virtual B *f(); +}; + +struct C : virtual A, virtual B { +// VFTABLES-LABEL: VFTable for 'pr21073::A' in 'pr21073::B' in 'pr21073::C' (2 entries). +// VFTABLES-NEXT: 0 | pr21073::B *pr21073::B::f() +// VFTABLES-NEXT: [return adjustment (to type 'struct pr21073::A *'): vbase #1, 0 non-virtual] +// VFTABLES-NEXT: [this adjustment: 8 non-virtual] +// VFTABLES-NEXT: 1 | pr21073::B *pr21073::B::f() +// VFTABLES-NEXT: [return adjustment (to type 'struct pr21073::B *'): 0 non-virtual] +// VFTABLES-NEXT: [this adjustment: 8 non-virtual] + C(); +}; + +C::C() {} + +// GLOBALS-LABEL: @"\01??_7C@pr21073@@6B@" = linkonce_odr unnamed_addr constant [2 x i8*] +// GLOBALS: @"\01?f@B@pr21073@@WPPPPPPPI@AEPAUA@2@XZ" +// GLOBALS: @"\01?f@B@pr21073@@WPPPPPPPI@AEPAU12@XZ" +} diff --git a/clang/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp b/clang/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp index 83f8114bae99..d600ebbf4c2f 100644 --- a/clang/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp +++ b/clang/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp @@ -423,7 +423,7 @@ void use(T *obj) { obj->f(); } namespace Test10 { struct X : virtual C, virtual A { - // CHECK-LABEL: VFTable for 'A' in 'C' in 'Test10::X' (2 entries). + // CHECK-LABEL: VFTable for 'A' in 'Test10::X' (2 entries). // CHECK-NEXT: 0 | void Test10::X::f() // CHECK-NEXT: 1 | void A::z()