From ba6d4973fc87567d8433bc47c5cedf546f838281 Mon Sep 17 00:00:00 2001 From: Reid Kleckner Date: Mon, 2 Apr 2018 20:00:39 +0000 Subject: [PATCH] Fix some DenseMap use-after-rehash bugs and hoist MethodVFTableLocation This re-lands r328845 with fixes for crbug.com/827810. The initial motiviation was to hoist MethodVFTableLocation to global scope so it could be forward declared. In this patch, I noticed that MicrosoftVTableContext uses some risky patterns. It has methods that return references to data stored in DenseMaps. I've made some of them return by value for trivial structs and I've moved some things into separate allocations. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@329007 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Mangle.h | 4 +- include/clang/AST/VTableBuilder.h | 71 ++++++++++++++++--------------- lib/AST/MicrosoftMangle.cpp | 24 ++++------- lib/AST/VTableBuilder.cpp | 21 ++++----- lib/CodeGen/CGDebugInfo.cpp | 2 +- lib/CodeGen/MicrosoftCXXABI.cpp | 25 +++++------ 6 files changed, 69 insertions(+), 78 deletions(-) diff --git a/include/clang/AST/Mangle.h b/include/clang/AST/Mangle.h index 7a45d88c23..71c78dcfab 100644 --- a/include/clang/AST/Mangle.h +++ b/include/clang/AST/Mangle.h @@ -30,6 +30,7 @@ namespace clang { class CXXDestructorDecl; class CXXMethodDecl; class FunctionDecl; + struct MethodVFTableLocation; class NamedDecl; class ObjCMethodDecl; class StringLiteral; @@ -201,7 +202,8 @@ public: raw_ostream &Out) = 0; virtual void mangleVirtualMemPtrThunk(const CXXMethodDecl *MD, - raw_ostream &) = 0; + const MethodVFTableLocation &ML, + raw_ostream &Out) = 0; virtual void mangleCXXVirtualDisplacementMap(const CXXRecordDecl *SrcRD, const CXXRecordDecl *DstRD, diff --git a/include/clang/AST/VTableBuilder.h b/include/clang/AST/VTableBuilder.h index b0b71e4735..8ac4ffa971 100644 --- a/include/clang/AST/VTableBuilder.h +++ b/include/clang/AST/VTableBuilder.h @@ -479,41 +479,42 @@ struct VirtualBaseInfo { VPtrInfoVector VBPtrPaths; }; +struct MethodVFTableLocation { + /// If nonzero, holds the vbtable index of the virtual base with the vfptr. + uint64_t VBTableIndex; + + /// If nonnull, holds the last vbase which contains the vfptr that the + /// method definition is adjusted to. + const CXXRecordDecl *VBase; + + /// This is the offset of the vfptr from the start of the last vbase, or the + /// complete type if there are no virtual bases. + CharUnits VFPtrOffset; + + /// Method's index in the vftable. + uint64_t Index; + + MethodVFTableLocation() + : VBTableIndex(0), VBase(nullptr), VFPtrOffset(CharUnits::Zero()), + Index(0) {} + + MethodVFTableLocation(uint64_t VBTableIndex, const CXXRecordDecl *VBase, + CharUnits VFPtrOffset, uint64_t Index) + : VBTableIndex(VBTableIndex), VBase(VBase), VFPtrOffset(VFPtrOffset), + Index(Index) {} + + bool operator<(const MethodVFTableLocation &other) const { + if (VBTableIndex != other.VBTableIndex) { + assert(VBase != other.VBase); + return VBTableIndex < other.VBTableIndex; + } + return std::tie(VFPtrOffset, Index) < + std::tie(other.VFPtrOffset, other.Index); + } +}; + class MicrosoftVTableContext : public VTableContextBase { public: - struct MethodVFTableLocation { - /// If nonzero, holds the vbtable index of the virtual base with the vfptr. - uint64_t VBTableIndex; - - /// If nonnull, holds the last vbase which contains the vfptr that the - /// method definition is adjusted to. - const CXXRecordDecl *VBase; - - /// This is the offset of the vfptr from the start of the last vbase, or the - /// complete type if there are no virtual bases. - CharUnits VFPtrOffset; - - /// Method's index in the vftable. - uint64_t Index; - - MethodVFTableLocation() - : VBTableIndex(0), VBase(nullptr), VFPtrOffset(CharUnits::Zero()), - Index(0) {} - - MethodVFTableLocation(uint64_t VBTableIndex, const CXXRecordDecl *VBase, - CharUnits VFPtrOffset, uint64_t Index) - : VBTableIndex(VBTableIndex), VBase(VBase), - VFPtrOffset(VFPtrOffset), Index(Index) {} - - bool operator<(const MethodVFTableLocation &other) const { - if (VBTableIndex != other.VBTableIndex) { - assert(VBase != other.VBase); - return VBTableIndex < other.VBTableIndex; - } - return std::tie(VFPtrOffset, Index) < - std::tie(other.VFPtrOffset, other.Index); - } - }; private: ASTContext &Context; @@ -522,7 +523,7 @@ private: MethodVFTableLocationsTy; MethodVFTableLocationsTy MethodVFTableLocations; - typedef llvm::DenseMap + typedef llvm::DenseMap> VFPtrLocationsMapTy; VFPtrLocationsMapTy VFPtrLocations; @@ -559,7 +560,7 @@ public: const VTableLayout &getVFTableLayout(const CXXRecordDecl *RD, CharUnits VFPtrOffset); - const MethodVFTableLocation &getMethodVFTableLocation(GlobalDecl GD); + MethodVFTableLocation getMethodVFTableLocation(GlobalDecl GD); const ThunkInfoVectorTy *getThunkInfo(GlobalDecl GD) override { // Complete destructors don't have a slot in a vftable, so no thunks needed. diff --git a/lib/AST/MicrosoftMangle.cpp b/lib/AST/MicrosoftMangle.cpp index 785ee3a26e..9d49e0f179 100644 --- a/lib/AST/MicrosoftMangle.cpp +++ b/lib/AST/MicrosoftMangle.cpp @@ -135,7 +135,8 @@ public: bool shouldMangleStringLiteral(const StringLiteral *SL) override; void mangleCXXName(const NamedDecl *D, raw_ostream &Out) override; void mangleVirtualMemPtrThunk(const CXXMethodDecl *MD, - raw_ostream &) override; + const MethodVFTableLocation &ML, + raw_ostream &Out) override; void mangleThunk(const CXXMethodDecl *MD, const ThunkInfo &Thunk, raw_ostream &) override; void mangleCXXDtorThunk(const CXXDestructorDecl *DD, CXXDtorType Type, @@ -297,9 +298,8 @@ public: void mangleMemberDataPointer(const CXXRecordDecl *RD, const ValueDecl *VD); void mangleMemberFunctionPointer(const CXXRecordDecl *RD, const CXXMethodDecl *MD); - void mangleVirtualMemPtrThunk( - const CXXMethodDecl *MD, - const MicrosoftVTableContext::MethodVFTableLocation &ML); + void mangleVirtualMemPtrThunk(const CXXMethodDecl *MD, + const MethodVFTableLocation &ML); void mangleNumber(int64_t Number); void mangleTagTypeKind(TagTypeKind TK); void mangleArtificalTagType(TagTypeKind TK, StringRef UnqualifiedName, @@ -607,7 +607,7 @@ MicrosoftCXXNameMangler::mangleMemberFunctionPointer(const CXXRecordDecl *RD, if (MD->isVirtual()) { MicrosoftVTableContext *VTContext = cast(getASTContext().getVTableContext()); - const MicrosoftVTableContext::MethodVFTableLocation &ML = + MethodVFTableLocation ML = VTContext->getMethodVFTableLocation(GlobalDecl(MD)); mangleVirtualMemPtrThunk(MD, ML); NVOffset = ML.VFPtrOffset.getQuantity(); @@ -644,8 +644,7 @@ MicrosoftCXXNameMangler::mangleMemberFunctionPointer(const CXXRecordDecl *RD, } void MicrosoftCXXNameMangler::mangleVirtualMemPtrThunk( - const CXXMethodDecl *MD, - const MicrosoftVTableContext::MethodVFTableLocation &ML) { + const CXXMethodDecl *MD, const MethodVFTableLocation &ML) { // Get the vftable offset. CharUnits PointerWidth = getASTContext().toCharUnitsFromBits( getASTContext().getTargetInfo().getPointerWidth(0)); @@ -2775,14 +2774,9 @@ static void mangleThunkThisAdjustment(const CXXMethodDecl *MD, } } -void -MicrosoftMangleContextImpl::mangleVirtualMemPtrThunk(const CXXMethodDecl *MD, - raw_ostream &Out) { - MicrosoftVTableContext *VTContext = - cast(getASTContext().getVTableContext()); - const MicrosoftVTableContext::MethodVFTableLocation &ML = - VTContext->getMethodVFTableLocation(GlobalDecl(MD)); - +void MicrosoftMangleContextImpl::mangleVirtualMemPtrThunk( + const CXXMethodDecl *MD, const MethodVFTableLocation &ML, + raw_ostream &Out) { msvc_hashing_ostream MHO(Out); MicrosoftCXXNameMangler Mangler(*this, MHO); Mangler.getStream() << '?'; diff --git a/lib/AST/VTableBuilder.cpp b/lib/AST/VTableBuilder.cpp index a95791ce88..aab2ca7aec 100644 --- a/lib/AST/VTableBuilder.cpp +++ b/lib/AST/VTableBuilder.cpp @@ -2367,8 +2367,6 @@ namespace { class VFTableBuilder { public: - typedef MicrosoftVTableContext::MethodVFTableLocation MethodVFTableLocation; - typedef llvm::DenseMap MethodVFTableLocationsTy; @@ -3544,10 +3542,9 @@ static void computeFullPathsForVFTables(ASTContext &Context, } } -static bool -vfptrIsEarlierInMDC(const ASTRecordLayout &Layout, - const MicrosoftVTableContext::MethodVFTableLocation &LHS, - const MicrosoftVTableContext::MethodVFTableLocation &RHS) { +static bool vfptrIsEarlierInMDC(const ASTRecordLayout &Layout, + const MethodVFTableLocation &LHS, + const MethodVFTableLocation &RHS) { CharUnits L = LHS.VFPtrOffset; CharUnits R = RHS.VFPtrOffset; if (LHS.VBase) @@ -3568,14 +3565,14 @@ void MicrosoftVTableContext::computeVTableRelatedInformation( const VTableLayout::AddressPointsMapTy EmptyAddressPointsMap; { - VPtrInfoVector VFPtrs; - computeVTablePaths(/*ForVBTables=*/false, RD, VFPtrs); - computeFullPathsForVFTables(Context, RD, VFPtrs); + auto VFPtrs = llvm::make_unique(); + computeVTablePaths(/*ForVBTables=*/false, RD, *VFPtrs); + computeFullPathsForVFTables(Context, RD, *VFPtrs); VFPtrLocations[RD] = std::move(VFPtrs); } MethodVFTableLocationsTy NewMethodLocations; - for (const std::unique_ptr &VFPtr : VFPtrLocations[RD]) { + for (const std::unique_ptr &VFPtr : *VFPtrLocations[RD]) { VFTableBuilder Builder(*this, RD, *VFPtr); VFTableIdTy id(RD, VFPtr->FullOffsetInMDC); @@ -3720,7 +3717,7 @@ MicrosoftVTableContext::getVFPtrOffsets(const CXXRecordDecl *RD) { computeVTableRelatedInformation(RD); assert(VFPtrLocations.count(RD) && "Couldn't find vfptr locations"); - return VFPtrLocations[RD]; + return *VFPtrLocations[RD]; } const VTableLayout & @@ -3733,7 +3730,7 @@ MicrosoftVTableContext::getVFTableLayout(const CXXRecordDecl *RD, return *VFTableLayouts[id]; } -const MicrosoftVTableContext::MethodVFTableLocation & +MethodVFTableLocation MicrosoftVTableContext::getMethodVFTableLocation(GlobalDecl GD) { assert(cast(GD.getDecl())->isVirtual() && "Only use this method for virtual methods or dtors"); diff --git a/lib/CodeGen/CGDebugInfo.cpp b/lib/CodeGen/CGDebugInfo.cpp index 42c0210e1d..b6d336a2e8 100644 --- a/lib/CodeGen/CGDebugInfo.cpp +++ b/lib/CodeGen/CGDebugInfo.cpp @@ -1416,7 +1416,7 @@ llvm::DISubprogram *CGDebugInfo::CreateCXXMemberFunction( // deleting dtor. const auto *DD = dyn_cast(Method); GlobalDecl GD = DD ? GlobalDecl(DD, Dtor_Deleting) : GlobalDecl(Method); - MicrosoftVTableContext::MethodVFTableLocation ML = + MethodVFTableLocation ML = CGM.getMicrosoftVTableContext().getMethodVFTableLocation(GD); VIndex = ML.Index; diff --git a/lib/CodeGen/MicrosoftCXXABI.cpp b/lib/CodeGen/MicrosoftCXXABI.cpp index 542e1bf5fa..d5b168d069 100644 --- a/lib/CodeGen/MicrosoftCXXABI.cpp +++ b/lib/CodeGen/MicrosoftCXXABI.cpp @@ -230,7 +230,7 @@ public: getThisArgumentTypeForMethod(const CXXMethodDecl *MD) override { MD = MD->getCanonicalDecl(); if (MD->isVirtual() && !isa(MD)) { - MicrosoftVTableContext::MethodVFTableLocation ML = + MethodVFTableLocation ML = CGM.getMicrosoftVTableContext().getMethodVFTableLocation(MD); // The vbases might be ordered differently in the final overrider object // and the complete object, so the "this" argument may sometimes point to @@ -616,9 +616,8 @@ private: const VBTableGlobals &enumerateVBTables(const CXXRecordDecl *RD); /// \brief Generate a thunk for calling a virtual member function MD. - llvm::Function *EmitVirtualMemPtrThunk( - const CXXMethodDecl *MD, - const MicrosoftVTableContext::MethodVFTableLocation &ML); + llvm::Function *EmitVirtualMemPtrThunk(const CXXMethodDecl *MD, + const MethodVFTableLocation &ML); public: llvm::Type *ConvertMemberPointerType(const MemberPointerType *MPT) override; @@ -1336,7 +1335,7 @@ MicrosoftCXXABI::getVirtualFunctionPrologueThisAdjustment(GlobalDecl GD) { LookupGD = GlobalDecl(DD, Dtor_Deleting); } - MicrosoftVTableContext::MethodVFTableLocation ML = + MethodVFTableLocation ML = CGM.getMicrosoftVTableContext().getMethodVFTableLocation(LookupGD); CharUnits Adjustment = ML.VFPtrOffset; @@ -1385,7 +1384,7 @@ Address MicrosoftCXXABI::adjustThisArgumentForVirtualFunctionCall( // with the base one, so look up the deleting one instead. LookupGD = GlobalDecl(DD, Dtor_Deleting); } - MicrosoftVTableContext::MethodVFTableLocation ML = + MethodVFTableLocation ML = CGM.getMicrosoftVTableContext().getMethodVFTableLocation(LookupGD); CharUnits StaticOffset = ML.VFPtrOffset; @@ -1851,8 +1850,7 @@ CGCallee MicrosoftCXXABI::getVirtualFunctionPointer(CodeGenFunction &CGF, llvm::Value *VTable = CGF.GetVTablePtr(VPtr, Ty, MethodDecl->getParent()); MicrosoftVTableContext &VFTContext = CGM.getMicrosoftVTableContext(); - MicrosoftVTableContext::MethodVFTableLocation ML = - VFTContext.getMethodVFTableLocation(GD); + MethodVFTableLocation ML = VFTContext.getMethodVFTableLocation(GD); // Compute the identity of the most derived class whose virtual table is // located at the MethodVFTableLocation ML. @@ -1937,16 +1935,16 @@ MicrosoftCXXABI::enumerateVBTables(const CXXRecordDecl *RD) { return VBGlobals; } -llvm::Function *MicrosoftCXXABI::EmitVirtualMemPtrThunk( - const CXXMethodDecl *MD, - const MicrosoftVTableContext::MethodVFTableLocation &ML) { +llvm::Function * +MicrosoftCXXABI::EmitVirtualMemPtrThunk(const CXXMethodDecl *MD, + const MethodVFTableLocation &ML) { assert(!isa(MD) && !isa(MD) && "can't form pointers to ctors or virtual dtors"); // Calculate the mangled name. SmallString<256> ThunkName; llvm::raw_svector_ostream Out(ThunkName); - getMangleContext().mangleVirtualMemPtrThunk(MD, Out); + getMangleContext().mangleVirtualMemPtrThunk(MD, ML, Out); // If the thunk has been generated previously, just return it. if (llvm::GlobalValue *GV = CGM.getModule().getNamedValue(ThunkName)) @@ -2760,8 +2758,7 @@ MicrosoftCXXABI::EmitMemberFunctionPointer(const CXXMethodDecl *MD) { FirstField = CGM.GetAddrOfFunction(MD, Ty); } else { auto &VTableContext = CGM.getMicrosoftVTableContext(); - MicrosoftVTableContext::MethodVFTableLocation ML = - VTableContext.getMethodVFTableLocation(MD); + MethodVFTableLocation ML = VTableContext.getMethodVFTableLocation(MD); FirstField = EmitVirtualMemPtrThunk(MD, ML); // Include the vfptr adjustment if the method is in a non-primary vftable. NonVirtualBaseAdjustment += ML.VFPtrOffset; -- 2.40.0