[ASTImporter] Fix infinite recursion on function import with struct definition in parameters

[gerazo]

Summary

Importing a function having a struct definition in the parameter list causes a crash in the importer via infinite recursion. This patch avoids the crash and reports such functions as not supported. Unit tests make sure that normal struct definitions inside function bodies work normally on the other hand and LLDB-like type imports also do.

[balazske]

Problem: This change interferes with https://reviews.llvm.org/D47445. Probably that should be committed, it is approved already.

[gerazo]

In D47946#1127679, @balazske wrote:

Problem: This change interferes with https://reviews.llvm.org/D47445. Probably that should be committed, it is approved already.

Ok. I'll wait for the other thing to be committed and I will rework this immediately. Thanks for telling.

[gerazo]

Updated to not conflict with and use the stuff implemented in https://reviews.llvm.org/D47445 (so became a bit smaller)
Now, it is ready for a review. Enjoy!

[martong]

LGTM, just found some minor things.

unittests/AST/ASTImporterTest.cpp

174 ↗

(On Diff #151533)

Now that the Importer is moved under the TU struct it is not that trivial so I think we should add a few more comments to it.
Something like:

Represents a "From" translation unit and holds an importer object which we use to import from this translation unit.

Also there is an invariant which should be forced: the "To" and the "From" context of the Importer must remain the same throughout the life time of any TU object. We can achieve that with an assert in lazyInitImporter.

(What's more, the different objects all should have the same "To" contexts, but that could be checked only in the TestBase and looks more difficult, so for this patch we could skip that. )

190 ↗

(On Diff #151533)

I think, if there is an existing Importer then we should assert whether that has a ToASTwhich is equal to the parameter.

[gerazo]

Added @martong 's suggestions.

[gerazo]

@a.sidorin what do you think?

[martong]

Ping.

[a.sidorin]

Hello Zoltán,

Sorry for the delay. I think the patch is fine, just some minor nits inline.

unittests/AST/ASTImporterTest.cpp

234 ↗

(On Diff #152055)

Can we move the side effect into import()?

1049 ↗

(On Diff #152055)

EXPECT_FALSE?

[gerazo]

unittests/AST/ASTImporterTest.cpp

234 ↗

(On Diff #152055)

Sure, thank you.

[gerazo]

Minor fixes for Aleksei's comments.

[martong]

@gerazo, Do you have commit rights, or should I help with the commit?

[gerazo]

@martong I don't have commit rights. Thanks for your help in advance.

cfe/trunk/lib/AST/ASTImporter.cpp

 //----------------------------------------------------------------------------
 // Import Declarations
 //----------------------------------------------------------------------------
 bool ASTNodeImporter::ImportDeclParts(NamedDecl *D, DeclContext *&DC, 
                                       DeclContext *&LexicalDC, 
                                       DeclarationName &Name, 
                                       NamedDecl *&ToD,
                                       SourceLocation &Loc) {
+  // Check if RecordDecl is in FunctionDecl parameters to avoid infinite loop.
+  // example: int struct_in_proto(struct data_t{int a;int b;} *d);
+  DeclContext *OrigDC = D->getDeclContext();
+  FunctionDecl *FunDecl;
+  if (isa<RecordDecl>(D) && (FunDecl = dyn_cast<FunctionDecl>(OrigDC)) &&
+      FunDecl->hasBody()) {
+    SourceRange RecR = D->getSourceRange();
+    SourceRange BodyR = FunDecl->getBody()->getSourceRange();
+    // If RecordDecl is not in Body (it is a param), we bail out.
+    if (RecR.isValid() && BodyR.isValid() &&
+        (RecR.getBegin() < BodyR.getBegin() ||
+         BodyR.getEnd() < RecR.getEnd())) {
+      Importer.FromDiag(D->getLocation(), diag::err_unsupported_ast_node)
+          << D->getDeclKindName();
+      return true;
+    }
+  }
+
   // Import the context of this declaration.
-  DC = Importer.ImportContext(D->getDeclContext());
+  DC = Importer.ImportContext(OrigDC);
   if (!DC)
     return true;
   
   LexicalDC = DC;
   if (D->getDeclContext() != D->getLexicalDeclContext()) {
     LexicalDC = Importer.ImportContext(D->getLexicalDeclContext());
     if (!LexicalDC)
       return true;

cfe/trunk/unittests/AST/ASTImporterTest.cpp

struct TU {
     TU(StringRef Code, StringRef FileName, ArgVector Args)
         : Code(Code), FileName(FileName),
           Unit(tooling::buildASTFromCodeWithArgs(this->Code, Args,
                                                  this->FileName)),
           TUDecl(Unit->getASTContext().getTranslationUnitDecl()) {
       Unit->enableSourceFileDiagnostics();
     }
 
-    Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+    void lazyInitImporter(ASTUnit *ToAST) {
       assert(ToAST);
       if (!Importer) {
         Importer.reset(new ASTImporter(
             ToAST->getASTContext(), ToAST->getFileManager(),
             Unit->getASTContext(), Unit->getFileManager(), false));
       }
+      assert(&ToAST->getASTContext() == &Importer->getToContext());
+      createVirtualFileIfNeeded(ToAST, FileName, Code);
+    }
+
+    Decl *import(ASTUnit *ToAST, Decl *FromDecl) {
+      lazyInitImporter(ToAST);
       return Importer->Import(FromDecl);
      }
+
+    QualType import(ASTUnit *ToAST, QualType FromType) {
+      lazyInitImporter(ToAST);
+      return Importer->Import(FromType);
+    }
   };
 
   // We may have several From contexts and related translation units. In each
   // AST, the buffers for the source are handled via references and are set
   // during the creation of the AST. These references must point to a valid
   // buffer until the AST is alive. Thus, we must use a list in order to avoid
   // moving of the stored objects because that would mean breaking the
   // references in the AST. By using a vector a move could happen when the
   // vector is expanding, with the list we won't have these issues.
   std::list<TU> FromTUs;
 
+  void lazyInitToAST(Language ToLang) {
+    if (ToAST)
+      return;
+    ArgVector ToArgs = getArgVectorForLanguage(ToLang);
+    // Build the AST from an empty file.
+    ToAST = tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
+    ToAST->enableSourceFileDiagnostics();
+  }
+
+  TU *findFromTU(Decl *From) {
+    // Create a virtual file in the To Ctx which corresponds to the file from
+    // which we want to import the `From` Decl. Without this source locations
+    // will be invalid in the ToCtx.
+    auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU &E) {
+      return E.TUDecl == From->getTranslationUnitDecl();
+    });
+    assert(It != FromTUs.end());
+    return &*It;
+  }
+
 public:
   // We may have several From context but only one To context.
   std::unique_ptr<ASTUnit> ToAST;
 
   // Creates an AST both for the From and To source code and imports the Decl
   // of the identifier into the To context.
   // Must not be called more than once within the same test.
   std::tuple<Decl *, Decl *>
TranslationUnitDecl *getToTuDecl(StringRef ToSrcCode, Language ToLang) {
 
     return ToAST->getASTContext().getTranslationUnitDecl();
   }
 
   // Import the given Decl into the ToCtx.
   // May be called several times in a given test.
   // The different instances of the param From may have different ASTContext.
   Decl *Import(Decl *From, Language ToLang) {
-    if (!ToAST) {
-      ArgVector ToArgs = getArgVectorForLanguage(ToLang);
-      // Build the AST from an empty file.
-      ToAST =
-          tooling::buildASTFromCodeWithArgs(/*Code=*/"", ToArgs, "empty.cc");
-      ToAST->enableSourceFileDiagnostics();
-    }
+    lazyInitToAST(ToLang);
+    TU *FromTU = findFromTU(From);
+    return FromTU->import(ToAST.get(), From);
 
-    // Create a virtual file in the To Ctx which corresponds to the file from
-    // which we want to import the `From` Decl. Without this source locations
-    // will be invalid in the ToCtx.
-    auto It = std::find_if(FromTUs.begin(), FromTUs.end(), [From](const TU &E) {
-      return E.TUDecl == From->getTranslationUnitDecl();
-    });
-    assert(It != FromTUs.end());
-    createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-
-    return It->import(ToAST.get(), From);
+  QualType ImportType(QualType FromType, Decl *TUDecl, Language ToLang) {
+    lazyInitToAST(ToLang);
+    TU *FromTU = findFromTU(TUDecl);
+    return FromTU->import(ToAST.get(), FromType);
    }
 
   ~ASTImporterTestBase() {
     if (!::testing::Test::HasFailure()) return;
 
     for (auto &Tu : FromTUs) {
       assert(Tu.Unit);
       llvm::errs() << "FromAST:\n";
       Tu.Unit->getASTContext().getTranslationUnitDecl()->dump();
testImport(
       "};"
       "void f() { declToImport<int>().m(); }",
       Lang_CXX11, "", Lang_CXX11, Verifier,
       classTemplateDecl(has(cxxRecordDecl(has(
           functionDecl(hasDescendant(
               typeTraitExpr(hasType(booleanType())))))))));
 }
 
+TEST_P(ImportDecl, ImportRecordDeclInFunc) {
+  MatchVerifier<Decl> Verifier;
+  testImport("int declToImport() { "
+             "  struct data_t {int a;int b;};"
+             "  struct data_t d;"
+             "  return 0;"
+             "}",
+             Lang_C, "", Lang_C, Verifier,
+             functionDecl(hasBody(compoundStmt(
+                 has(declStmt(hasSingleDecl(varDecl(hasName("d")))))))));
+}
+
+TEST_P(ASTImporterTestBase, ImportRecordTypeInFunc) {
+  Decl *FromTU = getTuDecl("int declToImport() { "
+                           "  struct data_t {int a;int b;};"
+                           "  struct data_t d;"
+                           "  return 0;"
+                           "}",
+                           Lang_C, "input.c");
+  auto FromVar =
+      FirstDeclMatcher<VarDecl>().match(FromTU, varDecl(hasName("d")));
+  ASSERT_TRUE(FromVar);
+  auto ToType =
+      ImportType(FromVar->getType().getCanonicalType(), FromVar, Lang_C);
+  EXPECT_FALSE(ToType.isNull());
+}
+
+TEST_P(ASTImporterTestBase, ImportRecordDeclInFuncParams) {
+  // This construct is not supported by ASTImporter.
+  Decl *FromTU =
+      getTuDecl("int declToImport(struct data_t{int a;int b;} *d){ return 0; }",
+                Lang_C, "input.c");
+  auto From = FirstDeclMatcher<FunctionDecl>().match(FromTU, functionDecl());
+  ASSERT_TRUE(From);
+  auto To = Import(From, Lang_C);
+  EXPECT_EQ(To, nullptr);
+}
+
 const internal::VariadicDynCastAllOfMatcher<Expr, CXXPseudoDestructorExpr>
     cxxPseudoDestructorExpr;
 
 TEST_P(ImportExpr, ImportCXXPseudoDestructorExpr) {
   MatchVerifier<Decl> Verifier;
   testImport(
       "typedef int T;"
       "void declToImport(int *p) {"