Cool, thanks! Should we also have a test for using a default arg with a pch? Now that ASTContext doesn't have this table any more, it'll work, but maybe it's good to have a regression test for the issue in the PR?
On Wed, Nov 23, 2016 at 11:51 AM, Reid Kleckner via cfe-commits < cfe-commits@lists.llvm.org> wrote: > Author: rnk > Date: Wed Nov 23 10:51:30 2016 > New Revision: 287774 > > URL: http://llvm.org/viewvc/llvm-project?rev=287774&view=rev > Log: > Remove C++ default arg side table for MS ABI ctor closures > > Summary: > We don't need a side table in ASTContext to hold CXXDefaultArgExprs. The > important part of building the CXXDefaultArgExprs was to ODR use the > default argument expressions, not to make AST nodes. Refactor the code > to only check the default argument, and remove the side table in > ASTContext which wasn't being serialized. > > Fixes PR31121 > > Reviewers: thakis, rsmith, majnemer > > Subscribers: cfe-commits > > Differential Revision: https://reviews.llvm.org/D27007 > > Added: > cfe/trunk/test/SemaCXX/default-arg-closures.cpp > Modified: > cfe/trunk/include/clang/AST/ASTContext.h > cfe/trunk/include/clang/Sema/Sema.h > cfe/trunk/lib/AST/ASTContext.cpp > cfe/trunk/lib/AST/CXXABI.h > cfe/trunk/lib/AST/ItaniumCXXABI.cpp > cfe/trunk/lib/AST/MicrosoftCXXABI.cpp > cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp > cfe/trunk/lib/Sema/SemaDeclCXX.cpp > cfe/trunk/lib/Sema/SemaExpr.cpp > cfe/trunk/lib/Sema/SemaExprCXX.cpp > > Modified: cfe/trunk/include/clang/AST/ASTContext.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/AST/ASTContext.h?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/include/clang/AST/ASTContext.h (original) > +++ cfe/trunk/include/clang/AST/ASTContext.h Wed Nov 23 10:51:30 2016 > @@ -2452,12 +2452,6 @@ public: > void addCopyConstructorForExceptionObject(CXXRecordDecl *RD, > CXXConstructorDecl *CD); > > - void addDefaultArgExprForConstructor(const CXXConstructorDecl *CD, > - unsigned ParmIdx, Expr *DAE); > - > - Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl *CD, > - unsigned ParmIdx); > - > void addTypedefNameForUnnamedTagDecl(TagDecl *TD, TypedefNameDecl > *TND); > > TypedefNameDecl *getTypedefNameForUnnamedTagDecl(const TagDecl *TD); > > Modified: cfe/trunk/include/clang/Sema/Sema.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/ > clang/Sema/Sema.h?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/include/clang/Sema/Sema.h (original) > +++ cfe/trunk/include/clang/Sema/Sema.h Wed Nov 23 10:51:30 2016 > @@ -4408,6 +4408,12 @@ public: > > ExprResult BuildCXXDefaultInitExpr(SourceLocation Loc, FieldDecl > *Field); > > + > + /// Instantiate or parse a C++ default argument expression as necessary. > + /// Return true on error. > + bool CheckCXXDefaultArgExpr(SourceLocation CallLoc, FunctionDecl *FD, > + ParmVarDecl *Param); > + > /// BuildCXXDefaultArgExpr - Creates a CXXDefaultArgExpr, instantiating > /// the default expr if needed. > ExprResult BuildCXXDefaultArgExpr(SourceLocation CallLoc, > > Modified: cfe/trunk/lib/AST/ASTContext.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ > ASTContext.cpp?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/AST/ASTContext.cpp (original) > +++ cfe/trunk/lib/AST/ASTContext.cpp Wed Nov 23 10:51:30 2016 > @@ -9172,18 +9172,6 @@ void ASTContext::addCopyConstructorForEx > cast<CXXConstructorDecl>(CD->getFirstDecl())); > } > > -void ASTContext::addDefaultArgExprForConstructor(const > CXXConstructorDecl *CD, > - unsigned ParmIdx, Expr > *DAE) { > - ABI->addDefaultArgExprForConstructor( > - cast<CXXConstructorDecl>(CD->getFirstDecl()), ParmIdx, DAE); > -} > - > -Expr *ASTContext::getDefaultArgExprForConstructor(const > CXXConstructorDecl *CD, > - unsigned ParmIdx) { > - return ABI->getDefaultArgExprForConstructor( > - cast<CXXConstructorDecl>(CD->getFirstDecl()), ParmIdx); > -} > - > void ASTContext::addTypedefNameForUnnamedTagDecl(TagDecl *TD, > TypedefNameDecl *DD) { > return ABI->addTypedefNameForUnnamedTagDecl(TD, DD); > > Modified: cfe/trunk/lib/AST/CXXABI.h > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ > CXXABI.h?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/AST/CXXABI.h (original) > +++ cfe/trunk/lib/AST/CXXABI.h Wed Nov 23 10:51:30 2016 > @@ -54,12 +54,6 @@ public: > virtual const CXXConstructorDecl * > getCopyConstructorForExceptionObject(CXXRecordDecl *) = 0; > > - virtual void addDefaultArgExprForConstructor(const CXXConstructorDecl > *CD, > - unsigned ParmIdx, Expr > *DAE) = 0; > - > - virtual Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl > *CD, > - unsigned ParmIdx) = 0; > - > virtual void addTypedefNameForUnnamedTagDecl(TagDecl *TD, > TypedefNameDecl *DD) = 0; > > > Modified: cfe/trunk/lib/AST/ItaniumCXXABI.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ > ItaniumCXXABI.cpp?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/AST/ItaniumCXXABI.cpp (original) > +++ cfe/trunk/lib/AST/ItaniumCXXABI.cpp Wed Nov 23 10:51:30 2016 > @@ -142,14 +142,6 @@ public: > void addCopyConstructorForExceptionObject(CXXRecordDecl *RD, > CXXConstructorDecl *CD) > override {} > > - void addDefaultArgExprForConstructor(const CXXConstructorDecl *CD, > - unsigned ParmIdx, Expr *DAE) > override {} > - > - Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl *CD, > - unsigned ParmIdx) override { > - return nullptr; > - } > - > void addTypedefNameForUnnamedTagDecl(TagDecl *TD, > TypedefNameDecl *DD) override {} > > > Modified: cfe/trunk/lib/AST/MicrosoftCXXABI.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ > MicrosoftCXXABI.cpp?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/AST/MicrosoftCXXABI.cpp (original) > +++ cfe/trunk/lib/AST/MicrosoftCXXABI.cpp Wed Nov 23 10:51:30 2016 > @@ -67,8 +67,6 @@ public: > class MicrosoftCXXABI : public CXXABI { > ASTContext &Context; > llvm::SmallDenseMap<CXXRecordDecl *, CXXConstructorDecl *> > RecordToCopyCtor; > - llvm::SmallDenseMap<std::pair<const CXXConstructorDecl *, unsigned>, > Expr *> > - CtorToDefaultArgExpr; > > llvm::SmallDenseMap<TagDecl *, DeclaratorDecl *> > UnnamedTagDeclToDeclaratorDecl; > @@ -92,16 +90,6 @@ public: > llvm_unreachable("unapplicable to the MS ABI"); > } > > - void addDefaultArgExprForConstructor(const CXXConstructorDecl *CD, > - unsigned ParmIdx, Expr *DAE) > override { > - CtorToDefaultArgExpr[std::make_pair(CD, ParmIdx)] = DAE; > - } > - > - Expr *getDefaultArgExprForConstructor(const CXXConstructorDecl *CD, > - unsigned ParmIdx) override { > - return CtorToDefaultArgExpr[std::make_pair(CD, ParmIdx)]; > - } > - > const CXXConstructorDecl * > getCopyConstructorForExceptionObject(CXXRecordDecl *RD) override { > return RecordToCopyCtor[RD]; > > Modified: cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/ > MicrosoftCXXABI.cpp?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp (original) > +++ cfe/trunk/lib/CodeGen/MicrosoftCXXABI.cpp Wed Nov 23 10:51:30 2016 > @@ -3869,11 +3869,11 @@ MicrosoftCXXABI::getAddrOfCXXCtorClosure > Args.add(RValue::get(SrcVal), SrcParam.getType()); > > // Add the rest of the default arguments. > - std::vector<Stmt *> ArgVec; > - for (unsigned I = IsCopy ? 1 : 0, E = CD->getNumParams(); I != E; ++I) { > - Stmt *DefaultArg = getContext().getDefaultArgExprForConstructor(CD, > I); > - assert(DefaultArg && "sema forgot to instantiate default args"); > - ArgVec.push_back(DefaultArg); > + SmallVector<const Stmt *, 4> ArgVec; > + ArrayRef<ParmVarDecl *> params = CD->parameters().drop_front(IsCopy ? > 1 : 0); > + for (const ParmVarDecl *PD : params) { > + assert(PD->hasDefaultArg() && "ctor closure lacks default args"); > + ArgVec.push_back(PD->getDefaultArg()); > } > > CodeGenFunction::RunCleanupsScope Cleanups(CGF); > > Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaDeclCXX.cpp?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original) > +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Wed Nov 23 10:51:30 2016 > @@ -10365,7 +10365,7 @@ void Sema::ActOnFinishCXXMemberDecls() { > } > } > > -static void getDefaultArgExprsForConstructors(Sema &S, CXXRecordDecl > *Class) { > +static void checkDefaultArgExprsForConstructors(Sema &S, CXXRecordDecl > *Class) { > // Don't do anything for template patterns. > if (Class->getDescribedClassTemplate()) > return; > @@ -10379,7 +10379,7 @@ static void getDefaultArgExprsForConstru > if (!CD) { > // Recurse on nested classes. > if (auto *NestedRD = dyn_cast<CXXRecordDecl>(Member)) > - getDefaultArgExprsForConstructors(S, NestedRD); > + checkDefaultArgExprsForConstructors(S, NestedRD); > continue; > } else if (!CD->isDefaultConstructor() || > !CD->hasAttr<DLLExportAttr>()) { > continue; > @@ -10404,14 +10404,9 @@ static void getDefaultArgExprsForConstru > LastExportedDefaultCtor = CD; > > for (unsigned I = 0; I != NumParams; ++I) { > - // Skip any default arguments that we've already instantiated. > - if (S.Context.getDefaultArgExprForConstructor(CD, I)) > - continue; > - > - Expr *DefaultArg = S.BuildCXXDefaultArgExpr(Class->getLocation(), > CD, > - > CD->getParamDecl(I)).get(); > + (void)S.CheckCXXDefaultArgExpr(Class->getLocation(), CD, > + CD->getParamDecl(I)); > S.DiscardCleanupsInEvaluationContext(); > - S.Context.addDefaultArgExprForConstructor(CD, I, DefaultArg); > } > } > } > @@ -10423,7 +10418,7 @@ void Sema::ActOnFinishCXXNonNestedClass( > // have default arguments or don't use the standard calling convention > are > // wrapped with a thunk called the default constructor closure. > if (RD && Context.getTargetInfo().getCXXABI().isMicrosoft()) > - getDefaultArgExprsForConstructors(*this, RD); > + checkDefaultArgExprsForConstructors(*this, RD); > > referenceDLLExportedClassMethods(); > } > > Modified: cfe/trunk/lib/Sema/SemaExpr.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaExpr.cpp?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Sema/SemaExpr.cpp (original) > +++ cfe/trunk/lib/Sema/SemaExpr.cpp Wed Nov 23 10:51:30 2016 > @@ -4513,16 +4513,15 @@ Sema::CreateBuiltinArraySubscriptExpr(Ex > ArraySubscriptExpr(LHSExp, RHSExp, ResultType, VK, OK, RLoc); > } > > -ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc, > - FunctionDecl *FD, > - ParmVarDecl *Param) { > +bool Sema::CheckCXXDefaultArgExpr(SourceLocation CallLoc, FunctionDecl > *FD, > + ParmVarDecl *Param) { > if (Param->hasUnparsedDefaultArg()) { > Diag(CallLoc, > diag::err_use_of_default_argument_to_function_declared_later) << > FD << cast<CXXRecordDecl>(FD->getDeclContext())->getDeclName(); > Diag(UnparsedDefaultArgLocs[Param], > diag::note_default_argument_declared_here); > - return ExprError(); > + return true; > } > > if (Param->hasUninstantiatedDefaultArg()) { > @@ -4538,11 +4537,11 @@ ExprResult Sema::BuildCXXDefaultArgExpr( > InstantiatingTemplate Inst(*this, CallLoc, Param, > MutiLevelArgList.getInnermost()); > if (Inst.isInvalid()) > - return ExprError(); > + return true; > if (Inst.isAlreadyInstantiating()) { > Diag(Param->getLocStart(), diag::err_recursive_default_argument) > << FD; > Param->setInvalidDecl(); > - return ExprError(); > + return true; > } > > ExprResult Result; > @@ -4557,7 +4556,7 @@ ExprResult Sema::BuildCXXDefaultArgExpr( > /*DirectInit*/false); > } > if (Result.isInvalid()) > - return ExprError(); > + return true; > > // Check the expression as an initializer for the parameter. > InitializedEntity Entity > @@ -4570,12 +4569,12 @@ ExprResult Sema::BuildCXXDefaultArgExpr( > InitializationSequence InitSeq(*this, Entity, Kind, ResultE); > Result = InitSeq.Perform(*this, Entity, Kind, ResultE); > if (Result.isInvalid()) > - return ExprError(); > + return true; > > Result = ActOnFinishFullExpr(Result.getAs<Expr>(), > Param->getOuterLocStart()); > if (Result.isInvalid()) > - return ExprError(); > + return true; > > // Remember the instantiated default argument. > Param->setDefaultArg(Result.getAs<Expr>()); > @@ -4588,7 +4587,7 @@ ExprResult Sema::BuildCXXDefaultArgExpr( > if (!Param->hasInit()) { > Diag(Param->getLocStart(), diag::err_recursive_default_argument) << > FD; > Param->setInvalidDecl(); > - return ExprError(); > + return true; > } > > // If the default expression creates temporaries, we need to > @@ -4615,9 +4614,15 @@ ExprResult Sema::BuildCXXDefaultArgExpr( > // as being "referenced". > MarkDeclarationsReferencedInExpr(Param->getDefaultArg(), > /*SkipLocalVariables=*/true); > - return CXXDefaultArgExpr::Create(Context, CallLoc, Param); > + return false; > } > > +ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc, > + FunctionDecl *FD, ParmVarDecl > *Param) { > + if (CheckCXXDefaultArgExpr(CallLoc, FD, Param)) > + return ExprError(); > + return CXXDefaultArgExpr::Create(Context, CallLoc, Param); > +} > > Sema::VariadicCallType > Sema::getVariadicCallType(FunctionDecl *FDecl, const FunctionProtoType > *Proto, > > Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/ > SemaExprCXX.cpp?rev=287774&r1=287773&r2=287774&view=diff > ============================================================ > ================== > --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original) > +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Wed Nov 23 10:51:30 2016 > @@ -863,13 +863,8 @@ bool Sema::CheckCXXThrowOperand(SourceLo > // We don't keep the instantiated default argument expressions > around so > // we must rebuild them here. > for (unsigned I = 1, E = CD->getNumParams(); I != E; ++I) { > - // Skip any default arguments that we've already instantiated. > - if (Context.getDefaultArgExprForConstructor(CD, I)) > - continue; > - > - Expr *DefaultArg = > - BuildCXXDefaultArgExpr(ThrowLoc, CD, > CD->getParamDecl(I)).get(); > - Context.addDefaultArgExprForConstructor(CD, I, DefaultArg); > + if (CheckCXXDefaultArgExpr(ThrowLoc, CD, CD->getParamDecl(I))) > + return true; > } > } > } > > Added: cfe/trunk/test/SemaCXX/default-arg-closures.cpp > URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ > SemaCXX/default-arg-closures.cpp?rev=287774&view=auto > ============================================================ > ================== > --- cfe/trunk/test/SemaCXX/default-arg-closures.cpp (added) > +++ cfe/trunk/test/SemaCXX/default-arg-closures.cpp Wed Nov 23 10:51:30 > 2016 > @@ -0,0 +1,45 @@ > +// RUN: %clang_cc1 -triple x86_64-windows-msvc -fexceptions > -fcxx-exceptions -fms-extensions -verify %s -std=c++11 > + > +// The MS ABI has a few ways to generate constructor closures, which > require > +// instantiating and checking the semantics of default arguments. Make > sure we > +// do that right. > + > +// FIXME: Don't diagnose this issue twice. > +template <typename T> > +struct DependentDefaultCtorArg { // expected-note {{in instantiation of > default function argument}} > + // expected-error@+1 2 {{type 'int' cannot be used prior to '::' > because it has no members}} > + DependentDefaultCtorArg(int n = T::error); > +}; > +struct > +__declspec(dllexport) // expected-note {{due to > 'ExportDefaultCtorClosure' being dllexported}} > +ExportDefaultCtorClosure // expected-note {{implicit default constructor > for 'ExportDefaultCtorClosure' first required here}} > +: DependentDefaultCtorArg<int> // expected-note {{in instantiation of > template class}} > +{}; > + > +template <typename T> > +struct DependentDefaultCopyArg { > + DependentDefaultCopyArg() {} > + // expected-error@+1 {{type 'int' cannot be used prior to '::' because > it has no members}} > + DependentDefaultCopyArg(const DependentDefaultCopyArg &o, int n = > T::member) {} > +}; > + > +struct HasMember { > + enum { member = 0 }; > +}; > +void UseDependentArg() { throw DependentDefaultCopyArg<HasMember>(); } > + > +void ErrorInDependentArg() { > + throw DependentDefaultCopyArg<int>(); // expected-note {{required > here}} > +} > + > +struct HasCleanup { > + ~HasCleanup(); > +}; > + > +struct Default { > + Default(const Default &o, int d = (HasCleanup(), 42)); > +}; > + > +void f(const Default &d) { > + throw d; > +} > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
