Page MenuHomePhabricator

[ASTImporter] Various source location and range import fixes.
Needs ReviewPublic

Authored by balazske on Apr 10 2019, 1:44 AM.

Details

Summary

ASTImporter contained wrong or missing imports of SourceLocation
and SourceRange for some objects. At least a part of such errors
is fixed now.
Tests for SourceLocation import do not exist yet. A separate
patch is needed to add a general SourceLocation import test
that runs on every import as part of the already existing tests.

Diff Detail

Event Timeline

balazske created this revision.Apr 10 2019, 1:44 AM
balazske updated this revision to Diff 201521.May 27 2019, 6:29 AM
  • Import BraceRange of EnumDecl.

I don't see any regressions but I am a little uncomfortable since there are no tests. So I would feel better if this was split into three parts: Namespaces, Enums and Templates.

Are there small test programs that fail due to the missing data? We can add them as regression tests.

lib/AST/ASTImporter.cpp
6192–6193

Curious why you decided to add the new arguments to the front as opposed to the end?

7218

Can you explain why E->getNameInfo().getLoc() is more correct than E->getExprLoc()?

7293

We still want to import a few lines down even if !E->getTemplateKeywordLoc().isValid()?

balazske added a comment.EditedMay 29 2019, 12:27 AM

There is a test for the SourceLocation import in https://reviews.llvm.org/D60463 (but this test does not work until the ordering of imported Decl is same as original).

balazske marked 3 inline comments as done.May 29 2019, 2:04 AM
balazske added inline comments.
lib/AST/ASTImporter.cpp
6192–6193

This overload of ImportTemplateArgumentListInfo already exists before the patch. It looks like that the last argument is the output value and the arguments before are input values.

7218

The getExprLoc returns (if I am correct) the begin location and getNameInfo().getLoc() returns in "X<T>::value" location of "value" (this should not be the same as BeginLoc, may be it is the EndLoc?). (The begin and end location is not imported explicitly, it is obtained from other location values in the expression object.) I just observed that E->getLocation() can be used instead of E->getNameInfo().getLoc().
(The reason why this is correct that the test in D60463 indicates failure if getExprLoc is used, the begin and end locations are the same. That test works only in our Ericsson fork because Decl reordering issue.)

7293

The TemplateKeywordLoc can be invalid if the optional template keyword is missing. The import function can import an invalid SourceLocation (as invalid but not error).

So an alternative to testing could be matching the AST output from a clang-import-test like we did here:

https://reviews.llvm.org/D61140

although it unfortunately looks like only only the AST dump of NamespaceDecl output the SourceLocation you are fixing, see it on godbolt when I try a clang-import-test it looks like it is missing that SourceLocation:

|-NamespaceDecl 0x7f8c1f884750 <enum.cpp:1:1, <invalid sloc>> col:11

and with your patch applied I see the SourceLocation:

|-NamespaceDecl 0x7fe3e6882f50 <enum.cpp:1:1, line:7:1> line:1:11 A

I think the AST dump for EnumDecl and ClassTemplateSpecializationDecl should be dumping the missing SourceLocations but I am having trouble tracking down where that should be done.

It looks like that the same test can be applied as in D60463 but check only the first line of the AST dump. The first line contains information about the actual Decl only. This checks less than checking the full AST dump but finds some of wrong SourceLocation import. The problem is that the test can not be added yet because there are other failures. At least one new patch is needed with corrections related to import of TypeSourceInfo. Anyway this change (and the others) can not be added together with the test because multiple patches (including this) are needed to make the test not fail.

Source locations can be observed here:
http://ce.steveire.com/z/WB9VAu

shafik requested changes to this revision.May 30 2019, 11:47 AM

Actually I was mistaken, we can see the difference for EnumDecl and ClassTemplateSpecializationDecl as well.

For EnumDecl before:

EnumDecl 0x7fd0ae884800 <line:2:1, col:6> col:6 referenced B

After:

EnumDecl 0x7fa703882600 <line:2:1, line:6:1> line:2:6 referenced B

For ClassTemplateSpecializationDecl before:

ClassTemplateSpecializationDecl 0x7f8c338846a0 <line:4:1, line:5:7> col:7 class A definition

after:

ClassTemplateSpecializationDecl 0x7fca150832a0 <line:4:1, line:7:1> line:5:7 class A definition

So once we have tests similar to D61140 I would feel good about this change. Especially since it is not clear when the other changes will be in and tests are important to guard against regressions.

This revision now requires changes to proceed.May 30 2019, 11:47 AM
balazske updated this revision to Diff 202438.May 31 2019, 7:59 AM
  • Import BraceRange of EnumDecl.
  • Added lit tests.

Added lit tests for the simple (Decl) cases. The Expr and type related changes are more tricky to do. But I do not like this approach to add new tests because the test function in D60463 does almost the same thing for every import in ASTTests without adding lot of new checks manually.