teemperor updated this revision to Diff 223007.
teemperor marked 2 inline comments as done.
teemperor added a comment.
- Addressed feedback (Thanks Gabor, Adrian & Shafik!)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68326/new/
https://reviews.llvm.org/D68326
Files:
clang/include/clang/AST/ExternalASTMerger.h
clang/lib/AST/ExternalASTMerger.cpp
lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py
lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp
lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
===================================================================
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -180,108 +180,20 @@
return ret;
}
-namespace {
-/// This class walks an AST and ensures that all DeclContexts defined inside the
-/// current source file are properly complete.
-///
-/// This is used to ensure that persistent types defined in the current source
-/// file migrate completely to the persistent AST context before they are
-/// reused. If that didn't happen, it would be impoossible to complete them
-/// because their origin would be gone.
-///
-/// The stragtegy used by this class is to check the SourceLocation (to be
-/// specific, the FileID) and see if it's the FileID for the current expression.
-/// Alternate strategies could include checking whether an ExternalASTMerger,
-/// set up to not have the current context as a source, can find an original for
-/// the type.
-class Completer : public clang::RecursiveASTVisitor<Completer> {
-private:
- /// Used to import Decl contents
- clang::ASTImporter &m_exporter;
- /// The file that's going away
- clang::FileID m_file;
- /// Visited Decls, to avoid cycles
- llvm::DenseSet<clang::Decl *> m_completed;
-
- bool ImportAndCheckCompletable(clang::Decl *decl) {
- llvm::Expected<clang::Decl *> imported_decl = m_exporter.Import(decl);
- if (!imported_decl) {
- llvm::consumeError(imported_decl.takeError());
- return false;
- }
- if (m_completed.count(decl))
- return false;
- if (!llvm::isa<DeclContext>(decl))
- return false;
- const clang::SourceLocation loc = decl->getLocation();
- if (!loc.isValid())
- return false;
- const clang::FileID file =
- m_exporter.getFromContext().getSourceManager().getFileID(loc);
- if (file != m_file)
- return false;
- // We are assuming the Decl was parsed in this very expression, so it
- // should not have external storage.
- lldbassert(!llvm::cast<DeclContext>(decl)->hasExternalLexicalStorage());
- return true;
- }
-
- void Complete(clang::Decl *decl) {
- m_completed.insert(decl);
- auto *decl_context = llvm::cast<DeclContext>(decl);
- llvm::Expected<clang::Decl *> imported_decl = m_exporter.Import(decl);
- if (!imported_decl) {
- llvm::consumeError(imported_decl.takeError());
- return;
- }
- m_exporter.CompleteDecl(decl);
- for (Decl *child : decl_context->decls())
- if (ImportAndCheckCompletable(child))
- Complete(child);
- }
-
- void MaybeComplete(clang::Decl *decl) {
- if (ImportAndCheckCompletable(decl))
- Complete(decl);
- }
-
-public:
- Completer(clang::ASTImporter &exporter, clang::FileID file)
- : m_exporter(exporter), m_file(file) {}
-
- // Implements the RecursiveASTVisitor's core API. It is called on each Decl
- // that the RecursiveASTVisitor encounters, and returns true if the traversal
- // should continue.
- bool VisitDecl(clang::Decl *decl) {
- MaybeComplete(decl);
- return true;
- }
-};
-} // namespace
-
-static void CompleteAllDeclContexts(clang::ASTImporter &exporter,
- clang::FileID file, clang::QualType root) {
- clang::QualType canonical_type = root.getCanonicalType();
- if (clang::TagDecl *tag_decl = canonical_type->getAsTagDecl()) {
- Completer(exporter, file).TraverseDecl(tag_decl);
- } else if (auto interface_type = llvm::dyn_cast<ObjCInterfaceType>(
- canonical_type.getTypePtr())) {
- Completer(exporter, file).TraverseDecl(interface_type->getDecl());
- } else {
- Completer(exporter, file).TraverseType(canonical_type);
- }
-}
-
static clang::QualType ExportAllDeclaredTypes(
- clang::ExternalASTMerger &merger, clang::ASTContext &source,
- clang::FileManager &source_file_manager,
+ clang::ExternalASTMerger &parent_merger, clang::ExternalASTMerger &merger,
+ clang::ASTContext &source, clang::FileManager &source_file_manager,
const clang::ExternalASTMerger::OriginMap &source_origin_map,
clang::FileID file, clang::QualType root) {
+ // Mark the source as temporary to make sure all declarations from the
+ // AST are exported. Also add the parent_merger as the merger into the
+ // source AST so that the merger can track back any declarations from
+ // the persistent ASTs we used as sources.
clang::ExternalASTMerger::ImporterSource importer_source(
- source, source_file_manager, source_origin_map);
+ source, source_file_manager, source_origin_map, /*Temporary*/ true,
+ &parent_merger);
merger.AddSources(importer_source);
clang::ASTImporter &exporter = merger.ImporterForOrigin(source);
- CompleteAllDeclContexts(exporter, file, root);
llvm::Expected<clang::QualType> ret_or_error = exporter.Import(root);
merger.RemoveSources(importer_source);
if (ret_or_error) {
@@ -312,8 +224,9 @@
auto scratch_ast_context = static_cast<ClangASTContextForExpressions *>(
m_target->GetScratchClangASTContext());
clang::QualType exported_type = ExportAllDeclaredTypes(
- scratch_ast_context->GetMergerUnchecked(), *source.getASTContext(),
- *source.getFileManager(), m_merger_up->GetOrigins(), source_file,
+ *m_merger_up.get(), scratch_ast_context->GetMergerUnchecked(),
+ *source.getASTContext(), *source.getFileManager(),
+ m_merger_up->GetOrigins(), source_file,
clang::QualType::getFromOpaquePtr(parser_type.GetOpaqueQualType()));
return TypeFromUser(exported_type.getAsOpaquePtr(), &target);
} else {
Index: lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp
===================================================================
--- lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp
+++ lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp
@@ -1,6 +1,14 @@
#include <utility>
+#include <string>
+#include <map>
+#include <unordered_map>
int main(int argc, char **argv) {
std::pair<int, long> pair = std::make_pair<int, float>(1, 2L);
+ std::string foo = "bar";
+ std::map<int, int> map;
+ map[1] = 2;
+ std::unordered_map<int, int> umap;
+ umap[1] = 2;
return pair.first; // Set break point at this line.
}
Index: lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py
+++ lldb/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py
@@ -18,3 +18,6 @@
# Test a few simple expressions that should still work with modern-type-lookup.
self.expect("expr pair", substrs=["(std::", "pair<int, long", "= (first = 1, second = 2)"])
+ self.expect("expr foo", substrs=["(std::", "string", "\"bar\""])
+ self.expect("expr map", substrs=["(std::", "map", "first = 1, second = 2"])
+ self.expect("expr umap", substrs=["(std::", "unordered_map", "first = 1, second = 2"])
Index: clang/lib/AST/ExternalASTMerger.cpp
===================================================================
--- clang/lib/AST/ExternalASTMerger.cpp
+++ clang/lib/AST/ExternalASTMerger.cpp
@@ -101,24 +101,103 @@
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 @@
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));
}
Index: clang/include/clang/AST/ExternalASTMerger.h
===================================================================
--- clang/include/clang/AST/ExternalASTMerger.h
+++ clang/include/clang/AST/ExternalASTMerger.h
@@ -84,13 +84,22 @@
ASTContext &AST;
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 @@
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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
