This is an archive of the discontinued LLVM Phabricator instance.

[CrossTU] Add a function to retrieve original source location.
ClosedPublic

Authored by balazske on Jul 11 2019, 3:46 AM.

Details

Summary

A new function will be added to get the original SourceLocation
for a SourceLocation that was imported as result of getCrossTUDefinition.
The returned SourceLocation is in the context of the (original)
SourceManager for the original source file. Additionally the
ASTUnit object for that source file is returned. This is needed
to get a SourceManager to operate on with the returned source location.

The new function works if multiple different source files are loaded
with the same CrossTU context.

This patch can be treated as part of a bigger change that is needed to
improve macro expansion handliong at plist generation.

Diff Detail

Repository
rL LLVM

Event Timeline

balazske created this revision.Jul 11 2019, 3:46 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
martong added a comment.EditedJul 12 2019, 2:15 AM

This patch can be treated as part of a bigger change that is needed to improve macro expansion handliong at plist generation.

@balazske Could you please prepare the upcoming patch and put that on the Stack? I think that may ease the work of the reviewers if they could see how you want to use this function.

martong added inline comments.Jul 16 2019, 2:51 AM
include/clang/AST/ASTImporter.h
234 ↗(On Diff #209160)

Unit->getASTContext must be equal to FromContext, right?
Then FromUnit would be a better naming.
The importer does handle two ASTContexts, so it would be great to know which ASTUnit's ASTContext is which. Or vica-versa, we must know precisely to which context does this unit belong. Also a comment would be useful that we must store the unit because there is no way to get that from the context.

martong added inline comments.Jul 16 2019, 3:20 AM
include/clang/AST/ASTImporter.h
317 ↗(On Diff #209160)

What if we provided an additional constructor where we take over the ASTUnits instead of the ASTContexts?
Then we would not need to pass the FileManagers neither.

ASTImporter(ASTUnit &ToUnit, 
            ASTUnit &FromUnit,
            bool MinimalImport,
            std::shared_ptr<ASTImporterSharedState> SharedState = nullptr,
balazske marked an inline comment as done.Jul 16 2019, 3:52 AM
balazske added inline comments.
include/clang/AST/ASTImporter.h
317 ↗(On Diff #209160)

Is the SharedState==nullptr case only for LLDB? If yes then it is possible to have a "LLDB" constructor when no shared state and no ASTUnit is needed. And another for CTU case when a From and To ASTUnit is specified and a shared state (theoretically minimal can be true in any case but probably only true for LLDB?).

balazske marked an inline comment as not done.Jul 16 2019, 5:40 AM
balazske added inline comments.
include/clang/AST/ASTImporter.h
317 ↗(On Diff #209160)

It is not trivial to make and use this new kind of constructor (with ToUnit), there is no ToUnit in the CrossTU context. Is it OK to have the original constructor, or one with FromUnit (but ToContext and ToFileManager)?

martong added inline comments.Jul 16 2019, 6:35 AM
include/clang/AST/ASTImporter.h
317 ↗(On Diff #209160)

Is the SharedState==nullptr case only for LLDB?

Yes.

Is it OK to have the original constructor, or one with FromUnit (but ToContext and ToFileManager)?

I think it is OK to have a new ctor with FromUnit (but ToContext and ToFileManager). Because ASTUnit is just a utility class for loading an ASTContext from an AST file. So, one of the "to" or the "from" context could be initialized by an ASTUnit.

In the future the API of the ASTImporter could be extended with such a ctor which takes two ASTUnits, also we could add the counterpart ctor where we have ToUnit, FromContext and FromFileManager as params.

martong added inline comments.Jul 16 2019, 6:36 AM
include/clang/AST/ASTImporter.h
317 ↗(On Diff #209160)

Note that if we have a ctor which takes FromUnit as a param then we won't need the assertions which check whether the Unit provides the same context or not.

balazske updated this revision to Diff 210490.Jul 18 2019, 12:07 AM
balazske marked an inline comment as not done.
  • Renamed 'Unit', changed constructors.
martong added inline comments.Jul 18 2019, 1:21 AM
include/clang/AST/ASTImporter.h
288 ↗(On Diff #210490)

Why do we still need FromUnit in this ctor? I thought the other ctor would handle that case.

balazske marked 4 inline comments as done.Jul 18 2019, 2:28 AM
balazske added inline comments.
include/clang/AST/ASTImporter.h
288 ↗(On Diff #210490)

This is a private and common constructor that can handle both cases (called by the others).

martong accepted this revision.Jul 18 2019, 6:37 AM

LGTM.

include/clang/AST/ASTImporter.h
288 ↗(On Diff #210490)

Ok, I've missed that.

This revision is now accepted and ready to land.Jul 18 2019, 6:37 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2019, 8:25 AM

Commit was reverted because we were not aware of that clangAST library can not use clangFrontend library. So a different solution is needed, probably pass a SourceLocation import callback to ASTImporter. (A Preprocessor that can be used in place of ASTUnit here can not be get from a SourceManager or ASTContext.) And then no ImportedFileIDs is needed in ASTImporterSharedState, instead in CrossTranslationUnitContext.