From d010d9ad7e2eb77083e433a2070993f5cc28b555 Mon Sep 17 00:00:00 2001 From: David Majnemer Date: Mon, 2 Nov 2015 09:01:44 +0000 Subject: [PATCH] [MS ABI] Don't zero-initialize vbptrs in bases Certain CXXConstructExpr nodes require zero-initialization before a constructor is called. We had a bug in the case where the constructor is called on a virtual base: we zero-initialized the base's vbptr field. A complementary bug is present in MSVC where no zero-initialization occurs for the subobject at all. This fixes PR25370. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@251783 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/CodeGen/CGCXXABI.cpp | 4 ++ lib/CodeGen/CGCXXABI.h | 3 + lib/CodeGen/CGExprCXX.cpp | 70 +++++++++++++++---- lib/CodeGen/MicrosoftCXXABI.cpp | 19 +++++ .../microsoft-abi-virtual-inheritance.cpp | 26 +++++++ 5 files changed, 108 insertions(+), 14 deletions(-) diff --git a/lib/CodeGen/CGCXXABI.cpp b/lib/CodeGen/CGCXXABI.cpp index 51f1b0730b..078b98d40d 100644 --- a/lib/CodeGen/CGCXXABI.cpp +++ b/lib/CodeGen/CGCXXABI.cpp @@ -326,3 +326,7 @@ CGCXXABI::emitTerminateForUnexpectedException(CodeGenFunction &CGF, CatchTypeInfo CGCXXABI::getCatchAllTypeInfo() { return CatchTypeInfo{nullptr, 0}; } + +std::vector CGCXXABI::getVBPtrOffsets(const CXXRecordDecl *RD) { + return std::vector(); +} diff --git a/lib/CodeGen/CGCXXABI.h b/lib/CodeGen/CGCXXABI.h index dc4d6d3827..7667361f7b 100644 --- a/lib/CodeGen/CGCXXABI.h +++ b/lib/CodeGen/CGCXXABI.h @@ -425,6 +425,9 @@ public: virtual size_t getSrcArgforCopyCtor(const CXXConstructorDecl *, FunctionArgList &Args) const = 0; + /// Gets the offsets of all the virtual base pointers in a given class. + virtual std::vector getVBPtrOffsets(const CXXRecordDecl *RD); + /// Gets the pure virtual member call function. virtual StringRef GetPureVirtualCallName() = 0; diff --git a/lib/CodeGen/CGExprCXX.cpp b/lib/CodeGen/CGExprCXX.cpp index 71e4b98986..47ee1b5872 100644 --- a/lib/CodeGen/CGExprCXX.cpp +++ b/lib/CodeGen/CGExprCXX.cpp @@ -15,6 +15,7 @@ #include "CGCUDARuntime.h" #include "CGCXXABI.h" #include "CGDebugInfo.h" +#include "CGRecordLayout.h" #include "CGObjCRuntime.h" #include "clang/CodeGen/CGFunctionInfo.h" #include "clang/Frontend/CodeGenOptions.h" @@ -356,7 +357,35 @@ static void EmitNullBaseClassInitialization(CodeGenFunction &CGF, DestPtr = CGF.Builder.CreateElementBitCast(DestPtr, CGF.Int8Ty); const ASTRecordLayout &Layout = CGF.getContext().getASTRecordLayout(Base); - llvm::Value *SizeVal = CGF.CGM.getSize(Layout.getNonVirtualSize()); + CharUnits NVSize = Layout.getNonVirtualSize(); + + // We cannot simply zero-initialize the entire base sub-object if vbptrs are + // present, they are initialized by the most derived class before calling the + // constructor. + SmallVector, 1> Stores; + Stores.emplace_back(CharUnits::Zero(), NVSize); + + // Each store is split by the existence of a vbptr. + CharUnits VBPtrWidth = CGF.getPointerSize(); + std::vector VBPtrOffsets = + CGF.CGM.getCXXABI().getVBPtrOffsets(Base); + for (CharUnits VBPtrOffset : VBPtrOffsets) { + std::pair LastStore = Stores.pop_back_val(); + CharUnits LastStoreOffset = LastStore.first; + CharUnits LastStoreSize = LastStore.second; + + CharUnits SplitBeforeOffset = LastStoreOffset; + CharUnits SplitBeforeSize = VBPtrOffset - SplitBeforeOffset; + assert(!SplitBeforeSize.isNegative() && "negative store size!"); + if (!SplitBeforeSize.isZero()) + Stores.emplace_back(SplitBeforeOffset, SplitBeforeSize); + + CharUnits SplitAfterOffset = VBPtrOffset + VBPtrWidth; + CharUnits SplitAfterSize = LastStoreSize - SplitAfterOffset; + assert(!SplitAfterSize.isNegative() && "negative store size!"); + if (!SplitAfterSize.isZero()) + Stores.emplace_back(SplitAfterOffset, SplitAfterSize); + } // If the type contains a pointer to data member we can't memset it to zero. // Instead, create a null constant and copy it to the destination. @@ -364,14 +393,12 @@ static void EmitNullBaseClassInitialization(CodeGenFunction &CGF, // like -1, which happens to be the pattern used by member-pointers. // TODO: isZeroInitializable can be over-conservative in the case where a // virtual base contains a member pointer. - if (!CGF.CGM.getTypes().isZeroInitializable(Base)) { - llvm::Constant *NullConstant = CGF.CGM.EmitNullConstantForBase(Base); - - llvm::GlobalVariable *NullVariable = - new llvm::GlobalVariable(CGF.CGM.getModule(), NullConstant->getType(), - /*isConstant=*/true, - llvm::GlobalVariable::PrivateLinkage, - NullConstant, Twine()); + llvm::Constant *NullConstantForBase = CGF.CGM.EmitNullConstantForBase(Base); + if (!NullConstantForBase->isNullValue()) { + llvm::GlobalVariable *NullVariable = new llvm::GlobalVariable( + CGF.CGM.getModule(), NullConstantForBase->getType(), + /*isConstant=*/true, llvm::GlobalVariable::PrivateLinkage, + NullConstantForBase, Twine()); CharUnits Align = std::max(Layout.getNonVirtualAlignment(), DestPtr.getAlignment()); @@ -380,14 +407,29 @@ static void EmitNullBaseClassInitialization(CodeGenFunction &CGF, Address SrcPtr = Address(CGF.EmitCastToVoidPtr(NullVariable), Align); // Get and call the appropriate llvm.memcpy overload. - CGF.Builder.CreateMemCpy(DestPtr, SrcPtr, SizeVal); - return; - } - + for (std::pair Store : Stores) { + CharUnits StoreOffset = Store.first; + CharUnits StoreSize = Store.second; + llvm::Value *StoreSizeVal = CGF.CGM.getSize(StoreSize); + CGF.Builder.CreateMemCpy( + CGF.Builder.CreateConstInBoundsByteGEP(DestPtr, StoreOffset), + CGF.Builder.CreateConstInBoundsByteGEP(SrcPtr, StoreOffset), + StoreSizeVal); + } + // Otherwise, just memset the whole thing to zero. This is legal // because in LLVM, all default initializers (other than the ones we just // handled above) are guaranteed to have a bit pattern of all zeros. - CGF.Builder.CreateMemSet(DestPtr, CGF.Builder.getInt8(0), SizeVal); + } else { + for (std::pair Store : Stores) { + CharUnits StoreOffset = Store.first; + CharUnits StoreSize = Store.second; + llvm::Value *StoreSizeVal = CGF.CGM.getSize(StoreSize); + CGF.Builder.CreateMemSet( + CGF.Builder.CreateConstInBoundsByteGEP(DestPtr, StoreOffset), + CGF.Builder.getInt8(0), StoreSizeVal); + } + } } void diff --git a/lib/CodeGen/MicrosoftCXXABI.cpp b/lib/CodeGen/MicrosoftCXXABI.cpp index f90013110e..8ef5331a1a 100644 --- a/lib/CodeGen/MicrosoftCXXABI.cpp +++ b/lib/CodeGen/MicrosoftCXXABI.cpp @@ -90,6 +90,25 @@ public: return 1; } + std::vector getVBPtrOffsets(const CXXRecordDecl *RD) override { + std::vector VBPtrOffsets; + const ASTContext &Context = getContext(); + const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD); + + const VBTableGlobals &VBGlobals = enumerateVBTables(RD); + for (const VPtrInfo *VBT : *VBGlobals.VBTables) { + const ASTRecordLayout &SubobjectLayout = + Context.getASTRecordLayout(VBT->BaseWithVPtr); + CharUnits Offs = VBT->NonVirtualOffset; + Offs += SubobjectLayout.getVBPtrOffset(); + if (VBT->getVBaseWithVPtr()) + Offs += Layout.getVBaseClassOffset(VBT->getVBaseWithVPtr()); + VBPtrOffsets.push_back(Offs); + } + llvm::array_pod_sort(VBPtrOffsets.begin(), VBPtrOffsets.end()); + return VBPtrOffsets; + } + StringRef GetPureVirtualCallName() override { return "_purecall"; } StringRef GetDeletedVirtualCallName() override { return "_purecall"; } diff --git a/test/CodeGenCXX/microsoft-abi-virtual-inheritance.cpp b/test/CodeGenCXX/microsoft-abi-virtual-inheritance.cpp index 9a0b011783..8897a38443 100644 --- a/test/CodeGenCXX/microsoft-abi-virtual-inheritance.cpp +++ b/test/CodeGenCXX/microsoft-abi-virtual-inheritance.cpp @@ -455,3 +455,29 @@ void destroy(E *obj) { } } + +namespace test5 { +// PR25370: Don't zero-initialize vbptrs in virtual bases. +struct A { + virtual void f(); +}; + +struct B : virtual A { + int Field; +}; + +struct C : B { + C(); +}; + +C::C() : B() {} +// CHECK-LABEL: define x86_thiscallcc %"struct.test5::C"* @"\01??0C@test5@@QAE@XZ"( +// CHECK: %[[THIS:.*]] = load %"struct.test5::C"*, %"struct.test5::C"** +// CHECK: br i1 %{{.*}}, label %[[INIT_VBASES:.*]], label %[[SKIP_VBASES:.*]] + +// CHECK: %[[SKIP_VBASES]] +// CHECK: %[[B:.*]] = bitcast %"struct.test5::C"* %[[THIS]] to %"struct.test5::B"* +// CHECK: %[[B_i8:.*]] = bitcast %"struct.test5::B"* %[[B]] to i8* +// CHECK: %[[FIELD:.*]] = getelementptr inbounds i8, i8* %[[B_i8]], i32 4 +// CHECK: call void @llvm.memset.p0i8.i32(i8* %[[FIELD]], i8 0, i32 4, i32 4, i1 false) +} -- 2.40.0