[ASTImporter] Import default expression of param before creating the param.

[balazske]

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.

[martong]

LGTM, though I have some comments.

clang/test/Analysis/Inputs/ctu-other.cpp

135

Perhaps we could write Default instead of Def.

144

testImportOfIncompleteDefaultParm ?

[shafik]

clang/lib/AST/ASTImporter.cpp

3892

This should be DefaultArg now?

clang/test/Analysis/Inputs/ctu-other.cpp

135

+1 to this and the name suggestion below.

[balazske]

clang/lib/AST/ASTImporter.cpp

3892

I am not sure if it is always correct, specially in the hasUninstantiatedDefaultArg case. (The constructor calls setDefaultArg only, probably this was the reason to set the default arg afterwards in different ways that is not doable with the constructor.)

clang/test/Analysis/Inputs/ctu-other.cpp

144

The reason for this name was that the import is incomplete, not the the default parameter. (Exactly, before the fix, the import results in a ParmVarDecl that is for temporary time interval incomplete when the default expression is missing.) In this way testIncompleteImportOfDefaultParm can be better, or testImportWithIncompleteDefaultParm.

[balazske]

A new kind of solution, the previous does not work always.
Still no test for this new problem case (circular dependency between a ParmVarDecl and function of it and a CXXDefaultArgExpr that references to the function probably by UsedContext).

[balazske]

Small but important fix.

[martong]

Still looks good to me, other than some style nits.

clang/lib/AST/ASTImporter.cpp

3847

This line seems to be longer than 80 columns, forgot to run clang-format?

6964

This would be more appropriate I guess: "ParmVarDecl was not imported?"

clang/test/Analysis/Inputs/ctu-other.cpp

135

Please do not forget to rename.

[martong]

Would it be an alternative solution if we imported the function parameters after we created the FunctionDecl? I guess it would be. Maybe that would result in a smaller change, what do you think?

[balazske]

clang/test/Analysis/Inputs/ctu-other.cpp

135

I did not get response to the name of testDefParmIncompleteImport, I think testImportOfIncompleteDefaultParm is not good, probably testImportOfIncompleteDefaultParmDuringImport is better.

[balazske]

Yes importing the parameters should be doable after the create of the function. I am not sure if it fixes this problem, the parameter that has the default expression can still be encountered without the default expression set in it.

[martong]

clang/test/Analysis/Inputs/ctu-other.cpp

135

Okay, so let's proceed with testImportOfIncompleteDefaultParmDuringImport.
Thanks!

[balazske]

Rename of variables and reformat.

clang/include/clang/AST/ASTImporter.h

public:
     /// it has already been imported from the "from" context.  Otherwise return
     /// nullptr.
     Decl *GetAlreadyImportedOrNull(const Decl *FromD) const;
 
     /// Return the translation unit from where the declaration was
     /// 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 <typename DeclT>
+    llvm::Optional<DeclT *> getImportedFromDecl(const DeclT *ToD) const {
+      auto FromI = ImportedFromDecls.find(ToD);
+      if (FromI == ImportedFromDecls.end())
+        return {};
+      auto *FromD = dyn_cast<DeclT>(FromI->second);
+      if (!FromD)
+        return {};
+      return FromD;
+    }
+
     /// Import the given declaration context from the "from"
     /// AST context into the "to" AST context.
     ///
     /// \returns the equivalent declaration context in the "to"
     /// context, or error value.
     llvm::Expected<DeclContext *> ImportContext(DeclContext *FromDC);
 
     /// Import the given expression from the "from" context into the

clang/lib/AST/ASTImporter.cpp

ImportFunctionTemplateWithTemplateArgsFromSpecialization(
         FunctionDecl *FromFD);
     Error ImportTemplateParameterLists(const DeclaratorDecl *FromD,
                                        DeclaratorDecl *ToD);
 
     Error ImportTemplateInformation(FunctionDecl *FromFD, FunctionDecl *ToFD);
 
     Error ImportFunctionDeclBody(FunctionDecl *FromFD, FunctionDecl *ToFD);
 
