[lldb][NFC] Remove ImportInProgress lock in ClangASTSource

[teemperor]

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.

[teemperor]

• Also removed the now unused member variable.

[JDevlieghere]

The variable wasn't even atomic? Good riddance.

LGTM

[teemperor]

In D81749#2097352, @JDevlieghere wrote:

The variable wasn't even atomic? Good riddance.

Actually the whole thing is supposed to be used in a single thread, so that's fine. Just called it a 'lock' as it's like a lock we have active around this one method call (the calls it can block can only come from the same thread as the whole expression parser code doesn't support threads)

[labath]

I don't exactly know what is this protecting against, but removing it definitely makes my life easier.

[labath]

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

55–56

You missed this occurence.

[teemperor]

• Don't initialise the now unused variable.

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.h

public:
   /// The worker function for FindExternalVisibleDeclsByName.
   ///
   /// \param[in] context
   ///     The NameSearchContext to use when filing results.
   virtual void FindExternalVisibleDecls(NameSearchContext &context);
 
   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;
   }
   bool GetLookupsEnabled() { return m_lookups_enabled; }
 
   /// \class ClangASTSourceProxy ClangASTSource.h
   /// "lldb/Expression/ClangASTSource.h" Proxy for ClangASTSource
   ///
protected:
   /// \param namespace_decl The DeclContext in which to search for namespaces.
   void FillNamespaceMap(NameSearchContext &context, lldb::ModuleSP module_sp,
                         const CompilerDeclContext &namespace_decl);
 
   clang::TagDecl *FindCompleteType(const clang::TagDecl *decl);
 
   friend struct NameSearchContext;
 
-  bool m_import_in_progress;
   bool m_lookups_enabled;
 
   /// The target to use in finding variables and types.
   const lldb::TargetSP m_target;
   /// The AST context requests are coming in for.
   clang::ASTContext *m_ast_context;
   /// The TypeSystemClang for m_ast_context.
   TypeSystemClang *m_clang_ast_context;

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

 private:
   std::set<const clang::Decl *> &m_active_lexical_decls;
   const clang::Decl *m_decl;
 };
 }
 
 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?");
 }
 
 void ClangASTSource::InstallASTContext(TypeSystemClang &clang_ast_context) {
   m_ast_context = &clang_ast_context.getASTContext();
   m_clang_ast_context = &clang_ast_context;
   m_file_manager = &m_ast_context->getSourceManager().getFileManager();
   m_ast_importer_sp->InstallMapCompleter(m_ast_context, *this);

 // The core lookup interface.
 bool ClangASTSource::FindExternalVisibleDeclsByName(
     const DeclContext *decl_ctx, DeclarationName clang_decl_name) {
   if (!m_ast_context) {
     SetNoExternalVisibleDeclsForName(decl_ctx, clang_decl_name);
     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()) {
   // Normal identifiers.
   case DeclarationName::Identifier: {
     clang::IdentifierInfo *identifier_info =
         clang_decl_name.getAsIdentifierInfo();
 

 }
 
 CompilerType ClangASTSource::GuardedCopyType(const CompilerType &src_type) {
   TypeSystemClang *src_ast =
       llvm::dyn_cast_or_null<TypeSystemClang>(src_type.GetTypeSystem());
   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
     // seems to be generating bad types on occasion.
     return CompilerType();
 
   return m_clang_ast_context->GetType(copied_qual_type);
 }

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp

 void ClangExpressionDeclMap::FindExternalVisibleDecls(
     NameSearchContext &context) {
   assert(m_ast_context);
 
   const ConstString name(context.m_decl_name.getAsString().c_str());
 
   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,
                "ClangExpressionDeclMap::FindExternalVisibleDecls for "
                "'{0}' in a NULL DeclContext",
                name);
     else if (const NamedDecl *context_named_decl =
                  dyn_cast<NamedDecl>(context.m_decl_context))