Author: Raphael Isemann Date: 2020-06-30T12:46:28+02:00 New Revision: 502773d743417a7896e184b4000bcf20fddb7ad9
URL: https://github.com/llvm/llvm-project/commit/502773d743417a7896e184b4000bcf20fddb7ad9 DIFF: https://github.com/llvm/llvm-project/commit/502773d743417a7896e184b4000bcf20fddb7ad9.diff LOG: [lldb][NFC] Remove ImportInProgress lock in ClangASTSource Summary: The ClangASTSource has a lock that globally disables all lookups into the external AST source when we explicitly "guarded" copy a type. It's not used for anything else, so importing declarations or importing types that are dependencies of a declaration actually won't activate that lock. The lookups it is supposed to prevent also don't actually happen in our test suite. The check in `ClangExpressionDeclMap::FindExternalVisibleDecls` is never executed and the check in the `ClangASTSource::FindExternalVisibleDeclsByName` is only ever reached by the `Import-std-module` tests (which explicitly do a lookup into the expression context on purpose). This lock was added in 6abfabff6158076eccdf6fcac5a12894039de2c9 as a replacement for a list of types we already looked up which appeared to be an optimisation strategy. I assume back then this lock had a purpose but these days the ASTImporter and LLDB seem to be smart enough to avoid whatever lookups this tried to prevent. I would say we remove it from LLDB. The main reason is that it blocks D81561 (which explicitly does a specific lookup to resolve placeholder types produced by `-flimit-debug-info`) but it's semantics are also very confusing. The naming implies it's a flag to indicate when we import something at the moment which is practically never true as described above. Also the fact that it makes our ExternalASTSource alternate between doing lookups into the debug info and pretending it doesn't know any external decls could really break our lookup in some weird way if Clang decides to cache a fake empty lookup result that was generated while the lock was active. Reviewers: labath, shafik, JDevlieghere, aprantl Reviewed By: labath, JDevlieghere, aprantl Subscribers: aprantl, abidh Differential Revision: https://reviews.llvm.org/D81749 Added: Modified: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp Removed: ################################################################################ diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp index 4a81b1ae6a3b..6fe85a1298fc 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp @@ -53,9 +53,9 @@ class ScopedLexicalDeclEraser { ClangASTSource::ClangASTSource( const lldb::TargetSP &target, const std::shared_ptr<ClangASTImporter> &importer) - : m_import_in_progress(false), m_lookups_enabled(false), m_target(target), - m_ast_context(nullptr), m_ast_importer_sp(importer), - m_active_lexical_decls(), m_active_lookups() { + : m_lookups_enabled(false), m_target(target), m_ast_context(nullptr), + m_ast_importer_sp(importer), m_active_lexical_decls(), + m_active_lookups() { assert(m_ast_importer_sp && "No ClangASTImporter passed to ClangASTSource?"); } @@ -103,11 +103,6 @@ bool ClangASTSource::FindExternalVisibleDeclsByName( return false; } - if (GetImportInProgress()) { - SetNoExternalVisibleDeclsForName(decl_ctx, clang_decl_name); - return false; - } - std::string decl_name(clang_decl_name.getAsString()); switch (clang_decl_name.getNameKind()) { @@ -1744,13 +1739,9 @@ CompilerType ClangASTSource::GuardedCopyType(const CompilerType &src_type) { if (src_ast == nullptr) return CompilerType(); - SetImportInProgress(true); - QualType copied_qual_type = ClangUtil::GetQualType( m_ast_importer_sp->CopyType(*m_clang_ast_context, src_type)); - SetImportInProgress(false); - if (copied_qual_type.getAsOpaquePtr() && copied_qual_type->getCanonicalTypeInternal().isNull()) // this shouldn't happen, but we're hardening because the AST importer diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h index 847a99abfff0..14761fbeb26b 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h @@ -197,11 +197,6 @@ class ClangASTSource : public clang::ExternalASTSource, clang::Sema *getSema(); - void SetImportInProgress(bool import_in_progress) { - m_import_in_progress = import_in_progress; - } - bool GetImportInProgress() { return m_import_in_progress; } - void SetLookupsEnabled(bool lookups_enabled) { m_lookups_enabled = lookups_enabled; } @@ -376,7 +371,6 @@ class ClangASTSource : public clang::ExternalASTSource, friend struct NameSearchContext; - bool m_import_in_progress; bool m_lookups_enabled; /// The target to use in finding variables and types. diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp index d71036f793b1..8c49898e1d6c 100644 --- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp +++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp @@ -632,11 +632,6 @@ void ClangExpressionDeclMap::FindExternalVisibleDecls( Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS)); - if (GetImportInProgress()) { - LLDB_LOGV(log, "Ignoring a query during an import"); - return; - } - if (log) { if (!context.m_decl_context) LLDB_LOG(log, _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits