From: John McCall Date: Wed, 6 Jan 2016 22:34:54 +0000 (+0000) Subject: Only instantiate a default argument once. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=d201541db4135d160d249eb86e3d0718285903a2;p=clang Only instantiate a default argument once. By storing the instantiated expression back in the ParmVarDecl, we remove the last need for separately storing the sub-expression of a CXXDefaultArgExpr. This makes PCH/Modules merging quite simple: CXXDefaultArgExpr records are serialized as references to the ParmVarDecl, and we ignore redundant attempts to overwrite the instantiated expression. This has some extremely marginal impact on user-facing semantics. However, the major effect is that it avoids IRGen errors about conflicting definitions due to lambdas in the argument being instantiated multiple times while sharing the same mangling. It should also slightly improve memory usage and module file size. rdar://23810407 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@256983 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/ASTMutationListener.h b/include/clang/AST/ASTMutationListener.h index 3ff392de11..cf3b55d7b2 100644 --- a/include/clang/AST/ASTMutationListener.h +++ b/include/clang/AST/ASTMutationListener.h @@ -29,6 +29,7 @@ namespace clang { class ObjCContainerDecl; class ObjCInterfaceDecl; class ObjCPropertyDecl; + class ParmVarDecl; class QualType; class RecordDecl; class TagDecl; @@ -88,6 +89,9 @@ public: /// \brief A function template's definition was instantiated. virtual void FunctionDefinitionInstantiated(const FunctionDecl *D) {} + /// \brief A default argument was instantiated. + virtual void DefaultArgumentInstantiated(const ParmVarDecl *D) {} + /// \brief A new objc category class was added for an interface. virtual void AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD, const ObjCInterfaceDecl *IFD) {} diff --git a/include/clang/AST/ExprCXX.h b/include/clang/AST/ExprCXX.h index 0608abac1f..6821274986 100644 --- a/include/clang/AST/ExprCXX.h +++ b/include/clang/AST/ExprCXX.h @@ -951,15 +951,9 @@ public: /// This wraps up a function call argument that was created from the /// corresponding parameter's default argument, when the call did not /// explicitly supply arguments for all of the parameters. -class CXXDefaultArgExpr final - : public Expr, - private llvm::TrailingObjects { +class CXXDefaultArgExpr final : public Expr { /// \brief The parameter whose default is being used. - /// - /// When the bit is set, the subexpression is stored after the - /// CXXDefaultArgExpr itself. When the bit is clear, the parameter's - /// actual default expression is the subexpression. - llvm::PointerIntPair Param; + ParmVarDecl *Param; /// \brief The location where the default argument expression was used. SourceLocation Loc; @@ -971,16 +965,7 @@ class CXXDefaultArgExpr final : param->getDefaultArg()->getType(), param->getDefaultArg()->getValueKind(), param->getDefaultArg()->getObjectKind(), false, false, false, false), - Param(param, false), Loc(Loc) { } - - CXXDefaultArgExpr(StmtClass SC, SourceLocation Loc, ParmVarDecl *param, - Expr *SubExpr) - : Expr(SC, SubExpr->getType(), - SubExpr->getValueKind(), SubExpr->getObjectKind(), - false, false, false, false), - Param(param, true), Loc(Loc) { - *getTrailingObjects() = SubExpr; - } + Param(param), Loc(Loc) { } public: CXXDefaultArgExpr(EmptyShell Empty) : Expr(CXXDefaultArgExprClass, Empty) {} @@ -992,24 +977,15 @@ public: return new (C) CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param); } - // \p Param is the parameter whose default argument is used by this - // expression, and \p SubExpr is the expression that will actually be used. - static CXXDefaultArgExpr *Create(const ASTContext &C, SourceLocation Loc, - ParmVarDecl *Param, Expr *SubExpr); - // Retrieve the parameter that the argument was created from. - const ParmVarDecl *getParam() const { return Param.getPointer(); } - ParmVarDecl *getParam() { return Param.getPointer(); } + const ParmVarDecl *getParam() const { return Param; } + ParmVarDecl *getParam() { return Param; } // Retrieve the actual argument to the function call. const Expr *getExpr() const { - if (Param.getInt()) - return *getTrailingObjects(); return getParam()->getDefaultArg(); } Expr *getExpr() { - if (Param.getInt()) - return *getTrailingObjects(); return getParam()->getDefaultArg(); } @@ -1033,7 +1009,6 @@ public: return child_range(child_iterator(), child_iterator()); } - friend TrailingObjects; friend class ASTStmtReader; friend class ASTStmtWriter; }; diff --git a/include/clang/Serialization/ASTWriter.h b/include/clang/Serialization/ASTWriter.h index ed345472fc..ef8c653413 100644 --- a/include/clang/Serialization/ASTWriter.h +++ b/include/clang/Serialization/ASTWriter.h @@ -871,6 +871,7 @@ public: const FunctionDecl *Delete) override; void CompletedImplicitDefinition(const FunctionDecl *D) override; void StaticDataMemberInstantiated(const VarDecl *D) override; + void DefaultArgumentInstantiated(const ParmVarDecl *D) override; void FunctionDefinitionInstantiated(const FunctionDecl *D) override; void AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD, const ObjCInterfaceDecl *IFD) override; diff --git a/lib/AST/ExprCXX.cpp b/lib/AST/ExprCXX.cpp index d35efcb101..ea983340a2 100644 --- a/lib/AST/ExprCXX.cpp +++ b/lib/AST/ExprCXX.cpp @@ -763,14 +763,6 @@ const IdentifierInfo *UserDefinedLiteral::getUDSuffix() const { return cast(getCalleeDecl())->getLiteralIdentifier(); } -CXXDefaultArgExpr * -CXXDefaultArgExpr::Create(const ASTContext &C, SourceLocation Loc, - ParmVarDecl *Param, Expr *SubExpr) { - void *Mem = C.Allocate(totalSizeToAlloc(1)); - return new (Mem) CXXDefaultArgExpr(CXXDefaultArgExprClass, Loc, Param, - SubExpr); -} - CXXDefaultInitExpr::CXXDefaultInitExpr(const ASTContext &C, SourceLocation Loc, FieldDecl *Field, QualType T) : Expr(CXXDefaultInitExprClass, T.getNonLValueExprType(C), diff --git a/lib/Frontend/MultiplexConsumer.cpp b/lib/Frontend/MultiplexConsumer.cpp index 12c85240bd..f8b73e9034 100644 --- a/lib/Frontend/MultiplexConsumer.cpp +++ b/lib/Frontend/MultiplexConsumer.cpp @@ -119,6 +119,7 @@ public: const FunctionDecl *Delete) override; void CompletedImplicitDefinition(const FunctionDecl *D) override; void StaticDataMemberInstantiated(const VarDecl *D) override; + void DefaultArgumentInstantiated(const ParmVarDecl *D) override; void AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD, const ObjCInterfaceDecl *IFD) override; void FunctionDefinitionInstantiated(const FunctionDecl *D) override; @@ -193,6 +194,11 @@ void MultiplexASTMutationListener::StaticDataMemberInstantiated( for (size_t i = 0, e = Listeners.size(); i != e; ++i) Listeners[i]->StaticDataMemberInstantiated(D); } +void MultiplexASTMutationListener::DefaultArgumentInstantiated( + const ParmVarDecl *D) { + for (size_t i = 0, e = Listeners.size(); i != e; ++i) + Listeners[i]->DefaultArgumentInstantiated(D); +} void MultiplexASTMutationListener::AddedObjCCategoryToInterface( const ObjCCategoryDecl *CatD, const ObjCInterfaceDecl *IFD) { diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index 5d0c6057f5..f1b9987a2f 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -4315,8 +4315,15 @@ ExprResult Sema::BuildCXXDefaultArgExpr(SourceLocation CallLoc, Expr *Arg = Result.getAs(); CheckCompletedExpr(Arg, Param->getOuterLocStart()); + + // Remember the instantiated default argument. + Param->setDefaultArg(Arg); + if (ASTMutationListener *L = getASTMutationListener()) { + L->DefaultArgumentInstantiated(Param); + } + // Build the default argument expression. - return CXXDefaultArgExpr::Create(Context, CallLoc, Param, Arg); + return CXXDefaultArgExpr::Create(Context, CallLoc, Param); } // If the default expression creates temporaries, we need to diff --git a/lib/Serialization/ASTCommon.h b/lib/Serialization/ASTCommon.h index e59bc891f9..64f583c987 100644 --- a/lib/Serialization/ASTCommon.h +++ b/lib/Serialization/ASTCommon.h @@ -29,6 +29,7 @@ enum DeclUpdateKind { UPD_CXX_ADDED_FUNCTION_DEFINITION, UPD_CXX_INSTANTIATED_STATIC_DATA_MEMBER, UPD_CXX_INSTANTIATED_CLASS_DEFINITION, + UPD_CXX_INSTANTIATED_DEFAULT_ARGUMENT, UPD_CXX_RESOLVED_DTOR_DELETE, UPD_CXX_RESOLVED_EXCEPTION_SPEC, UPD_CXX_DEDUCED_RETURN_TYPE, diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 8fb110e455..5bf95f878d 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -3626,6 +3626,21 @@ void ASTDeclReader::UpdateDecl(Decl *D, ModuleFile &ModuleFile, Reader.ReadSourceLocation(ModuleFile, Record, Idx)); break; + case UPD_CXX_INSTANTIATED_DEFAULT_ARGUMENT: { + auto Param = cast(D); + + // We have to read the default argument regardless of whether we use it + // so that hypothetical further update records aren't messed up. + // TODO: Add a function to skip over the next expr record. + auto DefaultArg = Reader.ReadExpr(F); + + // Only apply the update if the parameter still has an uninstantiated + // default argument. + if (Param->hasUninstantiatedDefaultArg()) + Param->setDefaultArg(DefaultArg); + break; + } + case UPD_CXX_ADDED_FUNCTION_DEFINITION: { FunctionDecl *FD = cast(D); if (Reader.PendingBodies[FD]) { diff --git a/lib/Serialization/ASTReaderStmt.cpp b/lib/Serialization/ASTReaderStmt.cpp index bc678aff78..ad81ac8442 100644 --- a/lib/Serialization/ASTReaderStmt.cpp +++ b/lib/Serialization/ASTReaderStmt.cpp @@ -1364,10 +1364,7 @@ void ASTStmtReader::VisitCXXThrowExpr(CXXThrowExpr *E) { void ASTStmtReader::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) { VisitExpr(E); - - assert((bool)Record[Idx] == E->Param.getInt() && "We messed up at creation ?"); - ++Idx; // HasOtherExprStored and SubExpr was handled during creation. - E->Param.setPointer(ReadDeclAs(Record, Idx)); + E->Param = ReadDeclAs(Record, Idx); E->Loc = ReadSourceLocation(Record, Idx); } @@ -3205,16 +3202,9 @@ Stmt *ASTReader::ReadStmtFromStream(ModuleFile &F) { case EXPR_CXX_THROW: S = new (Context) CXXThrowExpr(Empty); break; - case EXPR_CXX_DEFAULT_ARG: { - bool HasOtherExprStored = Record[ASTStmtReader::NumExprFields]; - if (HasOtherExprStored) { - Expr *SubExpr = ReadSubExpr(); - S = CXXDefaultArgExpr::Create(Context, SourceLocation(), nullptr, - SubExpr); - } else - S = new (Context) CXXDefaultArgExpr(Empty); + case EXPR_CXX_DEFAULT_ARG: + S = new (Context) CXXDefaultArgExpr(Empty); break; - } case EXPR_CXX_DEFAULT_INIT: S = new (Context) CXXDefaultInitExpr(Empty); break; diff --git a/lib/Serialization/ASTWriter.cpp b/lib/Serialization/ASTWriter.cpp index 0f50d7a42e..e36e918517 100644 --- a/lib/Serialization/ASTWriter.cpp +++ b/lib/Serialization/ASTWriter.cpp @@ -4611,6 +4611,11 @@ void ASTWriter::WriteDeclUpdatesBlocks(RecordDataImpl &OffsetsRecord) { AddSourceLocation(Update.getLoc(), Record); break; + case UPD_CXX_INSTANTIATED_DEFAULT_ARGUMENT: + AddStmt(const_cast( + cast(Update.getDecl())->getDefaultArg())); + break; + case UPD_CXX_INSTANTIATED_CLASS_DEFINITION: { auto *RD = cast(D); UpdatedDeclContexts.insert(RD->getPrimaryContext()); @@ -5779,6 +5784,15 @@ void ASTWriter::StaticDataMemberInstantiated(const VarDecl *D) { D->getMemberSpecializationInfo()->getPointOfInstantiation())); } +void ASTWriter::DefaultArgumentInstantiated(const ParmVarDecl *D) { + assert(!WritingAST && "Already writing the AST!"); + if (!D->isFromASTFile()) + return; + + DeclUpdates[D].push_back( + DeclUpdate(UPD_CXX_INSTANTIATED_DEFAULT_ARGUMENT, D)); +} + void ASTWriter::AddedObjCCategoryToInterface(const ObjCCategoryDecl *CatD, const ObjCInterfaceDecl *IFD) { assert(!WritingAST && "Already writing the AST!"); diff --git a/lib/Serialization/ASTWriterStmt.cpp b/lib/Serialization/ASTWriterStmt.cpp index e52ed052d3..000a2185f5 100644 --- a/lib/Serialization/ASTWriterStmt.cpp +++ b/lib/Serialization/ASTWriterStmt.cpp @@ -1336,15 +1336,8 @@ void ASTStmtWriter::VisitCXXThrowExpr(CXXThrowExpr *E) { void ASTStmtWriter::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) { VisitExpr(E); - - bool HasOtherExprStored = E->Param.getInt(); - // Store these first, the reader reads them before creation. - Record.push_back(HasOtherExprStored); - if (HasOtherExprStored) - Writer.AddStmt(E->getExpr()); Writer.AddDeclRef(E->getParam(), Record); Writer.AddSourceLocation(E->getUsedLocation(), Record); - Code = serialization::EXPR_CXX_DEFAULT_ARG; } diff --git a/test/PCH/chain-default-argument-instantiation.cpp b/test/PCH/chain-default-argument-instantiation.cpp new file mode 100644 index 0000000000..0accd544a3 --- /dev/null +++ b/test/PCH/chain-default-argument-instantiation.cpp @@ -0,0 +1,50 @@ +// Test default argument instantiation in chained PCH. + +// Without PCH +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -include %s -include %s %s + +// With PCH +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s -chain-include %s -chain-include %s + +// With modules +// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 -fmodules %s -chain-include %s -chain-include %s + +// expected-no-diagnostics + +#ifndef HEADER1 +#define HEADER1 +//===----------------------------------------------------------------------===// +// Primary header. + +namespace rdar23810407 { + template int f(T t) { + extern T rdar23810407_variable; + return 0; + } + template int g(int a = f([] {})); +} + +//===----------------------------------------------------------------------===// +#elif not defined(HEADER2) +#define HEADER2 +#if !defined(HEADER1) +#error Header inclusion order messed up +#endif + +//===----------------------------------------------------------------------===// +// Dependent header. + +inline void instantiate_once() { + rdar23810407::g(); +} + +//===----------------------------------------------------------------------===// +#else +//===----------------------------------------------------------------------===// + +void test() { + rdar23810407::g(); +} + +//===----------------------------------------------------------------------===// +#endif diff --git a/test/SemaCXX/conversion.cpp b/test/SemaCXX/conversion.cpp index fe449c8ccb..89751a493e 100644 --- a/test/SemaCXX/conversion.cpp +++ b/test/SemaCXX/conversion.cpp @@ -94,9 +94,9 @@ namespace test4 { // FIXME: We should warn for non-dependent args (only when the param type is also non-dependent) only once // not once for the template + once for every instantiation template - void tmpl(char c = NULL, // expected-warning 4 {{implicit conversion of NULL constant to 'char'}} + void tmpl(char c = NULL, // expected-warning 3 {{implicit conversion of NULL constant to 'char'}} T a = NULL, // expected-warning {{implicit conversion of NULL constant to 'char'}} \ - expected-warning 2 {{implicit conversion of NULL constant to 'int'}} + expected-warning {{implicit conversion of NULL constant to 'int'}} T b = 1024) { // expected-warning {{implicit conversion from 'int' to 'char' changes value from 1024 to 0}} } @@ -107,8 +107,7 @@ namespace test4 { void func() { tmpl(); // expected-note 2 {{in instantiation of default function argument expression for 'tmpl' required here}} tmpl(); // expected-note 2 {{in instantiation of default function argument expression for 'tmpl' required here}} - // FIXME: We should warn only once for each template instantiation - not once for each call - tmpl(); // expected-note 2 {{in instantiation of default function argument expression for 'tmpl' required here}} + tmpl(); tmpl2(); } } diff --git a/test/SemaTemplate/default-arguments-cxx0x.cpp b/test/SemaTemplate/default-arguments-cxx0x.cpp index 4cfd7a5843..0c97c2056b 100644 --- a/test/SemaTemplate/default-arguments-cxx0x.cpp +++ b/test/SemaTemplate/default-arguments-cxx0x.cpp @@ -56,3 +56,22 @@ namespace PR16975 { baz data{0}; } + +// rdar://23810407 +// An IRGen failure due to a symbol collision due to a default argument +// being instantiated twice. Credit goes to Richard Smith for this +// reduction to a -fsyntax-only failure. +namespace rdar23810407 { + // Instantiating the default argument multiple times will produce two + // different lambda types and thus instantiate this function multiple + // times, which will produce conflicting extern variable declarations. + template int f(T t) { + extern T rdar23810407_variable; + return 0; + } + template int g(int a = f([] {})); + void test() { + g(); + g(); + } +}