Merge pull request #5308 from adrian-prantl/restore-fallback

Restore the per-module scratch context fallback mechanism.

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()