Merge pull request llvm#9607 from adrian-prantl/remove-per-module-fallback

[lldb] Retire the per-module Swift scratch context fallback

Author: adrian-prantl

lldb/include/lldb/Target/Target.h

@@ -1591,14 +1591,6 @@ class Target : public std::enable_shared_from_this<Target>,
 
   void SetREPL(lldb::LanguageType language, lldb::REPLSP repl_sp);
 
-  /// Enable the use of a separate sscratch type system per lldb::Module.
-  void SetUseScratchTypesystemPerModule(bool value) {
-    m_use_scratch_typesystem_per_module = value;
-  }
-  bool UseScratchTypesystemPerModule() const {
-    return m_use_scratch_typesystem_per_module;
-  }
-
 public:
   StackFrameRecognizerManager &GetFrameRecognizerManager() {
     return *m_frame_recognizer_manager_up;

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.cpp

@@ -1084,7 +1084,6 @@ struct ModuleImportError : public llvm::ErrorInfo<ModuleImportError> {
   ModuleImportError(llvm::Twine message, bool is_new_dylib = false)
       : msg(message.str()), is_new_dylib(is_new_dylib) {}
   void log(llvm::raw_ostream &OS) const override {
-    OS << "error while processing module import: ";
     OS << msg;
   }
   std::error_code convertToErrorCode() const override {
@@ -1453,7 +1452,8 @@ SwiftExpressionParser::ParseAndImport(
         // fatal error state. One way this can happen is if the import
         // triggered a dylib import, in which case the context is
         // purposefully poisoned.
-        msg = "import may have triggered a dylib import";
+        msg = "import may have triggered a dylib import, "
+              "resetting compiler state";
       }
       return make_error<ModuleImportError>(msg, /*is_new_dylib=*/true);
     }
@@ -1722,14 +1722,8 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
           // There are no fallback contexts in REPL and playgrounds.
           if (repl || playground)
             return;
-          if (!m_sc.target_sp->UseScratchTypesystemPerModule()) {
-            // This, together with the fatal error forces
-            // a per-module scratch to be instantiated on
-            // retry.
-            m_sc.target_sp->SetUseScratchTypesystemPerModule(true);
-            m_swift_ast_ctx.RaiseFatalError(MIE.message());
-            retry = true;
-          }
+          // The fatal error causes a new compiler to be instantiated on retry.
+          m_swift_ast_ctx.RaiseFatalError(MIE.message());
         },
         [&](const SwiftASTContextError &SACE) {
           DiagnoseSwiftASTContextError();
@@ -1745,9 +1739,7 @@ SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
         });
 
     // Signal that we want to retry the expression exactly once with a
-    // fresh SwiftASTContext initialized with the flags from the
-    // current lldb::Module / Swift dylib to avoid header search
-    // mismatches.
+    // fresh SwiftASTContext.
     if (retry)
       return ParseResult::retry_fresh_context;
 

lldb/source/Plugins/ExpressionParser/Swift/SwiftExpressionParser.h

