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

2018-07-12 Thread Gabor Marton via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL336898: [ASTImporter] Fix infinite recursion on function 
import with struct definition… (authored by martong, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D47946?vs=154991=155151#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47946

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -1138,8 +1138,26 @@
   DeclarationName , 
   NamedDecl *,
   SourceLocation ) {
+  // 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(D) && (FunDecl = dyn_cast(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;
   
Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -301,14 +301,25 @@
   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(>getASTContext() == >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);
 }
   };
 
@@ -321,6 +332,26 @@
   // vector is expanding, with the list we won't have these issues.
   std::list 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 ) {
+  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 ToAST;
@@ -394,26 +425,17 @@
   // 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();
-}
-
-// 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 ) {
-  return E.TUDecl == From->getTranslationUnitDecl();
-});
-assert(It != FromTUs.end());
-createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-
-return It->import(ToAST.get(), From);
+lazyInitToAST(ToLang);
+TU *FromTU = findFromTU(From);
+return FromTU->import(ToAST.get(), From);
   }
 
+  QualType ImportType(QualType FromType, Decl *TUDecl, Language ToLang) {
+lazyInitToAST(ToLang);
+TU *FromTU = findFromTU(TUDecl);
+return 

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

2018-07-12 Thread Zoltán Gera via Phabricator via cfe-commits
gerazo added a comment.

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


https://reviews.llvm.org/D47946



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-07-11 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

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


https://reviews.llvm.org/D47946



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-07-11 Thread Zoltán Gera via Phabricator via cfe-commits
gerazo updated this revision to Diff 154991.
gerazo marked an inline comment as done.
gerazo added a comment.

Minor fixes for Aleksei's comments.


https://reviews.llvm.org/D47946

Files:
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -284,19 +284,44 @@
   // Buffer for the To context, must live in the test scope.
   std::string ToCode;
 
+  // Represents a "From" translation unit and holds an importer object which we
+  // use to import from this translation unit.
   struct TU {
 // Buffer for the context, must live in the test scope.
 std::string Code;
 std::string FileName;
 std::unique_ptr Unit;
 TranslationUnitDecl *TUDecl = nullptr;
+std::unique_ptr Importer;
+
 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();
 }
+
+void lazyInitImporter(ASTUnit *ToAST) {
+  assert(ToAST);
+  if (!Importer) {
+Importer.reset(new ASTImporter(
+ToAST->getASTContext(), ToAST->getFileManager(),
+Unit->getASTContext(), Unit->getFileManager(), false));
+  }
+  assert(>getASTContext() == >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
@@ -308,6 +333,26 @@
   // vector is expanding, with the list we won't have these issues.
   std::list 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 ) {
+  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 ToAST;
@@ -329,14 +374,10 @@
 ToAST = tooling::buildASTFromCodeWithArgs(ToCode, ToArgs, OutputFileName);
 ToAST->enableSourceFileDiagnostics();
 
-ASTContext  = FromTU.Unit->getASTContext(),
-= ToAST->getASTContext();
+ASTContext  = FromTU.Unit->getASTContext();
 
 createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromTU.Code);
 
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
- FromTU.Unit->getFileManager(), false);
-
 IdentifierInfo *ImportedII = (Identifier);
 assert(ImportedII && "Declaration with the given identifier "
  "should be specified in test!");
@@ -347,7 +388,7 @@
 
 assert(FoundDecls.size() == 1);
 
-Decl *Imported = Importer.Import(FoundDecls.front());
+Decl *Imported = FromTU.import(ToAST.get(), FoundDecls.front());
 assert(Imported);
 return std::make_tuple(*FoundDecls.begin(), Imported);
   }
@@ -384,30 +425,17 @@
   // 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();
-}
-
-// 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 ) {
-  return E.TUDecl == From->getTranslationUnitDecl();
-});
-assert(It != FromTUs.end());
-createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
-
-ASTContext  = From->getASTContext(),
-= ToAST->getASTContext();
-ASTImporter Importer(ToCtx, ToAST->getFileManager(), FromCtx,
-

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

2018-07-11 Thread Zoltán Gera via Phabricator via cfe-commits
gerazo marked 2 inline comments as done.
gerazo added inline comments.



Comment at: unittests/AST/ASTImporterTest.cpp:234
+assert(ToAST);
+createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
+return &*It;

a.sidorin wrote:
> Can we move the side effect into import()?
Sure, thank you.


https://reviews.llvm.org/D47946



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-07-11 Thread Aleksei Sidorin via Phabricator via cfe-commits
a.sidorin accepted this revision.
a.sidorin added a comment.

Hello Zoltán,

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




Comment at: unittests/AST/ASTImporterTest.cpp:234
+assert(ToAST);
+createVirtualFileIfNeeded(ToAST.get(), It->FileName, It->Code);
+return &*It;

Can we move the side effect into import()?



Comment at: unittests/AST/ASTImporterTest.cpp:1049
+  ImportType(FromVar->getType().getCanonicalType(), FromVar, Lang_C);
+  ASSERT_FALSE(ToType.isNull());
+}

EXPECT_FALSE?


https://reviews.llvm.org/D47946



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


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

2018-07-10 Thread Gabor Marton via Phabricator via cfe-commits
martong added a comment.

Ping.


https://reviews.llvm.org/D47946



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits