Page MenuHomePhabricator

ASTImporter: expressions, pt.1
ClosedPublic

Authored by a.sidorin on Nov 3 2015, 7:58 AM.

Details

Summary

This patch implements some expression-related AST node import. This is the first patch in a series.
http://reviews.llvm.org/D14224 was also partially used.

  • Introduce ASTImporter unit test framework
  • Support new node kinds:

InjectedClassNameType
TemplateTypeParmType
LabelDecl
GCCAsmStmt
VAArgExpr
GNUNullExpr
PredefinedExpr
InitListExpr
ImplicitValueInitExpr
DesignatedInitExpr
CXXNullPtrLiteralExpr
CXXBoolLiteralExpr
FloatingLiteral
StringLiteral
CompoundLiteralExpr
AtomicExpr
AddrLabelExpr
ParenListExpr
StmtExpr
ConditionalOperator
BinaryConditionalOperator
OpaqueValueExpr
CXXThisExpr

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin updated this revision to Diff 39064.Nov 3 2015, 7:58 AM
a.sidorin retitled this revision from to ASTImporter: expressions, pt.1.
a.sidorin updated this object.
a.sidorin added a reviewer: sepavloff.
a.sidorin set the repository for this revision to rL LLVM.
a.sidorin added a subscriber: cfe-commits.
a.sidorin updated this object.Nov 3 2015, 8:03 AM
sepavloff edited edge metadata.Nov 6 2015, 2:55 AM

The fix must contain tests.

