Page MenuHomePhabricator

[ASTImporter] import macro source locations
ClosedPublic

Authored by r.stahl on Jun 4 2018, 1:51 AM.

Diff Detail

Repository
rC Clang

Event Timeline

r.stahl created this revision.Jun 4 2018, 1:51 AM

The split-token case should be covered by this, because it takes the correct TokenLen and the isTokenRange flag. Other than that the split-token ExpansionInfo is indistinguishable.

What about loaded source locations? Do they need to be handled specifically or does this cover everything now?

r.stahl updated this revision to Diff 151138.Jun 13 2018, 6:02 AM

remove stray include

martong accepted this revision.Jun 19 2018, 8:04 AM

This patch is really useful and LGTM!
Just found some minor things.

lib/AST/ASTImporter.cpp
7193

Let's say we import a SourceLocation with ASTImporter::Import(SourceLocation FromLoc).
That calls into ASTImporter::Import(FileID FromID) where we again import other source locations.
Could the initial FromLoc be equal with any of these locations (FromEx.getSpellingLoc(), FromEx.getExpansionLocStart()) ?
My understanding is that this is not possible because we cannot have recursive macros, but please confirm.

unittests/AST/ASTImporterTest.cpp
1580

Perhaps it would be more concise and less error prone to use a FullSourceLoc which wraps one simple SourceLocation and a SourceManager.

1616
CompareSourceRanges(FullSourceRange{ToD->getSourceRange(), ToSM}, ..

?

This revision is now accepted and ready to land.Jun 19 2018, 8:04 AM
r.stahl added inline comments.Jun 20 2018, 4:24 AM
lib/AST/ASTImporter.cpp
7193

Yes, that was my understanding as well. If some compiler error is a macro expansion it eventually stops at some point while walking this chain.

unittests/AST/ASTImporterTest.cpp
1580

Will do, thanks!

r.stahl updated this revision to Diff 152436.Jun 22 2018, 1:43 AM
r.stahl marked 5 inline comments as done.

addressed review comment, but there is nothing like FullSourceRange to improve everything

martong added subscribers: balazske, xazax.hun.

Adding @balazske and @xazax.hun as reviewers. I think if it gets one more approve then we could merge. I'd like to speed up the things here ... we can't expect Aleksei to review all those many patches we sent lately.

balazske added inline comments.Jun 22 2018, 6:57 AM
unittests/AST/ASTImporterTest.cpp
1593

This test causes every case for expansion (macro, macro arg) to be executed at import?

This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?

This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?

I don't know for sure, but it should be used - yes. I have not measured a possible performance impact. Do you have a suggestion how I could do this on a Linux setup?

Note that I did not implement this as nice-to-have feature, but for fixing a concrete issue: https://reviews.llvm.org/D26054#1085413

unittests/AST/ASTImporterTest.cpp
1593

Yes, the last DeclRef will be "arg" on the RHS which is a macro argument expansion and the last IntegerLiteral will be the "1" which is a non-argument macro expansion.

martong added a comment.EditedJun 22 2018, 7:56 AM

This code is live when reading pchs, correct? Does this have any measurable perf impact on deserializing pchs for, say, Cocoa.h or Windows.h?

No. The deserialization of a PCH is handled in a completely independent source code (e.g. ASTUnit::LoadFromASTFile). ASTImporter is responsible to merge two ASTs. These ASTs may come from a PCH - deserialized - , or they may be provided by the parser; from the ASTImporer's point of view this is irrelevant. Thus, there is no performance impact on deserialization of PCHs.

ASTImporter is used in LLDB with the command exec when you want to evaluate an expression and during cross translation unit static analysis.

Hi Rafael,

I apologize for the delay in review and hope to get ASTImporter patches reviewed on this weekend.

lib/AST/ASTImporter.cpp
7188–7189

'for to' => 'for the "from" source manager to'?

Hi Rafael,

I think the patch is great. But, honestly, I have never dealt with SourceLocation machinery closely, so some things are a bit unclear to me.

lib/AST/ASTImporter.cpp
7193

If we have a macro referring another macro in the same file, will we import their FileID twice? First, during Import(getSpellingLoc()) and then in this caller?

7195

ToExLocE seems to be unused in the true branch below. Could we move it into 'else' branch?

unittests/AST/ASTImporterTest.cpp
1596

Could you please add a test with nested macros? I.e.

#define FUNC_INT void declToImport
#define FUNC FUNC_INT
FUNC(int a);
r.stahl updated this revision to Diff 152633.Jun 25 2018, 1:18 AM
r.stahl marked 5 inline comments as done.

improved code quality; added nested macro test. it "works", but is disabled because it revealed another bug: the function end location is not imported. will send a patch

r.stahl marked an inline comment as done.Jun 25 2018, 1:20 AM
r.stahl added inline comments.
lib/AST/ASTImporter.cpp
7193

That is taken care of by the ImportedFileIDs check.

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