[lldb] Report clang module build remarks

[kastiglione]

Summary

Update the Clang diagnostic consumer (in ClangModulesDeclVendor) to report
progress on Clang module builds, as both progress events and expression logs.

Module build remarks are enabled by with clang's -Rmodule-build flag.

With this change, command line users of lldb will see progress events showing
which modules are being built, and - by how long they stay on screen - how much
time it takes to build them. IDEs that show progress events can show these
updates if desired.

This does not show module-import remarks, although that may be added as a
future change.

[mib]

lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

84

Progress makes use of RAII to complete the progress report event.

I think a local variable would be more suitable.

201–205

I guess this could be refactored in a lambda function

212–215

same here

[kastiglione]

lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

84

Since this is a callback, and the work being done doesn't happen in this function, I reasoned that that a local wouldn't match the semantics. Since this function is so short lived, the Progress would be immediately destructed and completed, but the work of building the module is not yet completed. Also, when an event is completed, the console text is cleared. As a result, the user doesn't see which module is currently building.

[aprantl]

Nice! I have a suggestion to speed up the test and make it a little more robust.

lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

84

Might be good to comment this?

201

IIUC, this is redundant?
Ah — unless this is about the ordering and having the old one destroyed first, in which case this makes sense.

202

For some reason I prefer writing
m_current_progress_up = std::make_unique<Progress>(...)
but I think for all practical intents this is equivalent.

lldb/test/API/functionalities/progress_reporting/clang_modules/TestClangModuleBuildProgress.py

21

FYI this is the same as
mod_cache = self.getBuildArtifact("new-modules")

37

This is a really indirect way of triggering a module import.
Why not expr -- @import Foo?
This way you can also import an empty module instead of an expensive uncached compilation of all of Foundation.

[kastiglione]

lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

84

by extending the life of the Progress, this code, which clears the console of the current progress message, is deferred.

https://github.com/apple/llvm-project/blob/d0fbbd7e14e13b0f915eefdb20b57a60eb177039/lldb/source/Core/Debugger.cpp#L1945-L1949

