This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls
ClosedPublic

Authored by a.sidorin on Mar 4 2018, 9:41 AM.

Details

Summary
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.

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin created this revision.Mar 4 2018, 9:41 AM

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 </tmp/first.cpp:1:9, line:3:1> line:1:9 imported struct definition
|-FieldDecl 0x1abce68 <line:2:3, col:7> col:7 imported n 'int'
|-CXXConstructorDecl 0x1abced0 <line:1:9> col:9 imported implicit used  'void (void) throw()' inline default trivial
| `-CompoundStmt 0x1abd2e0 <col:9>
`-CXXConstructorDecl 0x1abcfd8 <col:9> col:9 imported implicit  'void (const T &)' inline default trivial noexcept-unevaluated 0x1abcfd8
  `-ParmVarDecl 0x1abd138 <col:9> col:9 imported 'const T &'
TypedefDecl 0x1abcc78 </tmp/first.cpp:1:1, line:3:3> 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 </tmp/first.cpp:1:9> col:9 struct definition
|-FieldDecl 0x1a51540 <line:2:3, col:7> col:7 n 'int'
|-CXXConstructorDecl 0x1a515e0 <line:1:9> col:9 implicit used  'void (void) throw()' inline trivial
| `-CompoundStmt 0x1a51688 <col:9>
`-CXXConstructorDecl 0x1a51768 <col:9> col:9 implicit  'void (const struct (anonymous at /tmp/first.cpp:1:9) &)' inline trivial noexcept-unevaluated 0x1a51768
  `-ParmVarDecl 0x1a51708 <col:9> col:9 'const struct (anonymous at /tmp/first.cpp:1:9) &'
TypedefDecl 0x1a518b0 </tmp/first.cpp:1:1, line:3:3> 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 ''

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:

a.sidorin updated this revision to Diff 141537.Apr 8 2018, 6:47 AM

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

xazax.hun accepted this revision.Apr 12 2018, 9:15 AM

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

unittests/AST/ASTImporterTest.cpp
91 ↗(On Diff #141537)

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

356 ↗(On Diff #141537)

Maybe using StringRef for DeclName too?

416 ↗(On Diff #141537)

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

This revision is now accepted and ready to land.Apr 12 2018, 9:15 AM
a.sidorin marked 2 inline comments as done.Apr 23 2018, 6:49 AM

Thank you Gabor!

unittests/AST/ASTImporterTest.cpp
356 ↗(On Diff #141537)

I don't think it has too much sense - hasName() matcher's argument is const std::string & anyway.

416 ↗(On Diff #141537)

Yes, that's actually redundant.

This revision was automatically updated to reflect the committed changes.
a.sidorin marked an inline comment as done.