Restore the per-module scratch context fallback mechanism.

This feature has been accidentally disabled when introducing
TypeSystemSwiftTypeRefForExpressions and thanks to how good
TypeSystemSwiftTypeRef has become, all of the tests for the feature
were still passing.

Author: adrian-prantl

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

@@ -302,10 +302,7 @@ bool SwiftUserExpression::Parse(DiagnosticManager &diagnostic_manager,
     return false;
   }
 
-  ExecutionContext target_scope(*target);
-  m_swift_scratch_ctx = target->GetSwiftScratchContext(
-      m_err, /* FIXME: should be exe_scope */ *target_scope
-                 .GetBestExecutionContextScope());
+  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;
@@ -314,7 +311,12 @@ bool SwiftUserExpression::Parse(DiagnosticManager &diagnostic_manager,
       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;
   }
 

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
+}

lldb/test/API/lang/swift/clangimporter/hard_macro_conflict/Makefile

@@ -0,0 +1,23 @@
+SWIFT_SOURCES = main.swift
+SWIFT_OBJC_INTEROP := 1
+
+all: Framework.framework a.out
+
+include Makefile.rules
+BAR_FLAGS= -Xcc -I$(SRCDIR)/Bar -Xcc -DMACRO=struct
+FOO_FLAGS= -Xcc -I$(SRCDIR)/Foo -Xcc -DMACRO=int
+SWIFTFLAGS_EXTRAS=$(BAR_FLAGS) \
+  -F$(BUILDDIR) -framework Framework -L$(BUILDDIR) \
+  -Xlinker -rpath -Xlinker $(BUILDDIR)
+
+Framework.framework: Framework.swift
+	$(MAKE) -f $(MAKEFILE_RULES) -C $(BUILDDIR) VPATH=$(VPATH) \
+                MAKE_DSYM=NO CC=$(CC) SWIFTC=$(SWIFTC) \
+		ARCH=$(ARCH) DSYMUTIL=$(DSYMUTIL) \
+		DYLIB_NAME=$(shell basename $< .swift) \
+		DYLIB_ONLY=YES DYLIB_SWIFT_SOURCES=Framework.swift \
+		SWIFT_OBJC_INTEROP=1 \
+		SWIFTFLAGS_EXTRAS="$(FOO_FLAGS)" \
+		FRAMEWORK=Framework
+	rm -f $(BUILDDIR)/Framework.swiftmodule
+	ln -s $(BUILDDIR)/Framework.framework/Framework $(BUILDDIR)/Framework # FIXME

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

@@ -0,0 +1,47 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+import os
+import unittest2
+import shutil
+
+class TestSwiftHardMacroConflict(TestBase):
+
+    def setUp(self):
+        TestBase.setUp(self)
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    # Don't run ClangImporter tests if Clangimporter is disabled.
+    @skipIf(setting=('symbols.use-swift-clangimporter', 'false'))
+    @skipUnlessDarwin
+    @swiftTest
+    def test(self):
+        self.runCmd("settings set symbols.use-swift-dwarfimporter false")
+        self.build()
+        target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(
+            self, 'break here', lldb.SBFileSpec('main.swift'),
+            extra_images=['Framework.framework'])
+        b_breakpoint = target.BreakpointCreateBySourceRegex(
+            'break here', lldb.SBFileSpec('Framework.swift'))
+        log = self.getBuildArtifact("types.log")
+        self.runCmd('log enable lldb types -f "%s"' % log)
+
+        # This is expected to succeed because ClangImporter was set up
+        # with the flags from the main executable.
+        self.expect("expr bar", "expected result", substrs=["42"])
+
+        process.Continue()
+        threads = lldbutil.get_threads_stopped_at_breakpoint(
+            process, b_breakpoint)
+        self.expect("p 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")

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

@@ -0,0 +1,12 @@
+import Bar
+import Framework
+
+func use<T>(_ t: T) {}
+
+func main() {
+  let bar = Bar(i: 42)
+  f() // break here
+  use(bar)
+}
+
+main()

lldb/test/API/lang/swift/lazy_framework/TestSwiftLazyFramework.py

@@ -20,7 +20,7 @@ def test_system_framework(self):
         self.expect("settings set target.swift-auto-import-frameworks true")
         target, process, thread, bkpt = lldbutil.run_to_source_breakpoint(
             self, 'break here', lldb.SBFileSpec('main.swift'))
-        
+
         # Verify that lazy is not linked in.
         self.runCmd("image list")
         output = self.res.GetOutput()