r373711 - [lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST
Raphael Isemann via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 4 01:26:17 PDT 2019
Author: teemperor
Date: Fri Oct 4 01:26:17 2019
New Revision: 373711
URL: http://llvm.org/viewvc/llvm-project?rev=373711&view=rev
Log:
[lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST
Summary:
As we figured out in D67803, importing declarations from a temporary ASTContext that were originally from a persistent ASTContext
causes a bunch of duplicated declarations where we end up having declarations in the target AST that have no associated ASTImporter that
can complete them.
I haven't figured out how/if we can solve this in the current way we do things in LLDB, but in the modern-type-lookup this is solvable
as we have a saner architecture with the ExternalASTMerger. As we can (hopefully) make modern-type-lookup the default mode in the future,
I would say we try fixing this issue here. As we don't use the hack that was reinstated in D67803 during modern-type-lookup, the test case for this
is essentially just printing any kind of container in `std::` as we would otherwise run into the issue that required a hack like D67803.
What this patch is doing in essence is that instead of importing a declaration from a temporary ASTContext, we instead check if the
declaration originally came from a persistent ASTContext (e.g. the debug information) and we directly import from there. The ExternalASTMerger
is already connected with ASTImporters to these different sources, so this patch is essentially just two parts:
1. Mark our temporary ASTContext/ImporterSource as temporary when we import from the expression AST.
2. If the ExternalASTMerger sees we import from the expression AST, instead of trying to import these temporary declarations, check if we
can instead import from the persistent ASTContext that is already connected. This ensures that all records from the persistent source actually
come from the persistent source and are minimally imported in a way that allows them to be completed later on in the target AST.
The next step is to run the ASTImporter for these temporary expressions with the MinimalImport mode disabled, but that's a follow up patch.
This patch fixes most test failures with modern-type-lookup enabled by default (down to 73 failing tests, which includes the 22 import-std-module tests
which need special treatment).
Reviewers: shafik, martong
Reviewed By: martong
Subscribers: aprantl, rnkovacs, christof, abidh, JDevlieghere, lldb-commits
Tags: #lldb
Differential Revision: https://reviews.llvm.org/D68326
Modified:
cfe/trunk/include/clang/AST/ExternalASTMerger.h
cfe/trunk/lib/AST/ExternalASTMerger.cpp
Modified: cfe/trunk/include/clang/AST/ExternalASTMerger.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExternalASTMerger.h?rev=373711&r1=373710&r2=373711&view=diff
==============================================================================
--- cfe/trunk/include/clang/AST/ExternalASTMerger.h (original)
+++ cfe/trunk/include/clang/AST/ExternalASTMerger.h Fri Oct 4 01:26:17 2019
@@ -84,13 +84,22 @@ public:
ASTContext *
FileManager &FM;
const OriginMap &OM;
+ /// True iff the source only exists temporary, i.e., it will be removed from
+ /// the ExternalASTMerger during the life time of the ExternalASTMerger.
+ bool Temporary;
+ /// If the ASTContext of this source has an ExternalASTMerger that imports
+ /// into this source, then this will point to that other ExternalASTMerger.
+ ExternalASTMerger *Merger;
public:
- ImporterSource(ASTContext &_AST, FileManager &_FM, const OriginMap &_OM)
- : AST(_AST), FM(_FM), OM(_OM) {}
+ ImporterSource(ASTContext &AST, FileManager &FM, const OriginMap &OM,
+ bool Temporary = false, ExternalASTMerger *Merger = nullptr)
+ : AST(AST), FM(FM), OM(OM), Temporary(Temporary), Merger(Merger) {}
ASTContext &getASTContext() const { return AST; }
FileManager &getFileManager() const { return FM; }
const OriginMap &getOriginMap() const { return OM; }
+ bool isTemporary() const { return Temporary; }
+ ExternalASTMerger *getMerger() const { return Merger; }
};
private:
@@ -106,6 +115,12 @@ public:
ExternalASTMerger(const ImporterTarget &Target,
llvm::ArrayRef<ImporterSource> Sources);
+ /// Asks all connected ASTImporters if any of them imported the given
+ /// declaration. If any ASTImporter did import the given declaration,
+ /// then this function returns the declaration that D was imported from.
+ /// Returns nullptr if no ASTImporter did import import D.
+ Decl *FindOriginalDecl(Decl *D);
+
/// Add a set of ASTContexts as possible origins.
///
/// Usually the set will be initialized in the constructor, but long-lived
Modified: cfe/trunk/lib/AST/ExternalASTMerger.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExternalASTMerger.cpp?rev=373711&r1=373710&r2=373711&view=diff
==============================================================================
--- cfe/trunk/lib/AST/ExternalASTMerger.cpp (original)
+++ cfe/trunk/lib/AST/ExternalASTMerger.cpp Fri Oct 4 01:26:17 2019
@@ -101,24 +101,103 @@ private:
ExternalASTMerger &Parent;
ASTImporter Reverse;
const ExternalASTMerger::OriginMap &FromOrigins;
-
+ /// @see ExternalASTMerger::ImporterSource::Temporary
+ bool TemporarySource;
+ /// Map of imported declarations back to the declarations they originated
+ /// from.
+ llvm::DenseMap<Decl *, Decl *> ToOrigin;
+ /// @see ExternalASTMerger::ImporterSource::Merger
+ ExternalASTMerger *SourceMerger;
llvm::raw_ostream &logs() { return Parent.logs(); }
public:
LazyASTImporter(ExternalASTMerger &_Parent, ASTContext &ToContext,
FileManager &ToFileManager,
- const ExternalASTMerger::ImporterSource &_Source,
+ const ExternalASTMerger::ImporterSource &S,
std::shared_ptr<ASTImporterSharedState> SharedState)
- : ASTImporter(ToContext, ToFileManager, _Source.getASTContext(),
- _Source.getFileManager(),
+ : ASTImporter(ToContext, ToFileManager, S.getASTContext(),
+ S.getFileManager(),
/*MinimalImport=*/true, SharedState),
Parent(_Parent),
- Reverse(_Source.getASTContext(), _Source.getFileManager(), ToContext,
- ToFileManager, /*MinimalImport=*/true),
- FromOrigins(_Source.getOriginMap()) {}
+ Reverse(S.getASTContext(), S.getFileManager(), ToContext, ToFileManager,
+ /*MinimalImport=*/true),
+ FromOrigins(S.getOriginMap()), TemporarySource(S.isTemporary()),
+ SourceMerger(S.getMerger()) {}
+
+ llvm::Expected<Decl *> ImportImpl(Decl *FromD) override {
+ if (!TemporarySource || !SourceMerger)
+ return ASTImporter::ImportImpl(FromD);
+
+ // If we get here, then this source is importing from a temporary ASTContext
+ // that also has another ExternalASTMerger attached. It could be
+ // possible that the current ExternalASTMerger and the temporary ASTContext
+ // share a common ImporterSource, which means that the temporary
+ // AST could contain declarations that were imported from a source
+ // that this ExternalASTMerger can access directly. Instead of importing
+ // such declarations from the temporary ASTContext, they should instead
+ // be directly imported by this ExternalASTMerger from the original
+ // source. This way the ExternalASTMerger can safely do a minimal import
+ // without creating incomplete declarations originated from a temporary
+ // ASTContext. If we would try to complete such declarations later on, we
+ // would fail to do so as their temporary AST could be deleted (which means
+ // that the missing parts of the minimally imported declaration in that
+ // ASTContext were also deleted).
+ //
+ // The following code tracks back any declaration that needs to be
+ // imported from the temporary ASTContext to a persistent ASTContext.
+ // Then the ExternalASTMerger tries to import from the persistent
+ // ASTContext directly by using the associated ASTImporter. If that
+ // succeeds, this ASTImporter just maps the declarations imported by
+ // the other (persistent) ASTImporter to this (temporary) ASTImporter.
+ // The steps can be visualized like this:
+ //
+ // Target AST <--- 3. Indirect import --- Persistent AST
+ // ^ of persistent decl ^
+ // | |
+ // 1. Current import 2. Tracking back to persistent decl
+ // 4. Map persistent decl |
+ // & pretend we imported. |
+ // | |
+ // Temporary AST -------------------------------'
+
+ // First, ask the ExternalASTMerger of the source where the temporary
+ // declaration originated from.
+ Decl *Persistent = SourceMerger->FindOriginalDecl(FromD);
+ // FromD isn't from a persistent AST, so just do a normal import.
+ if (!Persistent)
+ return ASTImporter::ImportImpl(FromD);
+ // Now ask the current ExternalASTMerger to try import the persistent
+ // declaration into the target.
+ ASTContext &PersistentCtx = Persistent->getASTContext();
+ ASTImporter &OtherImporter = Parent.ImporterForOrigin(PersistentCtx);
+ // Check that we never end up in the current Importer again.
+ assert((&PersistentCtx != &getFromContext()) && (&OtherImporter != this) &&
+ "Delegated to same Importer?");
+ auto DeclOrErr = OtherImporter.Import(Persistent);
+ // Errors when importing the persistent decl are treated as if we
+ // had errors with importing the temporary decl.
+ if (!DeclOrErr)
+ return DeclOrErr.takeError();
+ Decl *D = *DeclOrErr;
+ // Tell the current ASTImporter that this has already been imported
+ // to prevent any further queries for the temporary decl.
+ MapImported(FromD, D);
+ return D;
+ }
+
+ /// Implements the ASTImporter interface for tracking back a declaration
+ /// to its original declaration it came from.
+ Decl *GetOriginalDecl(Decl *To) override {
+ auto It = ToOrigin.find(To);
+ if (It != ToOrigin.end())
+ return It->second;
+ return nullptr;
+ }
/// Whenever a DeclContext is imported, ensure that ExternalASTSource's origin
/// map is kept up to date. Also set the appropriate flags.
void Imported(Decl *From, Decl *To) override {
+ ToOrigin[To] = From;
+
if (auto *ToDC = dyn_cast<DeclContext>(To)) {
const bool LoggingEnabled = Parent.LoggingEnabled();
if (LoggingEnabled)
@@ -322,9 +401,19 @@ ExternalASTMerger::ExternalASTMerger(con
AddSources(Sources);
}
+Decl *ExternalASTMerger::FindOriginalDecl(Decl *D) {
+ assert(&D->getASTContext() == &Target.AST);
+ for (const auto &I : Importers)
+ if (auto Result = I->GetOriginalDecl(D))
+ return Result;
+ return nullptr;
+}
+
void ExternalASTMerger::AddSources(llvm::ArrayRef<ImporterSource> Sources) {
for (const ImporterSource &S : Sources) {
assert(&S.getASTContext() != &Target.AST);
+ // Check that the associated merger actually imports into the source AST.
+ assert(!S.getMerger() || &S.getMerger()->Target.AST == &S.getASTContext());
Importers.push_back(std::make_unique<LazyASTImporter>(
*this, Target.AST, Target.FM, S, SharedState));
}
More information about the cfe-commits
mailing list