if (data->GetCompleted() == data->GetTotal()) {
  // Clear the current line.
  output->Printf("\x1B[2K");
  output->Flush();
  return;

This screen clearing deferred until either a new remark_module_build is emitted, or a remark_module_build_done is emitted. This allows the current message to stay on for screen, showing the user what's being built and giving them a sense for how long it's taking.

[kastiglione]

lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

201

yes it's ordering, destroying the previous event first, before constructing the new one. I will make the comment more clear.

202

that works for me, I thought about this too. This can also apply to the nullptr:

m_current_progress_up = nullptr;

lldb/test/API/functionalities/progress_reporting/clang_modules/TestClangModuleBuildProgress.py

21

cool

37

sounds great.

[kastiglione]

Optimized test by using a minimal clang module; Addressed review feedback

[aprantl]

I like the new test! LGTM if @mib is happy.

[mib]

Yep, LGTM!

lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

 //===-- ClangModulesDeclVendor.cpp ----------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/DiagnosticFrontend.h"
+#include "clang/Basic/DiagnosticSerialization.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Parse/Parser.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Serialization/ASTReader.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Threading.h"
 
 #include "ClangHost.h"
 #include "ClangModulesDeclVendor.h"
 #include "ModuleDependencyCollector.h"
 
 #include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
 #include "lldb/Core/ModuleList.h"
+#include "lldb/Core/Progress.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/SourceModule.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/LLDBLog.h"
public:
 
   void DumpDiagnostics(Stream &error_stream);
 
   void BeginSourceFile(const clang::LangOptions &LangOpts,
                        const clang::Preprocessor *PP = nullptr) override;
   void EndSourceFile() override;
 
 private:
+  bool HandleModuleRemark(const clang::Diagnostic &info);
+  void SetCurrentModuleProgress(llvm::StringRef module_name);
+
   typedef std::pair<clang::DiagnosticsEngine::Level, std::string>
       IDAndDiagnostic;
   std::vector<IDAndDiagnostic> m_diagnostics;
   /// The DiagnosticPrinter used for creating the full diagnostic messages
   /// that are stored in m_diagnostics.
   std::shared_ptr<clang::TextDiagnosticPrinter> m_diag_printer;
   /// Output stream of m_diag_printer.
   std::shared_ptr<llvm::raw_string_ostream> m_os;
   /// Output string filled by m_os. Will be reused for different diagnostics.
   std::string m_output;
   Log *m_log;
+  /// A Progress with explicitly managed lifetime.
+  std::unique_ptr<Progress> m_current_progress_up;
+  std::vector<std::string> m_module_build_stack;
 };
 
 /// The private implementation of our ClangModulesDeclVendor.  Contains all the
 /// Clang state required to load modules.
 class ClangModulesDeclVendorImpl : public ClangModulesDeclVendor {
 public:
   ClangModulesDeclVendorImpl(
       llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diagnostics_engine,
StoringDiagnosticConsumer::StoringDiagnosticConsumer() {
   clang::DiagnosticOptions *m_options = new clang::DiagnosticOptions();
   m_os = std::make_shared<llvm::raw_string_ostream>(m_output);
   m_diag_printer =
       std::make_shared<clang::TextDiagnosticPrinter>(*m_os, m_options);
 }
 
 void StoringDiagnosticConsumer::HandleDiagnostic(
     clang::DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &info) {
+  if (HandleModuleRemark(info))
+    return;
+
   // Print the diagnostic to m_output.
   m_output.clear();
   m_diag_printer->HandleDiagnostic(DiagLevel, info);
   m_os->flush();
 
   // Store the diagnostic for later.
   m_diagnostics.push_back(IDAndDiagnostic(DiagLevel, m_output));
 }

 }
 
 void StoringDiagnosticConsumer::BeginSourceFile(
     const clang::LangOptions &LangOpts, const clang::Preprocessor *PP) {
   m_diag_printer->BeginSourceFile(LangOpts, PP);
 }
 
 void StoringDiagnosticConsumer::EndSourceFile() {
+  m_current_progress_up = nullptr;
   m_diag_printer->EndSourceFile();
 }
 
+bool StoringDiagnosticConsumer::HandleModuleRemark(
+    const clang::Diagnostic &info) {
+  Log *log = GetLog(LLDBLog::Expressions);
+  switch (info.getID()) {
+  case clang::diag::remark_module_build: {
+    const auto &module_name = info.getArgStdStr(0);
+    SetCurrentModuleProgress(module_name);
+    m_module_build_stack.push_back(module_name);
+
+    const auto &module_path = info.getArgStdStr(1);
+    LLDB_LOG(log, "Building Clang module {0} as {1}", module_name, module_path);
+    return true;
+  }
+  case clang::diag::remark_module_build_done: {
+    // The current module is done.
+    m_module_build_stack.pop_back();
+    if (m_module_build_stack.empty()) {
+      m_current_progress_up = nullptr;
+    } else {
+      // Update the progress to re-show the module that was currently being
+      // built from the time the now completed module was originally began.
+      const auto &resumed_module_name = m_module_build_stack.back();
+      SetCurrentModuleProgress(resumed_module_name);
+    }
+
+    const auto &module_name = info.getArgStdStr(0);
+    LLDB_LOG(log, "Finished building Clang module {0}", module_name);
+    return true;
+  }
+  default:
+    return false;
+  }
+}
+
+void StoringDiagnosticConsumer::SetCurrentModuleProgress(
+    llvm::StringRef module_name) {
+  // Ensure the ordering of:
+  //   1. Completing the existing progress event.
+  //   2. Beginining a new progress event.
+  m_current_progress_up = nullptr;
+  m_current_progress_up = std::make_unique<Progress>(
+      llvm::formatv("Currently building module {0}", module_name));
+}
+
 ClangModulesDeclVendor::ClangModulesDeclVendor()
     : ClangDeclVendor(eClangModuleDeclVendor) {}
 
 ClangModulesDeclVendor::~ClangModulesDeclVendor() = default;
 
 ClangModulesDeclVendorImpl::ClangModulesDeclVendorImpl(
     llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diagnostics_engine,
     std::shared_ptr<clang::CompilerInvocation> compiler_invocation,
std::vector<std::string> compiler_invocation_arguments = {
       "-fimplicit-module-maps",
       "-fcxx-modules",
       "-fsyntax-only",
       "-femit-all-decls",
       "-target",
       arch.GetTriple().str(),
       "-fmodules-validate-system-headers",
       "-Werror=non-modular-include-in-framework-module",
-      "-Xclang=-fincremental-extensions"};
+      "-Xclang=-fincremental-extensions",
+      "-Rmodule-build"};
 
   target.GetPlatform()->AddClangModuleCompilationOptions(
       &target, compiler_invocation_arguments);
 
   compiler_invocation_arguments.push_back(ModuleImportBufferName);
 
   // Add additional search paths with { "-I", path } or { "-F", path } here.
 
for (size_t spi = 0, spe = module_search_paths.GetSize(); spi < spe; ++spi) {
     FileSpec clang_resource_dir = GetClangResourceDir();
 
     if (FileSystem::Instance().IsDirectory(clang_resource_dir.GetPath())) {
       compiler_invocation_arguments.push_back("-resource-dir");
       compiler_invocation_arguments.push_back(clang_resource_dir.GetPath());
     }
   }
 
-  llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diagnostics_engine =
-      clang::CompilerInstance::createDiagnostics(new clang::DiagnosticOptions,
-                                                 new StoringDiagnosticConsumer);
-
   std::vector<const char *> compiler_invocation_argument_cstrs;
   compiler_invocation_argument_cstrs.reserve(
       compiler_invocation_arguments.size());
   for (const std::string &arg : compiler_invocation_arguments)
     compiler_invocation_argument_cstrs.push_back(arg.c_str());
 
+  auto diag_options_up =
+      clang::CreateAndPopulateDiagOpts(compiler_invocation_argument_cstrs);
+  llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine> diagnostics_engine =
+      clang::CompilerInstance::createDiagnostics(diag_options_up.release(),
+                                                 new StoringDiagnosticConsumer);
+
   Log *log = GetLog(LLDBLog::Expressions);
   LLDB_LOG(log, "ClangModulesDeclVendor's compiler flags {0:$[ ]}",
            llvm::make_range(compiler_invocation_arguments.begin(),
                             compiler_invocation_arguments.end()));
 
   clang::CreateInvocationOptions CIOpts;
   CIOpts.Diags = diagnostics_engine;
   std::shared_ptr<clang::CompilerInvocation> invocation =

lldb/test/API/functionalities/progress_reporting/clang_modules/Makefile

+OBJC_SOURCES := main.m
+CFLAGS_EXTRAS = -fmodules -I$(BUILDDIR)
+
+include Makefile.rules

lldb/test/API/functionalities/progress_reporting/clang_modules/MyModule.h

+extern int doesNotActuallyExist;

lldb/test/API/functionalities/progress_reporting/clang_modules/TestClangModuleBuildProgress.py

+"""
+Test clang module build progress events.
+"""
+import os
+import shutil
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class TestCase(TestBase):
+    @skipUnlessDarwin
+    def test_clang_module_build_progress_report(self):
+        """Test receipt of progress events for clang module builds"""
+        self.build()
+
+        # Ensure an empty module cache.
+        mod_cache = self.getBuildArtifact("new-modules")
+        if os.path.isdir(mod_cache):
+            shutil.rmtree(mod_cache)
+        self.runCmd(f"settings set symbols.clang-modules-cache-path '{mod_cache}'")
+
+        # TODO: The need for this seems like a bug.
+        self.runCmd(
+            f"settings set target.clang-module-search-paths '{self.getSourceDir()}'"
+        )
+
+        lldbutil.run_to_name_breakpoint(self, "main")
+
+        # Just before triggering module builds, start listening for progress
+        # events. Listening any earlier would result in a queue filled with
+        # other unrelated progress events.
+        broadcaster = self.dbg.GetBroadcaster()
+        listener = lldbutil.start_listening_from(
+            broadcaster, lldb.SBDebugger.eBroadcastBitProgress
+        )
+
+        # Trigger module builds.
+        self.expect("expression @import MyModule")
+
+        event = lldbutil.fetch_next_event(self, listener, broadcaster)
+        payload = lldb.SBDebugger.GetProgressFromEvent(event)
+        message = payload[0]
+        self.assertEqual(message, "Currently building module MyModule")

lldb/test/API/functionalities/progress_reporting/clang_modules/main.m

+int main() {}

lldb/test/API/functionalities/progress_reporting/clang_modules/module.modulemap

+module MyModule {
+  header "MyModule.h"
+  export *
+}