[Lldb-commits] [lldb] r373711 - [lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST
Raphael Isemann via lldb-commits
lldb-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:
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py
lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
Modified: lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py?rev=373711&r1=373710&r2=373711&view=diff
==============================================================================
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/TestLibCxxModernTypeLookup.py Fri Oct 4 01:26:17 2019
@@ -18,3 +18,6 @@ class LibcxxModernTypeLookup(TestBase):
# 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"])
Modified: lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp?rev=373711&r1=373710&r2=373711&view=diff
==============================================================================
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp (original)
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/modern-type-lookup/libcxx/main.cpp Fri Oct 4 01:26:17 2019
@@ -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.
}
Modified: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp?rev=373711&r1=373710&r2=373711&view=diff
==============================================================================
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp (original)
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp Fri Oct 4 01:26:17 2019
@@ -180,108 +180,20 @@ ClangExpressionDeclMap::TargetInfo Clang
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 @@ TypeFromUser ClangExpressionDeclMap::Dep
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 {
More information about the lldb-commits
mailing list