+    Error ImportDefaultArgOfParmVarDecl(const ParmVarDecl *FromParam,
+                                        ParmVarDecl *ToParam);
+
     template <typename T>
     bool hasSameVisibilityContext(T *Found, T *From);
 
     bool IsStructuralMatch(Decl *From, Decl *To, bool Complain);
     bool IsStructuralMatch(RecordDecl *FromRecord, RecordDecl *ToRecord,
                            bool Complain = true);
     bool IsStructuralMatch(VarDecl *FromVar, VarDecl *ToVar,
                            bool Complain = true);
ExpectedDecl ASTNodeImporter::VisitImplicitParamDecl(ImplicitParamDecl *D) {
   ImplicitParamDecl *ToParm = nullptr;
   if (GetImportedOrCreateDecl(ToParm, D, Importer.getToContext(), DC,
                               ToLocation, ToDeclName.getAsIdentifierInfo(),
                               ToType, D->getParameterKind()))
     return ToParm;
   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.
   DeclContext *DC = Importer.getToContext().getTranslationUnitDecl();
 
   DeclarationName ToDeclName;
   SourceLocation ToLocation, ToInnerLocStart;
   QualType ToType;
   TypeSourceInfo *ToTypeSourceInfo;
   if (auto Imp = importSeq(
       D->getDeclName(), D->getLocation(), D->getType(), D->getInnerLocStart(),
       D->getTypeSourceInfo()))
     std::tie(
         ToDeclName, ToLocation, ToType, ToInnerLocStart,
         ToTypeSourceInfo) = *Imp;
   else
     return Imp.takeError();
 
   ParmVarDecl *ToParm;
   if (GetImportedOrCreateDecl(ToParm, D, Importer.getToContext(), DC,
                               ToInnerLocStart, ToLocation,
                               ToDeclName.getAsIdentifierInfo(), ToType,
                               ToTypeSourceInfo, D->getStorageClass(),
                               /*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());
     ToParm->setObjCDeclQualifier(D->getObjCDeclQualifier());
   } else {
     ToParm->setScopeInfo(D->getFunctionScopeDepth(),
                          D->getFunctionScopeIndex());
   }
ExpectedStmt ASTNodeImporter::VisitCXXDefaultArgExpr(CXXDefaultArgExpr *E) {
   auto ToParamOrErr = import(E->getParam());
   if (!ToParamOrErr)
     return ToParamOrErr.takeError();
 
   auto UsedContextOrErr = Importer.ImportContext(E->getUsedContext());
   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<ParmVarDecl *> 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
 ASTNodeImporter::VisitCXXScalarValueInitExpr(CXXScalarValueInitExpr *E) {
   auto Imp = importSeq(
       E->getType(), E->getTypeSourceInfo(), E->getRParenLoc());
   if (!Imp)
     return Imp.takeError();

clang/test/Analysis/Inputs/ctu-other.cpp

 };
 SCC extSCC{7};
 union U {
   const int a;
   const unsigned int b;
 };
 U extU = {.a = 4};
 
 class TestAnonUnionUSR {
 public:
   inline float f(int value) {
     union {
       float f;
       int i;
     };
     i = value;
     return f;
   }
   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();
+}

clang/test/Analysis/Inputs/ctu-other.cpp.externalDefMap.txt

 c:@S@A@a ctu-other.cpp.ast
 c:@extSC ctu-other.cpp.ast
 c:@S@ST@sc ctu-other.cpp.ast
 c:@extSCN ctu-other.cpp.ast
 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

clang/test/Analysis/ctu-main.cpp

union {
       int i;
     };
     i = value;
     return f;
   }
   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}}
   clang_analyzer_eval(f(5) == 3); // expected-warning{{FALSE}}
   clang_analyzer_eval(g(4) == 6); // expected-warning{{TRUE}}
   clang_analyzer_eval(h(2) == 8); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(myns::fns(2) == 9);                   // expected-warning{{TRUE}}
int main() {
   clang_analyzer_eval(extHere == 6); // expected-warning{{TRUE}}
   clang_analyzer_eval(A::a == 3); // expected-warning{{TRUE}}
   clang_analyzer_eval(extSC.a == 8); // expected-warning{{TRUE}}
   clang_analyzer_eval(ST::sc.a == 2); // expected-warning{{TRUE}}
   // clang_analyzer_eval(extSCN.scn.a == 9); // TODO
   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}}
 }