Merge pull request #11098 from adrian-prantl/156647851

[lldb] Set module import callbacks more consistently in more places

Author: adrian-prantl

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

@@ -1272,18 +1272,22 @@ SwiftExpressionParser::ParseAndImport(
     SwiftASTContext::ScopedDiagnostics &expr_diagnostics,
     SwiftExpressionParser::SILVariableMap &variable_map, unsigned &buffer_id,
     DiagnosticManager &diagnostic_manager) {
+
   Log *log = GetLog(LLDBLog::Expressions);
   bool repl = m_options.GetREPLEnabled();
   bool playground = m_options.GetPlaygroundTransformEnabled();
+
+  // Install a progress meter.
+  auto progress_raii = m_swift_ast_ctx.GetModuleImportProgressRAII(
+      "Importing modules used in expression");
+
   // If we are using the playground, hand import the necessary
   // modules.
   //
   // FIXME: We won't have to do this once the playground adds import
   //        statements for the things it needs itself.
   if (playground) {
-    SourceModule module_info;
-    module_info.path.emplace_back("Swift");
-    auto module_or_err = m_swift_ast_ctx.GetModule(module_info);
+    auto module_or_err = m_swift_ast_ctx.ImportStdlib();
 
     if (!module_or_err) {
       LLDB_LOG(log, "couldn't load Swift Standard Library");

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

@@ -840,9 +840,7 @@ bool SwiftUserExpression::Parse(DiagnosticManager &diagnostic_manager,
   if (!persistent_state)
     return error("could not start parsing (no persistent data)");
 
-  SourceModule module_info;
-  module_info.path.emplace_back("Swift");
-  auto module_decl_or_err = m_swift_ast_ctx->GetModule(module_info);
+  auto module_decl_or_err = m_swift_ast_ctx->ImportStdlib();
   if (!module_decl_or_err)
     return error("could not load Swift Standard Library",
                  llvm::toString(module_decl_or_err.takeError()).c_str());

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

@@ -3871,6 +3871,54 @@ void SwiftASTContext::CacheModule(std::string module_name,
   m_swift_module_cache.insert({module_name, *module});
 }
 
+/// An RAII object to install a progress report callback.
+SwiftASTContext::ModuleImportProgressRAII::ModuleImportProgressRAII(
+    SwiftASTContext &ctx, std::string category)
+    : m_ts(ctx.shared_from_this()), m_progress(category) {
+  if (!m_ts)
+    return;
+  ThreadSafeASTContext ast = ctx.GetASTContext();
+  if (!ast)
+    return;
+  ast->SetPreModuleImportCallback(
+      [&](llvm::StringRef name, swift::ASTContext::ModuleImportKind kind) {
+        switch (kind) {
+        case swift::ASTContext::Module:
+          m_progress.Increment(1, name.str());
+          break;
+        case swift::ASTContext::Overlay:
+          m_progress.Increment(1, name.str() + " (overlay)");
+          break;
+        case swift::ASTContext::BridgingHeader: {
+          // Module imports generate remarks, which are logged, but bridging
+          // headers don't.
+          auto &m_description = ctx.GetDescription();
+          HEALTH_LOG_PRINTF("Compiling bridging header: %s",
+                            name.str().c_str());
+          m_progress.Increment(1, "Compiling bridging header: " + name.str());
+          break;
+        }
+        }
+      });
+}
+
+SwiftASTContext::ModuleImportProgressRAII::~ModuleImportProgressRAII() {
+  if (!m_ts)
+    return;
+  ThreadSafeASTContext ast =
+      llvm::cast<SwiftASTContext>(m_ts.get())->GetASTContext();
+  if (!ast)
+    return;
+  ast->SetPreModuleImportCallback(
+      [](llvm::StringRef, swift::ASTContext::ModuleImportKind) {});
+}
+
+std::unique_ptr<SwiftASTContext::ModuleImportProgressRAII>
+SwiftASTContext::GetModuleImportProgressRAII(std::string category) {
+  return std::make_unique<SwiftASTContext::ModuleImportProgressRAII>(*this,
+                                                                     category);
+}
+
 llvm::Expected<swift::ModuleDecl &>
 SwiftASTContext::GetModule(const SourceModule &module, bool *cached) {
   if (cached)
@@ -3910,36 +3958,6 @@ SwiftASTContext::GetModule(const SourceModule &module, bool *cached) {
   // Create a diagnostic consumer for the diagnostics produced by the import.
   auto import_diags = getScopedDiagnosticConsumer();
 
-  // Report progress on module importing by using a callback function in
-  // swift::ASTContext.
-  std::unique_ptr<Progress> progress;
-  ast->SetPreModuleImportCallback(
-      [&progress](llvm::StringRef module_name,
-                  swift::ASTContext::ModuleImportKind kind) {
-        if (!progress)
-          progress = std::make_unique<Progress>("Importing Swift modules");
-        switch (kind) {
-        case swift::ASTContext::Module:
-          progress->Increment(1, module_name.str());
-          break;
-        case swift::ASTContext::Overlay:
-          progress->Increment(1, module_name.str() + " (overlay)");
-          break;
-        case swift::ASTContext::BridgingHeader:
-          progress->Increment(1, "Compiling bridging header: " +
-                                     module_name.str());
-          break;
-        }
-      });
-
-  // Clear the callback function on scope exit to prevent an out-of-scope access
-  // of the progress local variable
-  auto on_exit = llvm::make_scope_exit([&]() {
-    ast->SetPreModuleImportCallback(
-        [](llvm::StringRef module_name,
-           swift::ASTContext::ModuleImportKind kind) {});
-  });
-
   swift::ModuleDecl *module_decl = ast->getModuleByName(module_name);
 
   // Error handling.
@@ -3966,6 +3984,13 @@ SwiftASTContext::GetModule(const SourceModule &module, bool *cached) {
   return *module_decl;
 }
 
+llvm::Expected<swift::ModuleDecl &>
+SwiftASTContext::ImportStdlib() {
+  SourceModule module_info;
+  module_info.path.emplace_back(swift::STDLIB_NAME);
+  return GetModule(module_info);
+}
+
 llvm::Expected<swift::ModuleDecl &>
 SwiftASTContext::GetModule(const FileSpec &module_spec) {
   VALID_OR_RETURN(llvm::createStringError("no context"));
@@ -4520,15 +4545,9 @@ void SwiftASTContext::ImportSectionModules(
     Module &module, const std::vector<std::string> &module_names) {
   VALID_OR_RETURN();
 
-  Progress progress("Loading Swift module dependencies",
-                    module.GetFileSpec().GetFilename().GetString(),
-                    module_names.size());
-
-  size_t completion = 0;
+  auto module_import_progress_raii =
+      GetModuleImportProgressRAII("Importing Swift section modules");
   for (const std::string &module_name : module_names) {
-    // We have to increment the completion value even if we can't get the module
-    // object to stay in-sync with the total progress reporting.
-    progress.Increment(++completion, module_name);
     SourceModule module_info;
     module_info.path.push_back(ConstString(module_name));
     auto module_or_err = GetModule(module_info);
@@ -9128,9 +9147,6 @@ bool SwiftASTContextForExpressions::CacheUserImports(
 
   auto src_file_imports = source_file.getImports();
 
-  Progress progress("Importing modules used in expression");
-  size_t completion = 0;
-
   /// Find all explicit imports in the expression.
   struct UserImportFinder : public swift::ASTWalker {
     llvm::SmallDenseSet<swift::ModuleDecl*, 1> imports;
@@ -9146,9 +9162,6 @@ bool SwiftASTContextForExpressions::CacheUserImports(
   source_file.walk(import_finder);
 
   for (const auto &attributed_import : src_file_imports) {
-    progress.Increment(
-        ++completion,
-        attributed_import.module.importedModule->getModuleFilename().str());
     swift::ModuleDecl *module = attributed_import.module.importedModule;
     if (module && import_finder.imports.count(module)) {
       std::string module_name;
@@ -9248,7 +9261,7 @@ bool SwiftASTContext::GetCompileUnitImportsImpl(
 
   // Import the Swift standard library and its dependencies.
   SourceModule swift_module;
-  swift_module.path.emplace_back("Swift");
+  swift_module.path.emplace_back(swift::STDLIB_NAME);
   auto *stdlib = LoadOneModule(swift_module, *this, process_sp,
                                /*import_dylibs=*/true, error);
   if (!stdlib)
@@ -9297,13 +9310,11 @@ bool SwiftASTContext::GetCompileUnitImportsImpl(
   }
 
   LOG_PRINTF(GetLog(LLDBLog::Types), "Importing dependencies of current CU");
-  
-  std::string category = "Importing Swift module dependencies for ";
+  std::string category = "Importing dependencies for ";
   category += compile_unit->GetPrimaryFile().GetFilename().GetString();
-  Progress progress(category, "", cu_imports.size());
-  size_t completion = 0;
+  auto module_import_progress_raii = GetModuleImportProgressRAII(category);
+ 
   for (const SourceModule &module : cu_imports) {
-    progress.Increment(++completion, llvm::join(module.path, "."));
     // When building the Swift stdlib with debug info these will
     // show up in "Swift.o", but we already imported them and
     // manually importing them will fail.

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

@@ -17,6 +17,7 @@
 #include "Plugins/TypeSystem/Swift/TypeSystemSwift.h"
 #include "Plugins/TypeSystem/Swift/TypeSystemSwiftTypeRef.h"
 
+#include "lldb/Core/Progress.h"
 #include "lldb/Core/SwiftForward.h"
 #include "lldb/Core/ThreadSafeDenseSet.h"
 #include "lldb/Expression/DiagnosticManager.h"
@@ -325,13 +326,28 @@ class SwiftASTContext : public TypeSystemSwift {
   CreateModule(std::string module_name, swift::ImplicitImportInfo importInfo,
                swift::ModuleDecl::PopulateFilesFn populateFiles);
 
+  /// An RAII object to install a progress report callback.
+  class ModuleImportProgressRAII {
+    lldb::TypeSystemSP m_ts;
+    Progress m_progress;
+
+  public:
+    ModuleImportProgressRAII(SwiftASTContext &ctx, std::string category);
+    ~ModuleImportProgressRAII();
+  };
+
+  /// Install and return a module import RAII object.
+  std::unique_ptr<ModuleImportProgressRAII>
+  GetModuleImportProgressRAII(std::string category);
+
   // This function should only be called when all search paths
   // for all items in a swift::ASTContext have been setup to
   // allow for imports to happen correctly. Use with caution,
   // or use the GetModule() call that takes a FileSpec.
   llvm::Expected<swift::ModuleDecl &> GetModule(const SourceModule &module,
                                                 bool *cached = nullptr);
   llvm::Expected<swift::ModuleDecl &> GetModule(const FileSpec &module_spec);
+  llvm::Expected<swift::ModuleDecl &> ImportStdlib();
 
   void CacheModule(std::string module_name, swift::ModuleDecl *module);
 

lldb/test/API/functionalities/progress_reporting/swift_progress_reporting/TestSwiftProgressReporting.py

@@ -35,12 +35,11 @@ def test_swift_progress_report(self):
         self.runCmd("v s")
 
         beacons = [
-            "Loading Swift module",
             "Compiling bridging header",
             "Importing modules used in expression",
             "Setting up Swift reflection",
-            "Importing Swift module dependencies for main.swift",
-            "Importing Swift modules",
+            "Importing dependencies for main.swift",
+            "Importing dependencies for main.swift: Foundation",
             "Importing Swift standard library",
         ]
 
@@ -51,15 +50,6 @@ def test_swift_progress_report(self):
             if self.TraceOn():
                 print(ret_args[0])
 
-            # When importing Swift modules, make sure that we don't get two reports
-            # in a row with the title "Importing Swift modules", i.e. there should be
-            # a report with that title followed by a report with that title and details
-            # attached.
-            if ret_args[0] == "Importing Swift modules":
-                next_event = lldbutil.fetch_next_event(self, self.listener, self.broadcaster)
-                next_ret_args = lldb.SBDebugger.GetProgressFromEvent(next_event)
-                self.assertRegexpMatches(next_ret_args[0], r"Importing Swift modules:+")
-
             for beacon in beacons:
                 if beacon in ret_args[0]:
                     beacons.remove(beacon)