lib/AST/ASTImporter.cpp
35 ↗(On Diff #39064)

A name started with underscore is a reserved identifier (see C++ standard, [global.names]), so it is better to use something more neutral, like TheImporter or Imp or something else.

43 ↗(On Diff #39064)

I would propose more descriptive name like containsNullptr and move it into anonymous namespace, or maybe eliminated this function at all, and use std::find directly.

48 ↗(On Diff #39064)

This function is used in one place only, maybe inline its body in that place?

4642 ↗(On Diff #39064)

Check for S->getOutputIdentifier(i) seems redundant, output name cannot be omitted in asm statement.

4648 ↗(On Diff #39064)

Same as above.

4655 ↗(On Diff #39064)

Again, clobber cannot be null, maybe cast instead of cast_or_null?

4664 ↗(On Diff #39064)

cast_or_null -> cast?

4672 ↗(On Diff #39064)

cast_or_null -> cast?

4693 ↗(On Diff #39064)

cast_or_null -> cast?

5248 ↗(On Diff #39064)

Check for ILE->getArrayFiller() is useless, as in this branch ILE->hasArrayFiller() is true.

5253–5257 ↗(On Diff #39064)

Call to setInitializedFieldInUnion will always set type of respective union to declaration, so if it contained an expression, it will be lost. The safer solution is to check the kind of the source union and import either declaration or expression depending on the result.

5301 ↗(On Diff #39064)

According to LLVM coding standard variable names must start with upper case letter, so i -> I, e->E.

a.sidorin marked 7 inline comments as done.Nov 6 2015, 6:28 AM

Thank you for your comments. I leaved some replies and will update revision.
Something about lacking tests.

  1. We cannot check if expression import is correct until we implement FunctionDecl body import. I was going to upstream Decl-related parts later but maybe it is better to include this small case in the first patch. What do you think?
  2. What is the best way to test import of statements? Currently I have no idea how to do this (except of ast-dump). Any suggestions are welcome.
lib/AST/ASTImporter.cpp
35 ↗(On Diff #39064)

Ouch. I was just trying to unify this with the code already existing in ASTImporter. I'll rename this.
// TODO: refactor std::transform usage in ASTImporter.

48 ↗(On Diff #39064)

I'll use it in later patches so I'd prefer to keep it.

4655 ↗(On Diff #39064)

This guard is here because the return result of import may be null. This cast_or_null (and some others) handles such cases.

a.sidorin updated this revision to Diff 39517.Nov 6 2015, 6:32 AM
a.sidorin edited edge metadata.
a.sidorin marked an inline comment as done.

Some issues pointed on review were fixed.

a.sidorin updated this revision to Diff 40769.Nov 20 2015, 5:33 AM
a.sidorin removed rL LLVM as the repository for this revision.

Seems like I have found a way to test ASTImporter. What about some unit-tests? A sample test using AST matcher is attached. If it is OK, I'll write more tests with it.

a.sidorin updated this revision to Diff 40775.Nov 20 2015, 6:30 AM

Add language-related arguments for compilation.

Unit test, of course, checks import facility but i would say this is overkill. You need to create tests for all nodes you implement in imported, this is huge amount of work.

I would propose you to move tests from http://reviews.llvm.org/D14224 for nodes that are implemented there, for remaining nodes create short functions, that have respective node in the body, similar to what is done in D14224. Such test checks that the node is handled in Importer. For regression test it must be enough. More rigorous check must can be done in runtime, or in constexpr function, for some nodes. Unfortunately it looks like constexpr attribute is not preserved correctly by the importer class, so we have no choice here.

I understand your position. However, we cannot check that we don't import BreakStmt as ContinueStmt using just ASTMerge without any verification. As I understand, unit tests is what we need because we need to check that the overall imported type/expression is correct. And I think that we really need to create tests for all nodes with import implemented since that is a target of tests (yes, it can take some time, but I think it is strictly required). With static_assert we can perform only a limited amount of checks so I decided to use ASTMatchers.
But I think I can just use both approaches: I may use your tests and keep this unit test framework as well to perform more strict checks.

If you are going to make unit tests, consider alternative approach also. In repository test-suite the directory SingleSource/UnitTests contains runtime tests obtained from single file. You could modify Makefile that runs these tests in such a way that each test runs twice, one as now it runs and the other would run through the sequence compile to ast - compile empty source with with ast-import. Tests for missing nodes could be added into SingleSource/UnitTests, they would be substantially simpler that those using ASTMatchers. More importantly, such tests would make indeed exhaustive testing of importer.

a.sidorin updated this revision to Diff 48487.Feb 19 2016, 6:48 AM
a.sidorin updated this object.
a.sidorin set the repository for this revision to rL LLVM.

Add AST matcher-based unit tests; add some additional nodes to pass the tests.

Serge Pavlov: I'll enable tests from you patch (http://reviews.llvm.org/D14224) after all node types your patch supports will be supported. If you're agree, of course.

Serge: Sorry, that's because I forgot to add a dependence. There should be a patch implementing these matchers.

I still cannot build project with your changes, now compiler cannot find symbol hasSubStmt. When committing the change please make sure that all prerequisites are committed and unit tests run successfully.

I would recommend you to take tests from D14224 for literals. They check imported values as well, not only syntax structure. It is however only a recommendation.

There is no tests for InjectedClassNameType, TemplateTypeParmType, GCCAsmStmt, VAArgExpr and CXXBoolLiteralExpr. Probably these nodes may be moved to next code change?

Provided that the aforementioned nodes get tests of are moved to another change, the patch looks good for me. I do not have authority to approve patches for commit, you need to ask someone with proper rights for the approval.

Thank you!

lib/AST/ASTImporter.cpp
1827–1828 ↗(On Diff #48487)

Definition of ASTContext::getInjectedClassNameType does not contain comments that would explain why it is unsuitable, so this comment is confusing. Probably you wanted to reference ASTReader::readTypeRecord, it contains similar statement.

4379 ↗(On Diff #48487)

Comments should be proper English sentences. In this case ending period is missed.

4730 ↗(On Diff #48487)

According to LLVM coding style (http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly) variable names 'should be camel case, and start with an upper case letter', so i -> I, e -> E.

unittests/AST/ASTImporterTest.cpp
117 ↗(On Diff #48487)

Disadvantage of using matchers, apart of complexity, is that they check only syntax structure. This test does not check if imported literal has proper value.

a.sidorin updated this revision to Diff 50140.Mar 9 2016, 8:03 AM
a.sidorin removed rL LLVM as the repository for this revision.

Stylish fixes.

Serge: ASTMatcher review is complete so there should be no more build problems. Could you try again?
Also, I'd prefer to add your changes in the next patch, without splitting. About missing tests: they are planned in later patches too.

Serge: BTW, tests for CXXBoolLiteralExpr are presented in this patch. There is no test exactly for it but at least one test uses bool literal and matches it successfully with cxxBoolLiteral() matcher.

On Windows (VS2015 update1) two unit tests fail:

[ RUN      ] ImportExpr.ImportGNUNullExpr
input.cc:1:23: warning: expression result unused [-Wunused-value]
void declToImport() { __null; }
                      ^~~~~~
G:\arbeit\llvm2\llvm\tools\clang\unittests\AST\ASTImporterTest.cpp(149):
error: Value of: testImport("void declToImport() { __null; }", Lang_CXX,
"", Lang_CXX,
Verifier, functionDecl( hasBody( compoundStmt( has( gnuNullExpr( hasType(
asString("long"))))))))
  Actual: false (Could not find match)
Expected: true
[  FAILED  ] ImportExpr.ImportGNUNullExpr (61 ms)
[ RUN      ] ImportExpr.ImportParenListExpr
G:\arbeit\llvm2\llvm\tools\clang\unittests\AST\ASTImporterTest.cpp(284):
error: Value of: testImport( "  template<typename T> class declToImport {"
"    void f(
) { declToImport X(*this); } }; ", Lang_CXX, "", Lang_CXX, Verifier,
classTemplateDecl( has( cxxRecordDecl( hasMethod( allOf( hasName("f"),
hasBody( compoundStm
t( has( declStmt( hasSingleDecl( varDecl( hasInitializer( parenListExpr(
has( unaryOperator( hasOperatorName("*"), hasUnaryOperand(cxxThisExpr())
))))))))))))))
))
  Actual: false (Could not find match)
Expected: true
[  FAILED  ] ImportExpr.ImportParenListExpr (59 ms)
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] ImportExpr.ImportGNUNullExpr
[  FAILED  ] ImportExpr.ImportParenListExpr

 2 FAILED TESTS

Thanks,
--Serge

a.sidorin updated this revision to Diff 50741.Mar 15 2016, 8:54 AM

An attempt to fix ParenListExpr test on Windows

The last changes didn't help.

AST generated in ImportGNUNullExpr is:

TranslationUnitDecl 0xc46238 <<invalid sloc>> <invalid sloc>
…
|   `-BuiltinType 0xc46290 'char'
`-FunctionDecl 0xc468a0 <input.cc:1:1, col:31> col:6 declToImport 'void (void)'
  `-CompoundStmt 0xc46948 <col:21, col:31>
    `-GNUNullExpr 0xc46938 <col:23> 'int'

Note, type of GNUNullExpr is int but you check for long.
Similarly, source code in the check ImportParenListExpr produces AST:

TranslationUnitDecl 0xc47340 <<invalid sloc>> <invalid sloc>
…
| `-ClassTemplateSpecializationDecl 0xc47d00 <col:1, col:66> col:28 class dummy
|   `-TemplateArgument type 'int'
`-TypedefDecl 0xc47e38 <col:68, col:87> col:87 declToImport 'dummy<int>':'class dummy<int>'
  `-TemplateSpecializationType 0xc47db0 'dummy<int>' sugar dummy
    |-TemplateArgument type 'int'
    `-RecordType 0xc47da0 'class dummy<int>'
      `-ClassTemplateSpecialization 0xc47d00 'dummy'

There is no ParenListExpr, - templates are handled in msvc compatibility mode a bit differently.
Probably the tests could be fixed by hard coding target to Linux64.

a.sidorin updated this revision to Diff 51027.Mar 18 2016, 8:44 AM

Serge, thank you for help!
GNUNullExpr was 'long' on my platform. I weakened a matcher to check only if it has integer type.
ParenListExpr issue was also successfully reproduced with 'fdelayed-template-parsing' and fixed.
I hope this version passes all the tests. Could you recheck? I cannot gain access to MSVC now.

With the latest changes unit tests are successfully passed on Windows!
You can go forward in completing the fix.

Thanks,
--Serge

a.sidorin updated this object.Mar 21 2016, 4:35 AM

Is this patch OK to submit?

spyffe requested changes to this revision.Mar 29 2016, 11:12 AM
spyffe edited edge metadata.

Overall this is a great improvement and I look forward to taking advantage of this patch in LLDB!

I had one specific nit about NullPtrTy, and one more general comment about your infrastructure that imports multiple things – it seems like we can collaborate and create some combination of ImportArray and your code to handle all cases. Particularly you handle the cases where the array doesn't have to be allocated inside getToContext() – my code doesn't handle that at all.

Thanks for working on this.

lib/AST/ASTImporter.cpp
35 ↗(On Diff #51027)

I recently added a function, ImportArray, that does something similar. Could it be adapted to your needs?

5462 ↗(On Diff #51027)

I'm worried about using NullPtrTy here directly, because the type may have been coerced to a typedef or something like that. The fact that the constructor takes a type argument suggests that the type is not implicit – I'd feel much more comfortable just importing the type.

5466 ↗(On Diff #51027)

I recently committed VisitCXXBoolLiteralExpr. Sorry to step on your toes!

5878 ↗(On Diff #51027)

Sorry again! I recently committed VisitCXXThisExpr

unittests/AST/ASTImporterTest.cpp
118 ↗(On Diff #51027)

This is a good point; if you use LLDB to test this (e.g., lldb's test/testcases/expression_command/top-level) then you can verify that these imported entities make it all the way to generated machine code and work as expected.

Ideally there would be a mode in Clang that parses the input file, makes a translation unit out of it, imports everything from that translation unit into another translation unit, and then CodeGens the second translation unit. That is something I'd like to hook up when I get some time. I've also talked to Doug Gregor and he suggested that perhaps .pch files can be abused for this purpose, repurposing some of the functionality in the ASTMerge tests (though those don't do CodeGen).

This revision now requires changes to proceed.Mar 29 2016, 11:12 AM
a.sidorin updated this revision to Diff 53389.Apr 12 2016, 5:46 AM
a.sidorin edited edge metadata.

Update to current master;
Fix a memory leak introduced in cf8ccff5: an array is allocated in ImportArray and never freed.

spyffe added a subscriber: spyffe.Apr 12 2016, 11:01 AM

Aleksei,

it looks like I might have made a bad assumption in my ImportArray – namely that there were some entities that required arrays of things they’re constructed with to be allocated in the ASTContext. Looking at the constructors for those entities, it looks like most of them actually do the allocation and copying themselves.

Thanks for cleaning that up. I’m going to try running the LLDB testsuite with your patch applied and I’ll let you know what happens.

Sean

spyffe accepted this revision.Apr 12 2016, 11:46 AM
spyffe edited edge metadata.

This all looks fine to me, and the LLDB test suite is happy also.

This revision is now accepted and ready to land.Apr 12 2016, 11:46 AM
This revision was automatically updated to reflect the committed changes.