@@ -50,7 +50,7 @@ class SwiftExpressionParser : public ExpressionParser {
 public:
   enum class ParseResult {
     success,
-    retry_fresh_context, 
+    retry_fresh_context,
     retry_no_bind_generic_params,
     unrecoverable_error
   };

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp

@@ -2347,8 +2347,7 @@ ProcessModule(Module &module, std::string m_description,
 
 lldb::TypeSystemSP
 SwiftASTContext::CreateInstance(lldb::LanguageType language, Module &module,
-                                TypeSystemSwiftTypeRef &typeref_typesystem,
-                                bool fallback) {
+                                TypeSystemSwiftTypeRef &typeref_typesystem) {
   TargetSP target = typeref_typesystem.GetTargetWP().lock();
   if (!SwiftASTContextSupportsLanguage(language))
     return lldb::TypeSystemSP();
@@ -2361,10 +2360,7 @@ SwiftASTContext::CreateInstance(lldb::LanguageType language, Module &module,
   {
     llvm::raw_string_ostream ss(m_description);
     ss << "SwiftASTContext";
-    if (fallback)
-      ss << "ForExpressions";
-    else
-      ss << "ForModule";
+    ss << "ForModule";
     ss << '(' << '"';
     module.GetDescription(ss, eDescriptionLevelBrief);
     ss << '"' << ')';
@@ -2424,20 +2420,15 @@ SwiftASTContext::CreateInstance(lldb::LanguageType language, Module &module,
   }
 
   // If there is a target this may be a fallback scratch context.
-  assert((!fallback || target) && "fallback context must specify a target");
-  std::shared_ptr<SwiftASTContext> swift_ast_sp(
-      fallback
-          ? static_cast<SwiftASTContext *>(new SwiftASTContextForExpressions(
-                m_description, typeref_typesystem))
-          : static_cast<SwiftASTContext *>(new SwiftASTContextForModule(
-                m_description, typeref_typesystem)));
+  std::shared_ptr<SwiftASTContext> swift_ast_sp(static_cast<SwiftASTContext *>(
+      new SwiftASTContextForModule(m_description, typeref_typesystem)));
   bool suppress_config_log = false;
   auto defer_log =
-      llvm::make_scope_exit([swift_ast_sp, &suppress_config_log, fallback] {
+      llvm::make_scope_exit([swift_ast_sp, &suppress_config_log] {
         // To avoid spamming the log with useless info, we don't log the
         // configuration if everything went fine and the current module
         // doesn't have any Swift contents (i.e., the shared cache dylibs).
-        if (!suppress_config_log || fallback)
+        if (!suppress_config_log)
           swift_ast_sp->LogConfiguration();
       });
 

lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.h

@@ -198,12 +198,10 @@ class SwiftASTContext : public TypeSystemSwift {
   /// Create a SwiftASTContext from a Module.  This context is used
   /// for frame variable and uses ClangImporter options specific to
   /// this lldb::Module.  The optional target is necessary when
-  /// creating a module-specific scratch context.  If \p fallback is
-  /// true, then a SwiftASTContextForExpressions is created.
+  /// creating a module-specific scratch context.
   static lldb::TypeSystemSP
   CreateInstance(lldb::LanguageType language, Module &module,
-                 TypeSystemSwiftTypeRef &typeref_typesystem,
-                 bool fallback = false);
+                 TypeSystemSwiftTypeRef &typeref_typesystem);
   /// Create a SwiftASTContextForExpressions taylored to a specific symbol
   /// context.
   static lldb::TypeSystemSP

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.cpp

@@ -1659,21 +1659,6 @@ TypeSystemSwiftTypeRef::TypeSystemSwiftTypeRef(Module &module) {
             m_description.c_str());
 }
 
-TypeSystemSwiftTypeRefForExpressions::TypeSystemSwiftTypeRefForExpressions(
-    lldb::LanguageType language, Target &target, Module &module)
-    : TypeSystemSwiftTypeRef(module), m_target_wp(target.shared_from_this()),
-      m_persistent_state_up(new SwiftPersistentExpressionState) {
-  m_description = "TypeSystemSwiftTypeRefForExpressions(PerModuleFallback)";
-  LLDB_LOGF(GetLog(LLDBLog::Types),
-            "%s::TypeSystemSwiftTypeRefForExpressions()",
-            m_description.c_str());
-  m_swift_ast_context_map.insert(
-      {nullptr,
-       SwiftASTContext::CreateInstance(
-           LanguageType::eLanguageTypeSwift, module,
-           *const_cast<TypeSystemSwiftTypeRefForExpressions *>(this), true)});
-}
-
 TypeSystemSwiftTypeRefForExpressions::TypeSystemSwiftTypeRefForExpressions(
     lldb::LanguageType language, Target &target, const char *extra_options)
     : m_target_wp(target.shared_from_this()),

lldb/source/Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.h

@@ -520,10 +520,6 @@ class TypeSystemSwiftTypeRefForExpressions : public TypeSystemSwiftTypeRef {
                                        Target &target,
                                        const char *extra_options);
 
-  /// For per-module fallback contexts.
-  TypeSystemSwiftTypeRefForExpressions(lldb::LanguageType language,
-                                       Target &target, Module &module);
-
   SwiftASTContext *GetSwiftASTContext(const SymbolContext &sc) const override;
   SwiftASTContext *
   GetSwiftASTContextOrNull(const SymbolContext &sc) const override;

lldb/source/Target/Target.cpp

@@ -2932,96 +2932,7 @@ Target::GetSwiftScratchContext(Status &error, ExecutionContextScope &exe_scope,
     return nullptr;
   };
 
-  auto maybe_create_fallback_context = [&]() {
-    ModuleLanguage key = {lldb_module, lldb::eLanguageTypeSwift};
-    if (auto *cached_ts = get_cached_module_ts(lldb_module)) {
-      auto *cached_ast_ctx =
-          llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
-              cached_ts->GetSwiftASTContext(sc));
-      if (cached_ast_ctx && cached_ast_ctx->HasFatalErrors() &&
-          !m_cant_make_scratch_type_system.count(lldb::eLanguageTypeSwift)) {
-        DisplayFallbackSwiftContextErrors(cached_ast_ctx);
-        // Try again.
-        // FIXME: Shouldn't this continue rather than return?
-        auto &lock = GetSwiftScratchContextLock();
-        if (!lock.try_lock()) {
-          if (log)
-            log->Printf("module scratch context has errors but couldn't "
-                        "acquire scratch context lock\n");
-          return;
-        }
-        std::lock_guard<std::shared_mutex> unlock(lock, std::adopt_lock);
-        m_scratch_typesystem_for_module.erase(key);
-        if (log)
-          log->Printf("erased module-wide scratch context with errors\n");
-        return;
-      }
-      if (log)
-        log->PutCString("returned cached module-wide scratch context");
-      return;
-    }
-
-    if (!create_on_demand) {
-      if (log)
-        log->PutCString("not allowed to create a new context");
-      return;
-    }
-
-    // Call for its side effects of establishing the Swift scratch type
-    // system.
-    auto type_system_or_err =
-        GetScratchTypeSystemForLanguage(eLanguageTypeSwift, false);
-    if (!type_system_or_err) {
-      llvm::consumeError(type_system_or_err.takeError());
-      return;
-    }
-
-    auto &lock = GetSwiftScratchContextLock();
-    if (!lock.try_lock()) {
-      if (log)
-        log->PutCString("couldn't acquire scratch context lock");
-      return;
-    }
-    std::lock_guard<std::shared_mutex> unlock(lock, std::adopt_lock);
-
-    // With the lock held, get the current scratch type system. This ensures
-    // the current instance is used even in the unlikely event it was changed
-    // during the brief window between the call to
-    // `GetScratchTypeSystemForLanguage` and taking the lock.
-    type_system_or_err = m_scratch_type_system_map.GetTypeSystemForLanguage(
-        eLanguageTypeSwift, this, false);
-    if (!type_system_or_err) {
-      llvm::consumeError(type_system_or_err.takeError());
-      return;
-    }
-
-    if (auto *global_scratch_ctx =
-            llvm::cast_or_null<TypeSystemSwiftTypeRefForExpressions>(
-                type_system_or_err->get()))
-      if (auto *swift_ast_ctx =
-              llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
-                  global_scratch_ctx->GetSwiftASTContext(sc)))
-        DisplayFallbackSwiftContextErrors(swift_ast_ctx);
-
-    auto typesystem_sp = std::make_shared<TypeSystemSwiftTypeRefForExpressions>(
-        lldb::eLanguageTypeSwift, *this, *lldb_module);
-    typesystem_sp->GetSwiftASTContext(sc);
-    m_scratch_typesystem_for_module.insert({key, typesystem_sp});
-    if (log)
-      log->PutCString("created module-wide scratch context");
-    return;
-  };
-
   std::optional<SwiftScratchContextReader> reader;
-  if (lldb_module && m_use_scratch_typesystem_per_module) {
-    maybe_create_fallback_context();
-    std::shared_lock<std::shared_mutex> lock(GetSwiftScratchContextLock());
-    if (auto *cached_ts = get_cached_module_ts(lldb_module)) {
-      reader = SwiftScratchContextReader(std::move(lock), *cached_ts);
-      if (log)
-        log->PutCString("returned project-wide scratch context");
-    }
-  }
   // FIXME: Don't return the project-wide context after requesting the
   // module-wide one.
   if (!reader) {

lldb/test/API/lang/swift/clangimporter/hard_macro_conflict/TestSwiftHardMacroConflict.py

@@ -33,12 +33,3 @@ def test(self):
         threads = lldbutil.get_threads_stopped_at_breakpoint(
             process, b_breakpoint)
         self.expect("expression foo", error=True)
-
-        per_module_fallback = 0
-        import io
-        with open(log, "r", encoding='utf-8') as logfile:
-            for line in logfile:
-                if 'SwiftASTContextForExpressions("Framework")' in line:
-                    per_module_fallback += 1
-        self.assertGreater(per_module_fallback, 0,
-                           "failed to create per-module scratch context")