From 6494f1f34e7eb6a5cc71c7ae808d4728bba13e3e Mon Sep 17 00:00:00 2001 From: Vassil Vassilev Date: Wed, 3 Mar 2021 21:39:04 +0000 Subject: [PATCH] Reland another version of "The Sema::LookupConstructor is not iteration safe." When looking up a ctor the modules infrasturcture deserializes more ctor candidates in the body of the function causing the internal vector implementation to rellocate and invalidate the pointers. This patch makes Sema::LookupConstructor void and stabilizes the iteration. This patch is a previous version of the patch in https://reviews.llvm.org/D91524 and we, after being merged, we should be able to backport it. --- core/metacling/src/TClingMethodInfo.cxx | 4 +++- .../include/clang/AST/DeclContextInternals.h | 2 +- .../src/tools/clang/include/clang/Sema/Sema.h | 3 ++- .../tools/clang/lib/Sema/SemaCodeComplete.cpp | 4 +++- .../src/tools/clang/lib/Sema/SemaDeclCXX.cpp | 4 +++- .../src/tools/clang/lib/Sema/SemaExprCXX.cpp | 8 ++++++-- .../src/tools/clang/lib/Sema/SemaInit.cpp | 20 ++++++++++++------- .../src/tools/clang/lib/Sema/SemaLookup.cpp | 13 +++++++++--- .../src/tools/clang/lib/Sema/SemaOverload.cpp | 8 ++++++-- .../src/tools/clang/lib/Sema/SemaTemplate.cpp | 4 +++- 10 files changed, 50 insertions(+), 20 deletions(-) diff --git a/core/metacling/src/TClingMethodInfo.cxx b/core/metacling/src/TClingMethodInfo.cxx index 2859f48c7f02b..9a08d1096808d 100644 --- a/core/metacling/src/TClingMethodInfo.cxx +++ b/core/metacling/src/TClingMethodInfo.cxx @@ -271,7 +271,9 @@ TClingMethodInfo::TClingMethodInfo(cling::Interpreter *interp, // Assemble special functions (or FunctionTemplate-s) that are synthesized from DefinitionData but // won't be enumerated as part of decls_begin()/decls_end(). - for (clang::NamedDecl *ctor : SemaRef.LookupConstructors(CXXRD)) { + llvm::SmallVector Ctors; + SemaRef.LookupConstructors(CXXRD, Ctors); + for (clang::NamedDecl *ctor : Ctors) { // Filter out constructor templates, they are not functions we can iterate over: if (auto *CXXCD = llvm::dyn_cast(ctor)) SpecFuncs.emplace_back(CXXCD); diff --git a/interpreter/llvm/src/tools/clang/include/clang/AST/DeclContextInternals.h b/interpreter/llvm/src/tools/clang/include/clang/AST/DeclContextInternals.h index f570112579f41..c80e13fc7cbcf 100644 --- a/interpreter/llvm/src/tools/clang/include/clang/AST/DeclContextInternals.h +++ b/interpreter/llvm/src/tools/clang/include/clang/AST/DeclContextInternals.h @@ -33,7 +33,7 @@ class DependentDiagnostic; /// one entry. struct StoredDeclsList { /// When in vector form, this is what the Data pointer points to. - using DeclsTy = SmallVector; + using DeclsTy = SmallVector; /// A collection of declarations, with a flag to indicate if we have /// further external declarations. diff --git a/interpreter/llvm/src/tools/clang/include/clang/Sema/Sema.h b/interpreter/llvm/src/tools/clang/include/clang/Sema/Sema.h index 452deb4dfdcc3..26f3d669952bd 100644 --- a/interpreter/llvm/src/tools/clang/include/clang/Sema/Sema.h +++ b/interpreter/llvm/src/tools/clang/include/clang/Sema/Sema.h @@ -3463,7 +3463,8 @@ class Sema { LabelDecl *LookupOrCreateLabel(IdentifierInfo *II, SourceLocation IdentLoc, SourceLocation GnuLabelLoc = SourceLocation()); - DeclContextLookupResult LookupConstructors(CXXRecordDecl *Class); + void LookupConstructors(CXXRecordDecl *Class, + llvm::SmallVectorImpl &Constructors); CXXConstructorDecl *LookupDefaultConstructor(CXXRecordDecl *Class); CXXConstructorDecl *LookupCopyingConstructor(CXXRecordDecl *Class, unsigned Quals); diff --git a/interpreter/llvm/src/tools/clang/lib/Sema/SemaCodeComplete.cpp b/interpreter/llvm/src/tools/clang/lib/Sema/SemaCodeComplete.cpp index e4bbee86e3502..16e0c41d277c7 100644 --- a/interpreter/llvm/src/tools/clang/lib/Sema/SemaCodeComplete.cpp +++ b/interpreter/llvm/src/tools/clang/lib/Sema/SemaCodeComplete.cpp @@ -5212,7 +5212,9 @@ QualType Sema::ProduceConstructorSignatureHelp(Scope *S, QualType Type, OverloadCandidateSet CandidateSet(Loc, OverloadCandidateSet::CSK_Normal); - for (NamedDecl *C : LookupConstructors(RD)) { + llvm::SmallVector Ctors; + LookupConstructors(RD, Ctors); + for (NamedDecl *C : Ctors) { if (auto *FD = dyn_cast(C)) { AddOverloadCandidate(FD, DeclAccessPair::make(FD, C->getAccess()), Args, CandidateSet, diff --git a/interpreter/llvm/src/tools/clang/lib/Sema/SemaDeclCXX.cpp b/interpreter/llvm/src/tools/clang/lib/Sema/SemaDeclCXX.cpp index bf7627252507f..13dc68e9bd3e7 100644 --- a/interpreter/llvm/src/tools/clang/lib/Sema/SemaDeclCXX.cpp +++ b/interpreter/llvm/src/tools/clang/lib/Sema/SemaDeclCXX.cpp @@ -10162,7 +10162,9 @@ NamedDecl *Sema::BuildUsingDeclaration( UsingName.setName(Context.DeclarationNames.getCXXConstructorName( Context.getCanonicalType(Context.getRecordType(CurClass)))); UsingName.setNamedTypeInfo(nullptr); - for (auto *Ctor : LookupConstructors(RD)) + llvm::SmallVector Ctors; + LookupConstructors(RD, Ctors); + for (auto *Ctor : Ctors) R.addDecl(Ctor); R.resolveKind(); } else { diff --git a/interpreter/llvm/src/tools/clang/lib/Sema/SemaExprCXX.cpp b/interpreter/llvm/src/tools/clang/lib/Sema/SemaExprCXX.cpp index cfb3754d15bbc..e82e32109026e 100644 --- a/interpreter/llvm/src/tools/clang/lib/Sema/SemaExprCXX.cpp +++ b/interpreter/llvm/src/tools/clang/lib/Sema/SemaExprCXX.cpp @@ -4804,7 +4804,9 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT, bool FoundConstructor = false; unsigned FoundTQs; - for (const auto *ND : Self.LookupConstructors(RD)) { + llvm::SmallVector Ctors; + Self.LookupConstructors(RD, Ctors); + for (const auto *ND : Ctors) { // A template constructor is never a copy constructor. // FIXME: However, it may actually be selected at the actual overload // resolution point. @@ -4845,7 +4847,9 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT, return true; bool FoundConstructor = false; - for (const auto *ND : Self.LookupConstructors(RD)) { + llvm::SmallVector Ctors; + Self.LookupConstructors(RD, Ctors); + for (const auto *ND : Ctors) { // FIXME: In C++0x, a constructor template can be a default constructor. if (isa(ND->getUnderlyingDecl())) continue; diff --git a/interpreter/llvm/src/tools/clang/lib/Sema/SemaInit.cpp b/interpreter/llvm/src/tools/clang/lib/Sema/SemaInit.cpp index 60f34775c6b2d..75b4007214c46 100644 --- a/interpreter/llvm/src/tools/clang/lib/Sema/SemaInit.cpp +++ b/interpreter/llvm/src/tools/clang/lib/Sema/SemaInit.cpp @@ -3735,7 +3735,7 @@ ResolveConstructorOverload(Sema &S, SourceLocation DeclLoc, MultiExprArg Args, OverloadCandidateSet &CandidateSet, QualType DestType, - DeclContext::lookup_result Ctors, + const llvm::SmallVectorImpl& Ctors, OverloadCandidateSet::iterator &Best, bool CopyInitializing, bool AllowExplicit, bool OnlyListConstructors, bool IsListInit, @@ -3915,7 +3915,8 @@ static void TryConstructorInitialization(Sema &S, // - Otherwise, if T is a class type, constructors are considered. The // applicable constructors are enumerated, and the best one is chosen // through overload resolution. - DeclContext::lookup_result Ctors = S.LookupConstructors(DestRecordDecl); + llvm::SmallVector Ctors; + S.LookupConstructors(DestRecordDecl, Ctors); OverloadingResult Result = OR_No_Viable_Function; OverloadCandidateSet::iterator Best; @@ -4370,7 +4371,9 @@ static OverloadingResult TryRefInitWithConversionFunction( // to see if there is a suitable conversion. CXXRecordDecl *T1RecordDecl = cast(T1RecordType->getDecl()); - for (NamedDecl *D : S.LookupConstructors(T1RecordDecl)) { + llvm::SmallVector Ctors; + S.LookupConstructors(T1RecordDecl, Ctors); + for (NamedDecl *D : Ctors) { auto Info = getConstructorInfo(D); if (!Info.Constructor) continue; @@ -5014,7 +5017,9 @@ static void TryUserDefinedConversion(Sema &S, // Try to complete the type we're converting to. if (S.isCompleteType(Kind.getLocation(), DestType)) { - for (NamedDecl *D : S.LookupConstructors(DestRecordDecl)) { + llvm::SmallVector Ctors; + S.LookupConstructors(DestRecordDecl, Ctors); + for (NamedDecl *D : Ctors) { auto Info = getConstructorInfo(D); if (!Info.Constructor) continue; @@ -5987,7 +5992,8 @@ static ExprResult CopyObject(Sema &S, // C++11 [dcl.init]p16, second bullet for class types, this initialization // is direct-initialization. OverloadCandidateSet CandidateSet(Loc, OverloadCandidateSet::CSK_Normal); - DeclContext::lookup_result Ctors = S.LookupConstructors(Class); + llvm::SmallVector Ctors; + S.LookupConstructors(Class, Ctors); OverloadCandidateSet::iterator Best; switch (ResolveConstructorOverload( @@ -6129,8 +6135,8 @@ static void CheckCXX98CompatAccessibleCopy(Sema &S, // Find constructors which would have been considered. OverloadCandidateSet CandidateSet(Loc, OverloadCandidateSet::CSK_Normal); - DeclContext::lookup_result Ctors = - S.LookupConstructors(cast(Record->getDecl())); + llvm::SmallVector Ctors; + S.LookupConstructors(cast(Record->getDecl()), Ctors); // Perform overload resolution. OverloadCandidateSet::iterator Best; diff --git a/interpreter/llvm/src/tools/clang/lib/Sema/SemaLookup.cpp b/interpreter/llvm/src/tools/clang/lib/Sema/SemaLookup.cpp index 2958dd1412f99..aa5d2f34c54ab 100644 --- a/interpreter/llvm/src/tools/clang/lib/Sema/SemaLookup.cpp +++ b/interpreter/llvm/src/tools/clang/lib/Sema/SemaLookup.cpp @@ -3214,7 +3214,8 @@ CXXConstructorDecl *Sema::LookupMovingConstructor(CXXRecordDecl *Class, } /// Look up the constructors for the given class. -DeclContext::lookup_result Sema::LookupConstructors(CXXRecordDecl *Class) { +void Sema::LookupConstructors(CXXRecordDecl *Class, + llvm::SmallVectorImpl &Constructors) { // If the implicit constructors have not yet been declared, do so now. if (CanDeclareSpecialMemberFunction(Class)) { if (Class->needsImplicitDefaultConstructor()) @@ -3227,7 +3228,10 @@ DeclContext::lookup_result Sema::LookupConstructors(CXXRecordDecl *Class) { CanQualType T = Context.getCanonicalType(Context.getTypeDeclType(Class)); DeclarationName Name = Context.DeclarationNames.getCXXConstructorName(T); - return Class->lookup(Name); + // Working directly on R might trigger a deserialization, invalidating R if + // the underlying data structure needs to reallocate the storage. + DeclContext::lookup_result R = Class->lookup(Name); + Constructors.append(R.begin(), R.end()); } /// Look up the copying assignment operator for the given class. @@ -3462,7 +3466,10 @@ void Sema::ArgumentDependentLookup(DeclarationName Name, SourceLocation Loc, // namespaces even if they are not visible during an ordinary // lookup (11.4). DeclContext::lookup_result R = NS->lookup(Name); - for (auto *D : R) { + // The loop might trigger a deserialization, invalidating R if the + // underlying data structure needs to reallocate the storage. + llvm::SmallVector RCopy(R.begin(), R.end()); + for (auto *D : RCopy) { auto *Underlying = D; if (auto *USD = dyn_cast(D)) Underlying = USD->getTargetDecl(); diff --git a/interpreter/llvm/src/tools/clang/lib/Sema/SemaOverload.cpp b/interpreter/llvm/src/tools/clang/lib/Sema/SemaOverload.cpp index f632a4d3bd1a7..b176a9c6a794f 100644 --- a/interpreter/llvm/src/tools/clang/lib/Sema/SemaOverload.cpp +++ b/interpreter/llvm/src/tools/clang/lib/Sema/SemaOverload.cpp @@ -3230,7 +3230,9 @@ IsInitializerListConstructorConversion(Sema &S, Expr *From, QualType ToType, OverloadCandidateSet &CandidateSet, bool AllowExplicit) { CandidateSet.clear(OverloadCandidateSet::CSK_InitByUserDefinedConversion); - for (auto *D : S.LookupConstructors(To)) { + llvm::SmallVector Ctors; + S.LookupConstructors(To, Ctors); + for (auto *D : Ctors) { auto Info = getConstructorInfo(D); if (!Info) continue; @@ -3353,7 +3355,9 @@ IsUserDefinedConversion(Sema &S, Expr *From, QualType ToType, ListInitializing = true; } - for (auto *D : S.LookupConstructors(ToRecordDecl)) { + llvm::SmallVector Ctors; + S.LookupConstructors(ToRecordDecl, Ctors); + for (auto *D : Ctors) { auto Info = getConstructorInfo(D); if (!Info) continue; diff --git a/interpreter/llvm/src/tools/clang/lib/Sema/SemaTemplate.cpp b/interpreter/llvm/src/tools/clang/lib/Sema/SemaTemplate.cpp index 520c89ee9890b..6ada88492fa35 100644 --- a/interpreter/llvm/src/tools/clang/lib/Sema/SemaTemplate.cpp +++ b/interpreter/llvm/src/tools/clang/lib/Sema/SemaTemplate.cpp @@ -2158,7 +2158,9 @@ void Sema::DeclareImplicitDeductionGuides(TemplateDecl *Template, // for which some class template parameter without a default argument never // appears in a deduced context). bool AddedAny = false; - for (NamedDecl *D : LookupConstructors(Transform.Primary)) { + llvm::SmallVector Ctors; + LookupConstructors(Transform.Primary, Ctors); + for (NamedDecl *D : Ctors) { D = D->getUnderlyingDecl(); if (D->isInvalidDecl() || D->isImplicit()) continue;