Page MenuHomePhabricator

[ASTImporter] Various source location and range import fixes.
ClosedPublic

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
6129 ↗(On Diff #201521)

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

7150 ↗(On Diff #201521)

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

7225 ↗(On Diff #201521)

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
6129 ↗(On Diff #201521)

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.

7150 ↗(On Diff #201521)

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

7225 ↗(On Diff #201521)

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.

martong added a comment.EditedDec 3 2019, 10:12 AM

@shafik Ping
As I see, balazske added the desired test, Shafik could you please take a look?

balazske updated this revision to Diff 232097.Dec 4 2019, 5:37 AM

Rebased to monorepo and newer master.

shafik accepted this revision.Dec 4 2019, 5:02 PM

LGTM

This revision is now accepted and ready to land.Dec 4 2019, 5:02 PM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Dec 5 2019, 6:46 PM

This change caused the Import/namespace/struct-and-var/test.cpp test to fail on ARM due to an extra line with `-CXXRecordDecl being emitted by the compiler that was being matched instead of the intended line. I checked in a fix to tighten up the check a little more so that it gets the correct line in 757bc55. I don't think it should negatively affect the test, but please do review the change.

Sample test failure from build bot:
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/1131/steps/test-check-clang/logs/FAIL%3A%20Clang%3A%3Atest.cpp

******************** TEST 'Clang :: Import/struct-and-var/test.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   clang-import-test -dump-ast --import C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S1.cpp --import C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp -expression C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp | c:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\build\bin\filecheck.exe C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp
--
Exit Code: 1

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "clang-import-test" "-dump-ast" "--import" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S1.cpp" "--import" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp" "-expression" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp"
$ "c:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\build\bin\filecheck.exe" "C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp"
# command stderr:
C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var\test.cpp:4:16: error: CHECK-SAME: is not on the same line as the previous match

// CHECK-SAME: Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F

               ^

<stdin>:49:126: note: 'next' match was here

`-CXXRecordDecl 0x63d9992750 <C:\buildbot\as-builder-1\llvm-clang-win-x-armv7l\llvm-project\clang\test\Import\struct-and-var/Inputs/S2.cpp:1:1, line:3:1> line:1:8 struct F definition

                                                                                                                             ^

<stdin>:25:18: note: previous match ended here

| `-CXXRecordDecl 0x63d9992378 <<invalid sloc>> <invalid sloc> implicit <undeserialized declarations> struct __va_list definition

                 ^

The fix looks OK (other alternative is to remove the CHECK with CXXRecordDecl, or make a single line with regular expressions).