Restore the per-module scratch context fallback mechanism.

lldb/include/lldb/Core/SwiftScratchContextReader.h

@@ -40,6 +40,7 @@ class SharedMutex {
   }
   bool unlock_shared() {
     std::lock_guard<std::mutex> lock(m_reader_mutex);
+    assert(m_readers > 0);
     --m_readers;
     return m_impl.unlock_shared();
   }
@@ -57,7 +58,12 @@ struct ScopedSharedMutexReader {
     if (m_mutex)
       m_mutex->lock_shared();
   }
-  ScopedSharedMutexReader(const ScopedSharedMutexReader&) = default;
+
+  ScopedSharedMutexReader(const ScopedSharedMutexReader &copy)
+      : m_mutex(copy.m_mutex) {
+    if (m_mutex)
+      m_mutex->lock_shared();
+  }
 
   ~ScopedSharedMutexReader() {
     if (m_mutex)
@@ -101,9 +107,6 @@ class SwiftScratchContextReader : ScopedSharedMutexReader {
     assert(m_ptr && "invalid context");
   }
 
-  SwiftScratchContextReader(const SwiftScratchContextReader &copy)
-      : ScopedSharedMutexReader(copy.m_mutex), m_ptr(copy.m_ptr) {}
-
   TypeSystemSwiftTypeRefForExpressions *get() {
     assert(m_ptr && "invalid context");
     return m_ptr;

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

@@ -1206,15 +1206,12 @@ struct SwiftASTContextError : public llvm::ErrorInfo<SwiftASTContextError> {
 struct ModuleImportError : public llvm::ErrorInfo<ModuleImportError> {
   static char ID;
   std::string msg;
-  bool is_explicit;
+  bool is_new_dylib;
 
-  ModuleImportError(llvm::Twine message, bool is_explicit = false)
-      : msg(message.str()), is_explicit(is_explicit) {}
+  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 {
-    if (is_explicit)
-      OS << "error while processing import statement:";
-    else
-      OS << "error while importing implicit dependency:";
+    OS << "error while processing module import: ";
     OS << msg;
   }
   std::error_code convertToErrorCode() const override {
@@ -1406,7 +1403,9 @@ static llvm::Expected<ParsedExpression> ParseAndImport(
   swift::performImportResolution(*source_file);
 
   if (swift_ast_context.HasErrors())
-    return make_error<SwiftASTContextError>();
+    return make_error<ModuleImportError>(
+        swift_ast_context.GetAllErrors().AsCString(
+            "Explicit module import error"));
 
   std::unique_ptr<SwiftASTManipulator> code_manipulator;
   if (repl || !playground) {
@@ -1471,9 +1470,14 @@ static llvm::Expected<ParsedExpression> ParseAndImport(
                                            process_sp, *source_file,
                                            auto_import_error)) {
       const char *msg = auto_import_error.AsCString();
-      if (!msg)
-        msg = "error status positive, but import still failed";
-      return make_error<ModuleImportError>(msg, /*explicit=*/true);
+      if (!msg) {
+        // The import itself succeeded, but the AST context is in a
+        // 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";
+      }
+      return make_error<ModuleImportError>(msg, /*is_new_dylib=*/true);
     }
   }
 
@@ -1523,43 +1527,43 @@ unsigned SwiftExpressionParser::Parse(DiagnosticManager &diagnostic_manager,
       *this, m_stack_frame_wp, m_sc, *m_exe_scope, m_options, repl, playground);
 
   if (!parsed_expr) {
-    bool user_import = false;
     bool retry = false;
-    handleAllErrors(parsed_expr.takeError(),
-                    [&](const ModuleImportError &MIE) {
-                      if (MIE.is_explicit) {
-                        // A new dylib may have poisoned the context.
-                        retry = true;
-                        user_import = true;
-                      }
-                      if (m_swift_ast_ctx.GetClangImporter())
-                        // Already on backup power.
-                        diagnostic_manager.PutString(eDiagnosticSeverityError,
-                                                     MIE.message());
-                      else
-                        // Discard the shared scratch context and retry.
-                        retry = true;
-                    },
-                    [&](const SwiftASTContextError &SACE) {
-                      DiagnoseSwiftASTContextError();
-                    },
-                    [&](const StringError &SE) {
-                      diagnostic_manager.PutString(eDiagnosticSeverityError,
-                                                   SE.getMessage());
-                    },
-                    [](const PropagatedError &P) {});
-
-    // Unrecoverable error?
-    if (!retry)
-      return 1;
+    handleAllErrors(
+        parsed_expr.takeError(),
+        [&](const ModuleImportError &MIE) {
+          diagnostic_manager.PutString(eDiagnosticSeverityError, MIE.message());
+          // There are no fallback contexts in REPL and playgrounds.
+          if (repl || playground || MIE.is_new_dylib) {
+            retry = true;
+            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;
+          }
+        },
+        [&](const SwiftASTContextError &SACE) {
+          DiagnoseSwiftASTContextError();
+        },
+        [&](const StringError &SE) {
+          diagnostic_manager.PutString(eDiagnosticSeverityError,
+                                       SE.getMessage());
+        },
+        [](const PropagatedError &P) {});
 
     // 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.
-    if (!user_import)
-      m_sc.target_sp->SetUseScratchTypesystemPerModule(true);
-    return 2;
+    if (retry)
+      return 2;
+
+    // Unrecoverable error.
+    return 1;
   }
 
   swift::bindExtensions(parsed_expr->module);

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

@@ -57,18 +57,9 @@ SwiftUserExpression::SwiftUserExpression(
       m_type_system_helper(*m_target_wp.lock().get()),
       m_result_delegate(exe_scope.CalculateTarget(), *this, false),
       m_error_delegate(exe_scope.CalculateTarget(), *this, true),
-      m_persistent_variable_delegate(*this),
-      m_swift_scratch_ctx(
-          exe_scope.CalculateTarget()
-              ? exe_scope.CalculateTarget()->GetSwiftScratchContext(m_err,
-                                                                    exe_scope)
-              : llvm::None) {
+      m_persistent_variable_delegate(*this) {
   m_runs_in_playground_or_repl =
       options.GetREPLEnabled() || options.GetPlaygroundTransformEnabled();
-  if (m_swift_scratch_ctx)
-    if (m_swift_scratch_ctx->get())
-      m_swift_ast_ctx = llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
-          m_swift_scratch_ctx->get()->GetSwiftASTContext());
 }
 
 SwiftUserExpression::~SwiftUserExpression() {}
@@ -305,8 +296,27 @@ bool SwiftUserExpression::Parse(DiagnosticManager &diagnostic_manager,
     return false;
   }
 
+  auto exe_scope = exe_ctx.GetBestExecutionContextScope();
+  if (!exe_scope) {
+    LLDB_LOG(log, "no execution context scope");
+    return false;
+  }
+
+  m_swift_scratch_ctx = target->GetSwiftScratchContext(m_err, *exe_scope);
+  if (!m_swift_scratch_ctx) {
+    LLDB_LOG(log, "no scratch context", m_err.AsCString());
+    return false;
+  }
+  m_swift_ast_ctx = llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
+      m_swift_scratch_ctx->get()->GetSwiftASTContext());
+
   if (!m_swift_ast_ctx) {
-    LLDB_LOG(log, "no Swift AST Context");
+    LLDB_LOG(log, "no Swift AST context");
+    return false;
+  }
+
+  if (m_swift_ast_ctx->HasFatalErrors()) {
+    LLDB_LOG(log, "Swift AST context is in a fatal error state");
     return false;
   }
 
@@ -397,22 +407,6 @@ bool SwiftUserExpression::Parse(DiagnosticManager &diagnostic_manager,
     Callback m_callback;
   };
 
-  ExecutionContextScope *exe_scope = NULL;
-
-  Process *process = exe_ctx.GetProcessPtr();
-
-  do {
-    exe_scope = exe_ctx.GetFramePtr();
-    if (exe_scope)
-      break;
-
-    exe_scope = process;
-    if (exe_scope)
-      break;
-
-    exe_scope = exe_ctx.GetTargetPtr();
-  } while (0);
-
   auto *swift_parser =
       new SwiftExpressionParser(exe_scope, *m_swift_ast_ctx, *this, m_options);
   unsigned error_code = swift_parser->Parse(
@@ -482,6 +476,7 @@ bool SwiftUserExpression::Parse(DiagnosticManager &diagnostic_manager,
   auto module =
       m_execution_unit_sp->CreateJITModule(jit_module_name.GetString().data());
 
+  Process *process = exe_ctx.GetProcessPtr();
   auto *swift_runtime = SwiftLanguageRuntime::Get(process);
   if (module && swift_runtime) {
     ModuleList modules;

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

@@ -179,11 +179,11 @@ class SwiftUserExpression : public LLVMUserExpression {
     void DidDematerialize(lldb::ExpressionVariableSP &variable) override;
   };
 
+  llvm::Optional<SwiftScratchContextReader> m_swift_scratch_ctx;
   SwiftASTContextForExpressions *m_swift_ast_ctx;
   PersistentVariableDelegate m_persistent_variable_delegate;
   std::unique_ptr<SwiftExpressionParser> m_parser;
   Status m_err;
-  llvm::Optional<SwiftScratchContextReader> m_swift_scratch_ctx;
   bool m_runs_in_playground_or_repl;
   bool m_needs_object_ptr = false;
   bool m_in_static_method = false;

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

@@ -2154,18 +2154,23 @@ Status SwiftASTContext::IsCompatible() { return GetFatalErrors(); }
 
 Status SwiftASTContext::GetFatalErrors() const {
   Status error;
-  if (HasFatalErrors()) {
-    error = m_fatal_errors;
-    if (error.Success()) {
-      // Retrieve the error message from the DiagnosticConsumer.
-      DiagnosticManager diagnostic_manager;
-      PrintDiagnostics(diagnostic_manager);
-      error.SetErrorString(diagnostic_manager.GetString());
-    }
+  if (HasFatalErrors())
+    error = GetAllErrors();
+  return error;
+}
+
+Status SwiftASTContext::GetAllErrors() const {
+  Status error = m_fatal_errors;
+  if (error.Success()) {
+    // Retrieve the error message from the DiagnosticConsumer.
+    DiagnosticManager diagnostic_manager;
+    PrintDiagnostics(diagnostic_manager);
+    error.SetErrorString(diagnostic_manager.GetString());
   }
   return error;
 }
 
+
 void SwiftASTContext::LogFatalErrors() const {
   // Avoid spamming the health log with redundant copies of the fatal error.
   if (m_logged_fatal_error) {

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

@@ -437,12 +437,13 @@ class SwiftASTContext : public TypeSystemSwift {
 
   const SwiftModuleMap &GetModuleCache() { return m_swift_module_cache; }
 
+  void RaiseFatalError(std::string msg) { m_fatal_errors.SetErrorString(msg); }
   static bool HasFatalErrors(swift::ASTContext *ast_context);
-
   bool HasFatalErrors() const {
     return m_fatal_errors.Fail() || HasFatalErrors(m_ast_context_ap.get());
   }
 
+  Status GetAllErrors() const;
   Status GetFatalErrors() const;
   void DiagnoseWarnings(Process &process, Module &module) const override;
   void LogFatalErrors() const;

lldb/source/Target/Target.cpp

@@ -2352,8 +2352,9 @@ Target::GetScratchTypeSystemForLanguage(lldb::LanguageType language,
   }
 
   if (m_cant_make_scratch_type_system.count(language))
-    return llvm::make_error<llvm::StringError>("unable to construct scratch type system",
-                                               llvm::inconvertibleErrorCode());
+    return llvm::make_error<llvm::StringError>(
+        "unable to construct scratch type system",
+        llvm::inconvertibleErrorCode());
 
   auto type_system_or_err = m_scratch_type_system_map.GetTypeSystemForLanguage(
       language, this, create_on_demand, compiler_options);
@@ -2587,7 +2588,7 @@ Target::CreateUtilityFunction(std::string expression, std::string name,
 #ifdef LLDB_ENABLE_SWIFT
 llvm::Optional<SwiftScratchContextReader> Target::GetSwiftScratchContext(
     Status &error, ExecutionContextScope &exe_scope, bool create_on_demand) {
-  Log *log = GetLog(LLDBLog::Target);
+  Log *log = GetLog(LLDBLog::Target | LLDBLog::Types | LLDBLog::Expressions);
   LLDB_SCOPED_TIMER();
 
   Module *lldb_module = nullptr;
@@ -2624,20 +2625,29 @@ llvm::Optional<SwiftScratchContextReader> Target::GetSwiftScratchContext(
       return cached_ts;
     }
 
-    if (!create_on_demand || !GetSwiftScratchContextLock().try_lock())
+    if (!create_on_demand) {
+      if (log)
+        log->Printf("not allowed to create a new context\n");
       return nullptr;
+    }
+    if (!GetSwiftScratchContextLock().try_lock()) {
+      if (log)
+        log->Printf("couldn't acquire scratch context lock\n");
+      return nullptr;
+    }
 
     auto unlock = llvm::make_scope_exit(
         [this] { GetSwiftScratchContextLock().unlock(); });
 
-    auto type_system_or_err = GetScratchTypeSystemForLanguage(eLanguageTypeSwift, false);
+    auto type_system_or_err =
+        GetScratchTypeSystemForLanguage(eLanguageTypeSwift, false);
     if (!type_system_or_err) {
       llvm::consumeError(type_system_or_err.takeError());
       return nullptr;
     }
 
     if (auto *global_scratch_ctx =
-            llvm::cast_or_null<SwiftASTContextForExpressions>(
+            llvm::cast_or_null<TypeSystemSwiftTypeRefForExpressions>(
                 &*type_system_or_err))
       if (auto *swift_ast_ctx =
               llvm::dyn_cast_or_null<SwiftASTContextForExpressions>(
@@ -2676,7 +2686,8 @@ llvm::Optional<SwiftScratchContextReader> Target::GetSwiftScratchContext(
 
   if (!swift_scratch_ctx)
     return llvm::None;
-  return SwiftScratchContextReader(GetSwiftScratchContextLock(), *swift_scratch_ctx);
+  return SwiftScratchContextReader(GetSwiftScratchContextLock(),
+                                   *swift_scratch_ctx);
 }
 
 static SharedMutex *

lldb/test/API/lang/swift/clangimporter/hard_macro_conflict/Bar/Bar.h

@@ -0,0 +1,3 @@
+MACRO Bar {
+  int i;
+};

lldb/test/API/lang/swift/clangimporter/hard_macro_conflict/Bar/module.modulemap

@@ -0,0 +1,3 @@
+module Bar {
+  header "Bar.h"
+}

lldb/test/API/lang/swift/clangimporter/hard_macro_conflict/Foo/Foo.h

@@ -0,0 +1,3 @@
+struct Foo {
+  MACRO j;
+};

lldb/test/API/lang/swift/clangimporter/hard_macro_conflict/Foo/module.modulemap

@@ -0,0 +1,3 @@
+module Foo {
+  header "Foo.h"
+}

lldb/test/API/lang/swift/clangimporter/hard_macro_conflict/Framework.swift

@@ -0,0 +1,8 @@
+@_implementationOnly import Foo
+
+func use<T>(_ t: T) {}
+
+public func f() {
+  let foo = Foo(j: 23)
+  use(foo) // break here
+}