[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

[Aleksei Sidorin via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
a.sidorin marked an inline comment as done.
Closed by commit rL330704: [ASTImporter] Allow testing of import sequences; fix 
import of typedefs for… (authored by a.sidorin, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D44079?vs=141537=143702#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D44079

Files:
  cfe/trunk/include/clang/AST/ASTImporter.h
  cfe/trunk/lib/AST/ASTImporter.cpp
  cfe/trunk/test/Analysis/Inputs/ctu-other.cpp
  cfe/trunk/test/Analysis/Inputs/externalFnMap.txt
  cfe/trunk/test/Analysis/ctu-main.cpp
  cfe/trunk/unittests/AST/ASTImporterTest.cpp

Index: cfe/trunk/unittests/AST/ASTImporterTest.cpp
===
--- cfe/trunk/unittests/AST/ASTImporterTest.cpp
+++ cfe/trunk/unittests/AST/ASTImporterTest.cpp
@@ -20,10 +20,15 @@
 
 #include "DeclMatcher.h"
 #include "gtest/gtest.h"
+#include "llvm/ADT/StringMap.h"
 
 namespace clang {
 namespace ast_matchers {
 
+using internal::Matcher;
+using internal::BindableMatcher;
+using llvm::StringMap;
+
 typedef std::vector ArgVector;
 typedef std::vector RunOptions;
 
@@ -70,22 +75,61 @@
 
 // Creates a virtual file and assigns that to the context of given AST. If the
 // file already exists then the file will not be created again as a duplicate.
-static void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
-  const std::string ) {
+static void
+createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
+  std::unique_ptr &) {
   assert(ToAST);
   ASTContext  = ToAST->getASTContext();
   auto *OFS = static_cast(
   ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
   auto *MFS =
   static_cast(OFS->overlays_begin()->get());
-  MFS->addFile(FileName, 0, llvm::MemoryBuffer::getMemBuffer(Code.c_str()));
+  MFS->addFile(FileName, 0, std::move(Buffer));
 }
 
-template
+static void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
+  StringRef Code) {
+  return createVirtualFileIfNeeded(ToAST, FileName,
+   llvm::MemoryBuffer::getMemBuffer(Code));
+}
+
+template 
+NodeType importNode(ASTUnit *From, ASTUnit *To, ASTImporter ,
+NodeType Node) {
+  ASTContext  = To->getASTContext();
+
+  // Add 'From' file to virtual file system so importer can 'find' it
+  // while importing SourceLocations. It is safe to add same file multiple
+  // times - it just isn't replaced.
+  StringRef FromFileName = From->getMainFileName();
+  createVirtualFileIfNeeded(To, FromFileName,
+From->getBufferForFile(FromFileName));
+
+  auto Imported = Importer.Import(Node);
+
+  // This should dump source locations and assert if some source locations
+  // were not imported.
+  SmallString<1024> ImportChecker;
+  llvm::raw_svector_ostream ToNothing(ImportChecker);
+  ToCtx.getTranslationUnitDecl()->print(ToNothing);
+
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.
+  Imported->dump(ToNothing);
+
+  return Imported;
+}
+
+const StringRef DeclToImportID = "declToImport";
+const StringRef DeclToVerifyID = "declToVerify";
+
+template 
 testing::AssertionResult
 testImport(const std::string , const ArgVector ,
const std::string , const ArgVector ,
-   MatchVerifier , const MatcherType ) {
+   MatchVerifier ,
+   const BindableMatcher ,
+   const BindableMatcher ) {
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 
@@ -97,46 +141,47 @@
   ASTContext  = FromAST->getASTContext(),
= ToAST->getASTContext();
 
-  // Add input.cc to virtual file system so importer can 'find' it
-  // while importing SourceLocations.
-  createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromCode);
-
   ASTImporter Importer(ToCtx, ToAST->getFileManager(),
FromCtx, FromAST->getFileManager(), false);
 
-  IdentifierInfo *ImportedII = ("declToImport");
-  assert(ImportedII && "Declaration with 'declToImport' name"
-   "should be specified in test!");
-  DeclarationName ImportDeclName(ImportedII);
-  SmallVector FoundDecls;
-  FromCtx.getTranslationUnitDecl()->localUncachedLookup(
-ImportDeclName, FoundDecls);
-
-  if (FoundDecls.size() != 1)
-return testing::AssertionFailure() << "Multiple declarations were found!";
+  auto FoundNodes = match(SearchMatcher, FromCtx);
+  if (FoundNodes.size() != 1)
+return testing::AssertionFailure()
+   << "Multiple potential nodes were found!";
+
+  auto ToImport = selectFirst(DeclToImportID, FoundNodes);
+  if (!ToImport)
+return testing::AssertionFailure() << "Node type mismatch!";
 
   // Sanity check: the node being imported should 

[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin marked 2 inline comments as done.
a.sidorin added a comment.

Thank you Gabor!




Comment at: unittests/AST/ASTImporterTest.cpp:356
+  ImportAction(StringRef FromFilename, StringRef ToFilename,
+   const std::string )
+  : FromFilename(FromFilename), ToFilename(ToFilename),

xazax.hun wrote:
> Maybe using StringRef for DeclName too?
I don't think it has too much sense - `hasName()` matcher's argument is `const 
std::string &` anyway.



Comment at: unittests/AST/ASTImporterTest.cpp:416
+  AllASTUnits[Filename] = Found->getValue().createASTUnits(Filename);
+  BuiltASTs[Filename] = true;
+}

xazax.hun wrote:
> Do we need this? Can't we just say if a filename is in `AllASTUnits`, it is 
> built?
Yes, that's actually redundant.


Repository:
  rC Clang

https://reviews.llvm.org/D44079



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

[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

Overall looks good, some minor nits inline.  If those can be resolved 
trivially, I am ok with committing this without another round of reviews.




Comment at: unittests/AST/ASTImporterTest.cpp:91
+static void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
+  const std::string ) {
+  return createVirtualFileIfNeeded(

If we already touching this part, maybe better to take the code as a StringRef?



Comment at: unittests/AST/ASTImporterTest.cpp:356
+  ImportAction(StringRef FromFilename, StringRef ToFilename,
+   const std::string )
+  : FromFilename(FromFilename), ToFilename(ToFilename),

Maybe using StringRef for DeclName too?



Comment at: unittests/AST/ASTImporterTest.cpp:416
+  AllASTUnits[Filename] = Found->getValue().createASTUnits(Filename);
+  BuiltASTs[Filename] = true;
+}

Do we need this? Can't we just say if a filename is in `AllASTUnits`, it is 
built?


Repository:
  rC Clang

https://reviews.llvm.org/D44079



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

[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin updated this revision to Diff 141537.
a.sidorin added a comment.

Rebase to the latest master; add a suggestion from Adam Balogh (thanks!).


Repository:
  rC Clang

https://reviews.llvm.org/D44079

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  test/Analysis/Inputs/ctu-other.cpp
  test/Analysis/Inputs/externalFnMap.txt
  test/Analysis/ctu-main.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -20,10 +20,15 @@
 
 #include "DeclMatcher.h"
 #include "gtest/gtest.h"
+#include "llvm/ADT/StringMap.h"
 
 namespace clang {
 namespace ast_matchers {
 
+using internal::Matcher;
+using internal::BindableMatcher;
+using llvm::StringMap;
+
 typedef std::vector ArgVector;
 typedef std::vector RunOptions;
 
@@ -70,22 +75,61 @@
 
 // Creates a virtual file and assigns that to the context of given AST. If the
 // file already exists then the file will not be created again as a duplicate.
-static void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
-  const std::string ) {
+static void
+createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
+  std::unique_ptr &) {
   assert(ToAST);
   ASTContext  = ToAST->getASTContext();
   auto *OFS = static_cast(
   ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
   auto *MFS =
   static_cast(OFS->overlays_begin()->get());
-  MFS->addFile(FileName, 0, llvm::MemoryBuffer::getMemBuffer(Code.c_str()));
+  MFS->addFile(FileName, 0, std::move(Buffer));
+}
+
+static void createVirtualFileIfNeeded(ASTUnit *ToAST, StringRef FileName,
+  const std::string ) {
+  return createVirtualFileIfNeeded(
+  ToAST, FileName, llvm::MemoryBuffer::getMemBuffer(Code.c_str()));
 }
 
-template
+template 
+NodeType importNode(ASTUnit *From, ASTUnit *To, ASTImporter ,
+NodeType Node) {
+  ASTContext  = To->getASTContext();
+
+  // Add 'From' file to virtual file system so importer can 'find' it
+  // while importing SourceLocations. It is safe to add same file multiple
+  // times - it just isn't replaced.
+  StringRef FromFileName = From->getMainFileName();
+  createVirtualFileIfNeeded(To, FromFileName,
+From->getBufferForFile(FromFileName));
+
+  auto Imported = Importer.Import(Node);
+
+  // This should dump source locations and assert if some source locations
+  // were not imported.
+  SmallString<1024> ImportChecker;
+  llvm::raw_svector_ostream ToNothing(ImportChecker);
+  ToCtx.getTranslationUnitDecl()->print(ToNothing);
+
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.
+  Imported->dump(ToNothing);
+
+  return Imported;
+}
+
+const StringRef DeclToImportID = "declToImport";
+const StringRef DeclToVerifyID = "declToVerify";
+
+template 
 testing::AssertionResult
 testImport(const std::string , const ArgVector ,
const std::string , const ArgVector ,
-   MatchVerifier , const MatcherType ) {
+   MatchVerifier ,
+   const BindableMatcher ,
+   const BindableMatcher ) {
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 
@@ -97,46 +141,47 @@
   ASTContext  = FromAST->getASTContext(),
= ToAST->getASTContext();
 
-  // Add input.cc to virtual file system so importer can 'find' it
-  // while importing SourceLocations.
-  createVirtualFileIfNeeded(ToAST.get(), InputFileName, FromCode);
-
   ASTImporter Importer(ToCtx, ToAST->getFileManager(),
FromCtx, FromAST->getFileManager(), false);
 
-  IdentifierInfo *ImportedII = ("declToImport");
-  assert(ImportedII && "Declaration with 'declToImport' name"
-   "should be specified in test!");
-  DeclarationName ImportDeclName(ImportedII);
-  SmallVector FoundDecls;
-  FromCtx.getTranslationUnitDecl()->localUncachedLookup(
-ImportDeclName, FoundDecls);
+  auto FoundNodes = match(SearchMatcher, FromCtx);
+  if (FoundNodes.size() != 1)
+return testing::AssertionFailure()
+   << "Multiple potential nodes were found!";
 
-  if (FoundDecls.size() != 1)
-return testing::AssertionFailure() << "Multiple declarations were found!";
+  auto ToImport = selectFirst(DeclToImportID, FoundNodes);
+  if (!ToImport)
+return testing::AssertionFailure() << "Node type mismatch!";
 
   // Sanity check: the node being imported should match in the same way as
   // the result node.
-  EXPECT_TRUE(Verifier.match(FoundDecls.front(), AMatcher));
+  BindableMatcher WrapperMatcher(VerificationMatcher);
+  EXPECT_TRUE(Verifier.match(ToImport, WrapperMatcher));
 
-  auto Imported = Importer.Import(FoundDecls.front());
+  auto Imported = importNode(FromAST.get(), ToAST.get(), 

[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

By moving the code that sets the type name of an anoynmous declaration from 
Import(Decl*) to ImportDefinition(RecordDecl*, RecordDecl*, 
ImportDefinitionKind) (and the same for Enum) we will not crash upon importing 
typedefs containing anonymous strcutures. This is a patch-over-patch for it, 
including the test cases:

F5887321: D44079-Mod.diff 


Repository:
  rC Clang

https://reviews.llvm.org/D44079



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

[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

Hi,

Thank you for the patch.

It seems that the new approach still does not solve a problem with anonymous 
structures in typedefs.In C++ a copy constructor is automatically generated, 
and its parameter is the anonymous structure itself. This triggers caching 
`NoLinkage` for it during the import of the constructor, but later fails with 
an assert because at the end the computed linkage is `ExternalLinkage`. It 
seems that anonymous structures are somewhat tricky in the original AST. For 
this struct:

  typedef struct {
int n;
  } T;

the original AST is:

  CXXRecordDecl 0x1abcb38  line:1:9 imported 
struct definition
  |-FieldDecl 0x1abce68  col:7 imported n 'int'
  |-CXXConstructorDecl 0x1abced0  col:9 imported implicit used  'void 
(void) throw()' inline default trivial
  | `-CompoundStmt 0x1abd2e0 
  `-CXXConstructorDecl 0x1abcfd8  col:9 imported implicit  'void (const 
T &)' inline default trivial noexcept-unevaluated 0x1abcfd8
`-ParmVarDecl 0x1abd138  col:9 imported 'const T &'
  TypedefDecl 0x1abcc78  col:3 imported 
referenced T 'struct T':'T'
  `-ElaboratedType 0x1abccd0 'struct T' sugar imported
`-RecordType 0x1abcc50 'T' imported
  `-CXXRecord 0x1abcb38 ''

But the imported one is:

  CXXRecordDecl 0x1a51400  col:9 struct definition
  |-FieldDecl 0x1a51540  col:7 n 'int'
  |-CXXConstructorDecl 0x1a515e0  col:9 implicit used  'void (void) 
throw()' inline trivial
  | `-CompoundStmt 0x1a51688 
  `-CXXConstructorDecl 0x1a51768  col:9 implicit  'void (const struct 
(anonymous at /tmp/first.cpp:1:9) &)' inline trivial noexcept-unevaluated 
0x1a51768
`-ParmVarDecl 0x1a51708  col:9 'const struct (anonymous at 
/tmp/first.cpp:1:9) &'
  TypedefDecl 0x1a518b0  col:3 T 'struct 
(anonymous struct at /tmp/first.cpp:1:9)':'struct (anonymous at 
/tmp/first.cpp:1:9)'
  `-ElaboratedType 0x1a51860 'struct (anonymous struct at /tmp/first.cpp:1:9)' 
sugar
`-RecordType 0x1a514a0 'struct (anonymous at /tmp/first.cpp:1:9)'
  `-CXXRecord 0x1a51400 ''


Repository:
  rC Clang

https://reviews.llvm.org/D44079



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

[PATCH] D44079: [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin created this revision.
a.sidorin added reviewers: r.stahl, xazax.hun, jingham, szepet.
Herald added subscribers: martong, rnkovacs.

  This patch introduces the ability to test an arbitrary sequence of imports
  between a given set of virtual source files. This should finally allow
  us to write simple tests and fix annoying issues inside ASTImporter
  that cause failures in CSA CTU. This is done by refactoring
  ASTImporterTest functions and introducing `testImportSequence` facility.
  As a side effect, `testImport` facility was generalized a bit more. It
  should now allow import of non-decl AST nodes; however, there is still no
  test using this ability.
  
  As a "test for test", there is also a fix for import anonymous TagDecls
  referred by typedef. Before this patch, the setting of typedef for anonymous
  structure was delayed; however, this approach misses the corner case if
  an enum constant is imported directly. In this patch, typedefs for
  anonymous declarations are imported right after the anonymous declaration
  is imported, without any delay.


Repository:
  rC Clang

https://reviews.llvm.org/D44079

Files:
  include/clang/AST/ASTImporter.h
  lib/AST/ASTImporter.cpp
  unittests/AST/ASTImporterTest.cpp

Index: unittests/AST/ASTImporterTest.cpp
===
--- unittests/AST/ASTImporterTest.cpp
+++ unittests/AST/ASTImporterTest.cpp
@@ -18,10 +18,15 @@
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include "llvm/ADT/StringMap.h"
 
 namespace clang {
 namespace ast_matchers {
 
+using internal::Matcher;
+using internal::BindableMatcher;
+using llvm::StringMap;
+
 typedef std::vector ArgVector;
 typedef std::vector RunOptions;
 
@@ -61,11 +66,48 @@
   return {BasicArgs};
 }
 
-template
+template 
+NodeType importNode(ASTUnit *From, ASTUnit *To, ASTImporter ,
+NodeType Node) {
+  ASTContext  = To->getASTContext();
+
+  // Add 'From' file to virtual file system so importer can 'find' it
+  // while importing SourceLocations. It is safe to add same file multiple
+  // times - it just isn't replaced.
+  vfs::OverlayFileSystem *OFS = static_cast(
+  ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
+  vfs::InMemoryFileSystem *MFS =
+  static_cast(OFS->overlays_begin()->get());
+
+  StringRef FromFileName = From->getMainFileName();
+  MFS->addFile(FromFileName, 0, From->getBufferForFile(FromFileName));
+
+  auto Imported = Importer.Import(Node);
+
+  // This should dump source locations and assert if some source locations
+  // were not imported.
+  SmallString<1024> ImportChecker;
+  llvm::raw_svector_ostream ToNothing(ImportChecker);
+  ToCtx.getTranslationUnitDecl()->print(ToNothing);
+
+  // This traverses the AST to catch certain bugs like poorly or not
+  // implemented subtrees.
+  Imported->dump(ToNothing);
+
+  return Imported;
+}
+
+
+const StringRef DeclToImportID = "declToImport";
+const StringRef DeclToVerifyID = "declToVerify";
+
+template 
 testing::AssertionResult
 testImport(const std::string , const ArgVector ,
const std::string , const ArgVector ,
-   MatchVerifier , const MatcherType ) {
+   MatchVerifier ,
+   const BindableMatcher ,
+   const BindableMatcher ) {
   const char *const InputFileName = "input.cc";
   const char *const OutputFileName = "output.cc";
 
@@ -77,50 +119,47 @@
   ASTContext  = FromAST->getASTContext(),
= ToAST->getASTContext();
 
-  // Add input.cc to virtual file system so importer can 'find' it
-  // while importing SourceLocations.
-  vfs::OverlayFileSystem *OFS = static_cast(
-ToCtx.getSourceManager().getFileManager().getVirtualFileSystem().get());
-  vfs::InMemoryFileSystem *MFS = static_cast(
-OFS->overlays_begin()->get());
-  MFS->addFile(InputFileName, 0, llvm::MemoryBuffer::getMemBuffer(FromCode));
-
   ASTImporter Importer(ToCtx, ToAST->getFileManager(),
FromCtx, FromAST->getFileManager(), false);
 
-  IdentifierInfo *ImportedII = ("declToImport");
-  assert(ImportedII && "Declaration with 'declToImport' name"
-   "should be specified in test!");
-  DeclarationName ImportDeclName(ImportedII);
-  SmallVector FoundDecls;
-  FromCtx.getTranslationUnitDecl()->localUncachedLookup(
-ImportDeclName, FoundDecls);
+  auto FoundNodes = match(SearchMatcher, FromCtx);
+  if (FoundNodes.size() != 1)
+return testing::AssertionFailure()
+   << "Multiple potential nodes were found!";
 
-  if (FoundDecls.size() != 1)
-return testing::AssertionFailure() << "Multiple declarations were found!";
+  auto ToImport = selectFirst(DeclToImportID, FoundNodes);
+  if (!ToImport)
+return testing::AssertionFailure() << "Node type mismatch!";
 
   // Sanity check: the node being imported should match in the same way as
   // the result node.
-