From c50959431928097bbd675ff3a2346813982db176 Mon Sep 17 00:00:00 2001 From: Balazs Keri <1.int32@gmail.com> Date: Wed, 14 Aug 2019 09:41:39 +0000 Subject: [PATCH] [ASTImporter] Import default expression of param before creating the param. Summary: The default expression of a parameter variable should be imported before the parameter variable object is created. Otherwise the function is created with an incomplete parameter variable (default argument is nullptr) and in this intermediary state the expression is imported. This import can have a reference to the incomplete parameter variable that causes crash. Reviewers: martong, a.sidorin, shafik Reviewed By: martong Subscribers: rnkovacs, dkrupp, Szelethus, gamesh411, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D65577 llvm-svn: 368818 --- clang/include/clang/AST/ASTImporter.h | 14 ++++ clang/lib/AST/ASTImporter.cpp | 66 +++++++++++++------ clang/test/Analysis/Inputs/ctu-other.cpp | 19 ++++++ .../Inputs/ctu-other.cpp.externalDefMap.txt | 1 + clang/test/Analysis/ctu-main.cpp | 5 ++ 5 files changed, 86 insertions(+), 19 deletions(-) diff --git a/clang/include/clang/AST/ASTImporter.h b/clang/include/clang/AST/ASTImporter.h index 12bd5623ec0f..6f30fa47d841 100644 --- a/clang/include/clang/AST/ASTImporter.h +++ b/clang/include/clang/AST/ASTImporter.h @@ -378,6 +378,20 @@ class TypeSourceInfo; /// imported. If it does not exist nullptr is returned. TranslationUnitDecl *GetFromTU(Decl *ToD); + /// Return the declaration in the "from" context from which the declaration + /// in the "to" context was imported. If it was not imported or of the wrong + /// type a null value is returned. + template + llvm::Optional getImportedFromDecl(const DeclT *ToD) const { + auto FromI = ImportedFromDecls.find(ToD); + if (FromI == ImportedFromDecls.end()) + return {}; + auto *FromD = dyn_cast(FromI->second); + if (!FromD) + return {}; + return FromD; + } + /// Import the given declaration context from the "from" /// AST context into the "to" AST context. /// diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp index 651c675649ec..5b7b5438f6c3 100644 --- a/clang/lib/AST/ASTImporter.cpp +++ b/clang/lib/AST/ASTImporter.cpp @@ -426,6 +426,9 @@ namespace clang { Error ImportFunctionDeclBody(FunctionDecl *FromFD, FunctionDecl *ToFD); + Error ImportDefaultArgOfParmVarDecl(const ParmVarDecl *FromParam, + ParmVarDecl *ToParam); + template bool hasSameVisibilityContext(T *Found, T *From); @@ -3841,6 +3844,28 @@ ExpectedDecl ASTNodeImporter::VisitImplicitParamDecl(ImplicitParamDecl *D) { return ToParm; } +Error ASTNodeImporter::ImportDefaultArgOfParmVarDecl( + const ParmVarDecl *FromParam, ParmVarDecl *ToParam) { + ToParam->setHasInheritedDefaultArg(FromParam->hasInheritedDefaultArg()); + ToParam->setKNRPromoted(FromParam->isKNRPromoted()); + + if (FromParam->hasUninstantiatedDefaultArg()) { + if (auto ToDefArgOrErr = import(FromParam->getUninstantiatedDefaultArg())) + ToParam->setUninstantiatedDefaultArg(*ToDefArgOrErr); + else + return ToDefArgOrErr.takeError(); + } else if (FromParam->hasUnparsedDefaultArg()) { + ToParam->setUnparsedDefaultArg(); + } else if (FromParam->hasDefaultArg()) { + if (auto ToDefArgOrErr = import(FromParam->getDefaultArg())) + ToParam->setDefaultArg(*ToDefArgOrErr); + else + return ToDefArgOrErr.takeError(); + } + + return Error::success(); +} + ExpectedDecl ASTNodeImporter::VisitParmVarDecl(ParmVarDecl *D) { // Parameters are created in the translation unit's context, then moved // into the function declaration's context afterward. @@ -3867,23 +3892,11 @@ ExpectedDecl ASTNodeImporter::VisitParmVarDecl(ParmVarDecl *D) { /*DefaultArg*/ nullptr)) return ToParm; - // Set the default argument. - ToParm->setHasInheritedDefaultArg(D->hasInheritedDefaultArg()); - ToParm->setKNRPromoted(D->isKNRPromoted()); - - if (D->hasUninstantiatedDefaultArg()) { - if (auto ToDefArgOrErr = import(D->getUninstantiatedDefaultArg())) - ToParm->setUninstantiatedDefaultArg(*ToDefArgOrErr); - else - return ToDefArgOrErr.takeError(); - } else if (D->hasUnparsedDefaultArg()) { - ToParm->setUnparsedDefaultArg(); - } else if (D->hasDefaultArg()) { - if (auto ToDefArgOrErr = import(D->getDefaultArg())) - ToParm->setDefaultArg(*ToDefArgOrErr); - else - return ToDefArgOrErr.takeError(); - } + // Set the default argument. It should be no problem if it was already done. + // Do not import the default expression before GetImportedOrCreateDecl call + // to avoid possible infinite import loop because circular dependency. + if (Error Err = ImportDefaultArgOfParmVarDecl(D, ToParm)) + return std::move(Err); if (D->isObjCMethodParameter()) { ToParm->setObjCMethodScopeInfo(D->getFunctionScopeIndex()); @@ -6939,8 +6952,23 @@ ExpectedStmt ASTNodeImporter::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) { if (!UsedContextOrErr) return UsedContextOrErr.takeError(); - return CXXDefaultArgExpr::Create( - Importer.getToContext(), *ToUsedLocOrErr, *ToParamOrErr, *UsedContextOrErr); + // Import the default arg if it was not imported yet. + // This is needed because it can happen that during the import of the + // default expression (from VisitParmVarDecl) the same ParmVarDecl is + // encountered here. The default argument for a ParmVarDecl is set in the + // ParmVarDecl only after it is imported (set in VisitParmVarDecl if not here, + // see VisitParmVarDecl). + ParmVarDecl *ToParam = *ToParamOrErr; + if (!ToParam->getDefaultArg()) { + Optional FromParam = Importer.getImportedFromDecl(ToParam); + assert(FromParam && "ParmVarDecl was not imported?"); + + if (Error Err = ImportDefaultArgOfParmVarDecl(*FromParam, ToParam)) + return std::move(Err); + } + + return CXXDefaultArgExpr::Create(Importer.getToContext(), *ToUsedLocOrErr, + *ToParamOrErr, *UsedContextOrErr); } ExpectedStmt diff --git a/clang/test/Analysis/Inputs/ctu-other.cpp b/clang/test/Analysis/Inputs/ctu-other.cpp index c2410ced35dc..64c990c5e7cf 100644 --- a/clang/test/Analysis/Inputs/ctu-other.cpp +++ b/clang/test/Analysis/Inputs/ctu-other.cpp @@ -145,3 +145,22 @@ public: static const int Test; }; const int TestAnonUnionUSR::Test = 5; + +struct DefaultParmContext { + static const int I; + int f(); +}; + +int fDefaultParm(int I = DefaultParmContext::I) { + return I; +} + +int testImportOfIncompleteDefaultParmDuringImport(int I) { + return fDefaultParm(I); +} + +const int DefaultParmContext::I = 0; + +int DefaultParmContext::f() { + return fDefaultParm(); +} diff --git a/clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt b/clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt index da95098f4c83..73cd61b0cb26 100644 --- a/clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt +++ b/clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt @@ -26,3 +26,4 @@ c:@extSubSCN ctu-other.cpp.ast c:@extSCC ctu-other.cpp.ast c:@extU ctu-other.cpp.ast c:@S@TestAnonUnionUSR@Test ctu-other.cpp.ast +c:@F@testImportOfIncompleteDefaultParmDuringImport#I# ctu-other.cpp.ast diff --git a/clang/test/Analysis/ctu-main.cpp b/clang/test/Analysis/ctu-main.cpp index aae4808e90ff..c4aee409ee2f 100644 --- a/clang/test/Analysis/ctu-main.cpp +++ b/clang/test/Analysis/ctu-main.cpp @@ -125,6 +125,8 @@ public: static const int Test; }; +extern int testImportOfIncompleteDefaultParmDuringImport(int); + int main() { clang_analyzer_eval(f(3) == 2); // expected-warning{{TRUE}} clang_analyzer_eval(f(4) == 3); // expected-warning{{TRUE}} @@ -157,5 +159,8 @@ int main() { clang_analyzer_eval(extSubSCN.a == 1); // expected-warning{{TRUE}} // clang_analyzer_eval(extSCC.a == 7); // TODO clang_analyzer_eval(extU.a == 4); // expected-warning{{TRUE}} + clang_analyzer_eval(TestAnonUnionUSR::Test == 5); // expected-warning{{TRUE}} + + clang_analyzer_eval(testImportOfIncompleteDefaultParmDuringImport(9) == 9); // expected-warning{{TRUE}} }