r218285 - Fix a vftable mangling bug
Reid Kleckner
reid at kleckner.net
Mon Sep 22 16:14:46 PDT 2014
Author: rnk
Date: Mon Sep 22 18:14:46 2014
New Revision: 218285
URL: http://llvm.org/viewvc/llvm-project?rev=218285&view=rev
Log:
Fix a vftable mangling bug
We need to walk the class hierarchy twice: once in depth-first base
specifier order for mangling and again in depth-first layout order for
vftable layout.
Vftable layout seems to depend on the full path from the most derived
class to the base containing the vfptr.
Fixes PR21031.
Modified:
cfe/trunk/lib/AST/VTableBuilder.cpp
cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp
Modified: cfe/trunk/lib/AST/VTableBuilder.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/VTableBuilder.cpp?rev=218285&r1=218284&r2=218285&view=diff
==============================================================================
--- cfe/trunk/lib/AST/VTableBuilder.cpp (original)
+++ cfe/trunk/lib/AST/VTableBuilder.cpp Mon Sep 22 18:14:46 2014
@@ -3162,16 +3162,9 @@ void MicrosoftVTableContext::computeVTab
Paths.push_back(new VPtrInfo(RD));
// Recursive case: get all the vbtables from our bases and remove anything
- // that shares a virtual base. Look at non-virtual bases first so we get
- // longer inheritance paths from the derived class to the virtual bases.
- llvm::SmallVector<CXXBaseSpecifier, 10> Bases;
- std::copy_if(RD->bases_begin(), RD->bases_end(), std::back_inserter(Bases),
- [](CXXBaseSpecifier bs) { return !bs.isVirtual(); });
- std::copy_if(RD->bases_begin(), RD->bases_end(), std::back_inserter(Bases),
- [](CXXBaseSpecifier bs) { return bs.isVirtual(); });
-
+ // that shares a virtual base.
llvm::SmallPtrSet<const CXXRecordDecl*, 4> VBasesSeen;
- for (const auto &B : Bases) {
+ for (const auto &B : RD->bases()) {
const CXXRecordDecl *Base = B.getType()->getAsCXXRecordDecl();
if (B.isVirtual() && VBasesSeen.count(Base))
continue;
@@ -3196,10 +3189,6 @@ void MicrosoftVTableContext::computeVTab
if (P->MangledPath.empty() || P->MangledPath.back() != Base)
P->NextBaseToMangle = Base;
- // Keep track of the full path.
- // FIXME: Why do we need this?
- P->PathToBaseWithVPtr.insert(P->PathToBaseWithVPtr.begin(), Base);
-
// Keep track of which vtable the derived class is going to extend with
// new methods or bases. We append to either the vftable of our primary
// base, or the first non-virtual base that has a vbtable.
@@ -3287,6 +3276,59 @@ MicrosoftVTableContext::~MicrosoftVTable
llvm::DeleteContainerSeconds(VBaseInfo);
}
+static bool
+findPathForVPtr(ASTContext &Context, const ASTRecordLayout &MostDerivedLayout,
+ const CXXRecordDecl *RD, CharUnits Offset,
+ llvm::SmallPtrSetImpl<const CXXRecordDecl *> &VBasesSeen,
+ VPtrInfo::BasePath &FullPath, VPtrInfo *Info) {
+ if (RD == Info->BaseWithVPtr && Offset == Info->FullOffsetInMDC) {
+ Info->PathToBaseWithVPtr = FullPath;
+ return true;
+ }
+
+ const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
+
+ // Recurse with non-virtual bases first.
+ // FIXME: Does this need to be in layout order? Virtual bases will be in base
+ // specifier order, which isn't necessarily layout order.
+ SmallVector<CXXBaseSpecifier, 4> 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.count(Base))
+ return false;
+ VBasesSeen.insert(Base);
+ NewOffset = MostDerivedLayout.getVBaseClassOffset(Base);
+ }
+ FullPath.push_back(Base);
+ if (findPathForVPtr(Context, MostDerivedLayout, Base, NewOffset, VBasesSeen,
+ FullPath, Info))
+ return true;
+ FullPath.pop_back();
+ }
+ return false;
+}
+
+static void computeFullPathsForVFTables(ASTContext &Context,
+ const CXXRecordDecl *RD,
+ VPtrInfoVector &Paths) {
+ llvm::SmallPtrSet<const CXXRecordDecl*, 4> VBasesSeen;
+ const ASTRecordLayout &MostDerivedLayout = Context.getASTRecordLayout(RD);
+ VPtrInfo::BasePath FullPath;
+ for (VPtrInfo *Info : Paths) {
+ findPathForVPtr(Context, MostDerivedLayout, RD, CharUnits::Zero(),
+ VBasesSeen, FullPath, Info);
+ VBasesSeen.clear();
+ FullPath.clear();
+ }
+}
+
void MicrosoftVTableContext::computeVTableRelatedInformation(
const CXXRecordDecl *RD) {
assert(RD->isDynamicClass());
@@ -3299,6 +3341,7 @@ void MicrosoftVTableContext::computeVTab
VPtrInfoVector *VFPtrs = new VPtrInfoVector();
computeVTablePaths(/*ForVBTables=*/false, RD, *VFPtrs);
+ computeFullPathsForVFTables(Context, RD, *VFPtrs);
VFPtrLocations[RD] = VFPtrs;
MethodVFTableLocationsTy NewMethodLocations;
Modified: cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp?rev=218285&r1=218284&r2=218285&view=diff
==============================================================================
--- cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/microsoft-abi-vtables-virtual-inheritance.cpp Mon Sep 22 18:14:46 2014
@@ -573,10 +573,6 @@ struct Q {
// PR19172: Yet another diamond we miscompiled.
struct R : virtual Q, X {
- // CHECK-LABEL: VFTable for 'vdtors::X' in 'vdtors::R' (2 entries).
- // CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
- // CHECK-NEXT: 1 | void vdtors::X::zzz()
-
// CHECK-LABEL: VFTable for 'vdtors::Q' in 'vdtors::R' (1 entry).
// CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
// CHECK-NEXT: [this adjustment: -8 non-virtual]
@@ -584,6 +580,10 @@ struct R : virtual Q, X {
// CHECK-LABEL: Thunks for 'vdtors::R::~R()' (1 entry).
// CHECK-NEXT: 0 | [this adjustment: -8 non-virtual]
+ // CHECK-LABEL: VFTable for 'vdtors::X' in 'vdtors::R' (2 entries).
+ // CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
+ // CHECK-NEXT: 1 | void vdtors::X::zzz()
+
// CHECK-LABEL: VFTable indices for 'vdtors::R' (1 entry).
// CHECK-NEXT: 0 | vdtors::R::~R() [scalar deleting]
virtual ~R();
@@ -694,10 +694,10 @@ struct X : virtual B {};
struct Y : virtual X, B {
Y();
- // CHECK-LABEL: VFTable for 'B' in 'pr19066::Y' (1 entry).
+ // CHECK-LABEL: VFTable for 'B' in 'pr19066::X' in 'pr19066::Y' (1 entry).
// CHECK-NEXT: 0 | void B::g()
- // CHECK-LABEL: VFTable for 'B' in 'pr19066::X' in 'pr19066::Y' (1 entry).
+ // CHECK-LABEL: VFTable for 'B' in 'pr19066::Y' (1 entry).
// CHECK-NEXT: 0 | void B::g()
};
@@ -772,3 +772,36 @@ struct __declspec(dllexport) A {
virtual void f() = delete;
};
}
+
+namespace pr21031_1 {
+// This ordering of base specifiers regressed in r202425.
+struct A { virtual void f(void); };
+struct B : virtual A { virtual void g(void); };
+struct C : virtual A, B { C(); };
+C::C() {}
+
+// CHECK-LABEL: VFTable for 'pr21031_1::A' in 'pr21031_1::B' in 'pr21031_1::C' (1 entry)
+// CHECK-NEXT: 0 | void pr21031_1::A::f()
+
+// CHECK-LABEL: VFTable for 'pr21031_1::B' in 'pr21031_1::C' (1 entry)
+// CHECK-NEXT: 0 | void pr21031_1::B::g()
+
+// MANGLING-DAG: @"\01??_7C at pr21031_1@@6BB at 1@@" = {{.*}} constant [1 x i8*]
+// MANGLING-DAG: @"\01??_7C at pr21031_1@@6B@" = {{.*}} constant [1 x i8*]
+}
+
+namespace pr21031_2 {
+struct A { virtual void f(void); };
+struct B : virtual A { virtual void g(void); };
+struct C : B, virtual A { C(); };
+C::C() {}
+
+// CHECK-LABEL: VFTable for 'pr21031_2::B' in 'pr21031_2::C' (1 entry)
+// CHECK-NEXT: 0 | void pr21031_2::B::g()
+
+// CHECK-LABEL: VFTable for 'pr21031_2::A' in 'pr21031_2::B' in 'pr21031_2::C' (1 entry)
+// CHECK-NEXT: 0 | void pr21031_2::A::f()
+
+// MANGLING-DAG: @"\01??_7C at pr21031_2@@6BA at 1@@" = {{.*}} constant [1 x i8*]
+// MANGLING-DAG: @"\01??_7C at pr21031_2@@6BB at 1@@" = {{.*}} constant [1 x i8*]
+}
More information about the cfe-commits
mailing list