[lldb] Add an artifact provider mechanism to Diagnostics

[JDevlieghere]

Add a hook that lets a subsystem contribute a file to the diagnostics
bundle, without breaking layering and making Core depend on it. A plugin registers an
ArtifactProvider callback and a file name via AddArtifactProvider.
Providers run when a bundle is collected, and each one that yields
content is written into the bundle and recorded in the report's
attachments.

This restores, in a simpler form, the collection point that the removed
Diagnostics callback mechanism offered. The motivating consumer is the
downstream Swift health check, which registers a provider to write its
"swift-healthcheck.log" into the bundle. Unlike the old callback, a
provider does not open or write its own file: it just returns the
contents as a string and Diagnostics handles the rest.

I have a follow-up PR that takes advantage of this new mechanism
upstream to collect the GDB remote packet log.

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

[llvmorg-github-actions]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Add a hook that lets a subsystem contribute a file to the diagnostics
bundle, without breaking layering and making Core depend on it. A plugin registers an
ArtifactProvider callback and a file name via AddArtifactProvider.
Providers run when a bundle is collected, and each one that yields
content is written into the bundle and recorded in the report's
attachments.

This restores, in a simpler form, the collection point that the removed
Diagnostics callback mechanism offered. The motivating consumer is the
downstream Swift health check, which registers a provider to write its
"swift-healthcheck.log" into the bundle. Unlike the old callback, a
provider does not open or write its own file: it just returns the
contents as a string and Diagnostics handles the rest.

I have a follow-up PR that takes advantage of this new mechanism
upstream to collect the GDB remote packet log.

---

_{Stack created with <a href="https://github.com/github/gh-stack"&gt;GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback"&gt;Give Feedback 💬</a>}

---

Full diff: https://github.com/llvm/llvm-project/pull/210417.diff

4 Files Affected:

• (modified) lldb/include/lldb/Core/Diagnostics.h (+30)
• (modified) lldb/source/Core/Diagnostics.cpp (+30)
• (modified) lldb/unittests/Core/CMakeLists.txt (+1)
• (added) lldb/unittests/Core/DiagnosticsTest.cpp (+140)

diff --git a/lldb/include/lldb/Core/Diagnostics.h b/lldb/include/lldb/Core/Diagnostics.h
index 554314e32e8ce..bcacf3b9c301d 100644
--- a/lldb/include/lldb/Core/Diagnostics.h
+++ b/lldb/include/lldb/Core/Diagnostics.h
@@ -14,6 +14,8 @@
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/Error.h"
 
+#include <functional>
+#include <mutex>
 #include <optional>
 #include <string>
 #include <vector>
@@ -90,6 +92,19 @@ class Diagnostics {
   /// Record a diagnostic message into the always-on, in-memory log.
   void Record(llvm::StringRef message);
 
+  /// Supplies an artifact's contents on demand. Subsystems register a provider
+  /// so Core need not depend on them. Each runs when a bundle is collected.
+  using ArtifactProvider = std::function<std::string()>;
+  using ArtifactProviderID = uint64_t;
+
+  /// Register \p provider to contribute file \p name. Returns an id for
+  /// RemoveArtifactProvider. Thread-safe.
+  ArtifactProviderID AddArtifactProvider(std::string name,
+                                         ArtifactProvider provider);
+
+  /// Unregister a provider. Thread-safe.
+  void RemoveArtifactProvider(ArtifactProviderID id);
+
   static Diagnostics &Instance();
 
   static DiagnosticsProperties &GetGlobalProperties();
@@ -122,6 +137,8 @@ class Diagnostics {
   static void CollectBinaries(const ExecutionContext &exe_ctx,
                               const FileSpec &dir,
                               std::vector<std::string> &files);
+  void CollectArtifactProviders(const FileSpec &dir,
+                                std::vector<std::string> &files);
   /// @}
 
   /// Scalars carried in the report rather than written as files.
@@ -131,6 +148,19 @@ class Diagnostics {
   /// @}
 
   RotatingLogHandler m_log_handler;
+
+  struct ArtifactProviderEntry {
+    ArtifactProviderID id;
+    std::string name;
+    ArtifactProvider provider;
+  };
+
+  /// Registered artifact providers, guarded by the mutex.
+  /// @{
+  ArtifactProviderID m_next_artifact_provider_id = 0;
+  std::vector<ArtifactProviderEntry> m_artifact_providers;
+  std::mutex m_artifact_providers_mutex;
+  /// @}
 };
 
 /// Render a diagnostics report as JSON, for `diagnostics dump`'s terminal
diff --git a/lldb/source/Core/Diagnostics.cpp b/lldb/source/Core/Diagnostics.cpp
index 311d8fab2b1bc..08adcaebdf038 100644
--- a/lldb/source/Core/Diagnostics.cpp
+++ b/lldb/source/Core/Diagnostics.cpp
@@ -25,12 +25,14 @@
 #include "lldb/Utility/ProcessInfo.h"
 #include "lldb/Version/Version.h"
 
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/raw_ostream.h"
 
+#include <mutex>
 #include <optional>
 #include <string>
 #include <vector>
@@ -149,6 +151,20 @@ void Diagnostics::Record(llvm::StringRef message) {
   m_log_handler.Emit(message);
 }
 
+Diagnostics::ArtifactProviderID
+Diagnostics::AddArtifactProvider(std::string name, ArtifactProvider provider) {
+  std::lock_guard<std::mutex> guard(m_artifact_providers_mutex);
+  ArtifactProviderID id = m_next_artifact_provider_id++;
+  m_artifact_providers.push_back({id, std::move(name), std::move(provider)});
+  return id;
+}
+
+void Diagnostics::RemoveArtifactProvider(ArtifactProviderID id) {
+  std::lock_guard<std::mutex> guard(m_artifact_providers_mutex);
+  llvm::erase_if(m_artifact_providers,
+                 [id](const ArtifactProviderEntry &e) { return e.id == id; });
+}
+
 // Write a single artifact into the bundle and, on success, record its name in
 // \p files. Best-effort: a write failure leaves the file out of the list, so a
 // missing artifact stays visible. The file is made owner-only because the
@@ -253,6 +269,7 @@ Diagnostics::Collect(Debugger &debugger, const ExecutionContext &exe_ctx,
   CollectCommands(debugger, exe_ctx, dir, report.attachments.files);
   if (GetGlobalProperties().GetCollectBinaries())
     CollectBinaries(exe_ctx, dir, report.attachments.files);
+  CollectArtifactProviders(dir, report.attachments.files);
 
   report.version = lldb_private::GetVersion();
   report.os = GetHostDescription(exe_ctx);
@@ -317,6 +334,19 @@ void Diagnostics::CollectBinaries(const ExecutionContext &exe_ctx,
     CopyBinary(process->GetCoreFile(), dir, files);
 }
 
+void Diagnostics::CollectArtifactProviders(const FileSpec &dir,
+                                           std::vector<std::string> &files) {
+  // Snapshot under the lock, then run providers without it: a provider can be
+  // slow and must not block registration or removal.
+  std::vector<ArtifactProviderEntry> providers;
+  {
+    std::lock_guard<std::mutex> guard(m_artifact_providers_mutex);
+    providers = m_artifact_providers;
+  }
+  for (const ArtifactProviderEntry &entry : providers)
+    WriteArtifact(dir, entry.name, entry.provider(), files);
+}
+
 std::string Diagnostics::GetHostDescription(const ExecutionContext &exe_ctx) {
   std::string os = HostInfo::GetTargetTriple().str();
   Target *target = exe_ctx.GetTargetPtr();
diff --git a/lldb/unittests/Core/CMakeLists.txt b/lldb/unittests/Core/CMakeLists.txt
index d69432d332f44..959793d7d4de0 100644
--- a/lldb/unittests/Core/CMakeLists.txt
+++ b/lldb/unittests/Core/CMakeLists.txt
@@ -3,6 +3,7 @@ add_lldb_unittest(LLDBCoreTests
   DebuggerTest.cpp
   CommunicationTest.cpp
   DiagnosticEventTest.cpp
+  DiagnosticsTest.cpp
   DumpDataExtractorTest.cpp
   DumpRegisterInfoTest.cpp
   FormatEntityTest.cpp
diff --git a/lldb/unittests/Core/DiagnosticsTest.cpp b/lldb/unittests/Core/DiagnosticsTest.cpp
new file mode 100644
index 0000000000000..6bf0e7720b8fe
--- /dev/null
+++ b/lldb/unittests/Core/DiagnosticsTest.cpp
@@ -0,0 +1,140 @@
+//===----------------------------------------------------------------------===//
+//
+// 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 "lldb/Core/Diagnostics.h"
+#include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+#include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h"
+#include "TestingSupport/TestUtilities.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/ExecutionContext.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+#include <mutex>
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+class DiagnosticsTest : public ::testing::Test {
+public:
+  void SetUp() override {
+    FileSystem::Initialize();
+    HostInfo::Initialize();
+    PlatformMacOSX::Initialize();
+    std::call_once(TestUtilities::g_debugger_initialize_flag,
+                   []() { Debugger::Initialize(nullptr); });
+    ArchSpec arch("x86_64-apple-macosx-");
+    Platform::SetHostPlatform(
+        PlatformRemoteMacOSX::CreateInstance(true, &arch));
+  }
+  void TearDown() override {
+    PlatformMacOSX::Terminate();
+    HostInfo::Terminate();
+    FileSystem::Terminate();
+  }
+};
+
+// Read a file from the bundle directory, or "" if it cannot be read.
+std::string ReadBundleFile(const FileSpec &dir, llvm::StringRef name) {
+  FileSpec path = dir.CopyByAppendingPathComponent(name);
+  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> buffer =
+      llvm::MemoryBuffer::getFile(path.GetPath());
+  if (!buffer)
+    return "";
+  return (*buffer)->getBuffer().str();
+}
+} // namespace
+
+TEST_F(DiagnosticsTest, ArtifactProviderIDsAreUnique) {
+  Diagnostics diagnostics;
+  Diagnostics::ArtifactProviderID id0 =
+      diagnostics.AddArtifactProvider("a.txt", [] { return "a"; });
+  Diagnostics::ArtifactProviderID id1 =
+      diagnostics.AddArtifactProvider("b.txt", [] { return "b"; });
+  EXPECT_NE(id0, id1);
+}
+
+TEST_F(DiagnosticsTest, ArtifactProviderContributesToBundle) {
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+
+  Diagnostics diagnostics;
+  int call_count = 0;
+  Diagnostics::ArtifactProviderID id = diagnostics.AddArtifactProvider(
+      "my-artifact.txt", [&call_count]() -> std::string {
+        ++call_count;
+        return "hello diagnostics";
+      });
+
+  ExecutionContext exe_ctx;
+
+  // A registered provider is invoked and its content lands in the bundle and
+  // in the report's attachments.
+  {
+    llvm::Expected<FileSpec> dir = Diagnostics::CreateUniqueDirectory();
+    ASSERT_THAT_EXPECTED(dir, llvm::Succeeded());
+    llvm::Expected<Diagnostics::Report> report =
+        diagnostics.Collect(*debugger_sp, exe_ctx, *dir);
+    ASSERT_THAT_EXPECTED(report, llvm::Succeeded());
+
+    EXPECT_EQ(call_count, 1);
+    EXPECT_TRUE(
+        llvm::is_contained(report->attachments.files, "my-artifact.txt"));
+    EXPECT_EQ(ReadBundleFile(*dir, "my-artifact.txt"), "hello diagnostics");
+
+    llvm::sys::fs::remove_directories(dir->GetPath());
+  }
+
+  // After removal the provider is neither invoked nor present in the bundle.
+  diagnostics.RemoveArtifactProvider(id);
+  call_count = 0;
+  {
+    llvm::Expected<FileSpec> dir = Diagnostics::CreateUniqueDirectory();
+    ASSERT_THAT_EXPECTED(dir, llvm::Succeeded());
+    llvm::Expected<Diagnostics::Report> report =
+        diagnostics.Collect(*debugger_sp, exe_ctx, *dir);
+    ASSERT_THAT_EXPECTED(report, llvm::Succeeded());
+
+    EXPECT_EQ(call_count, 0);
+    EXPECT_FALSE(
+        llvm::is_contained(report->attachments.files, "my-artifact.txt"));
+
+    llvm::sys::fs::remove_directories(dir->GetPath());
+  }
+
+  Debugger::Destroy(debugger_sp);
+}
+
+TEST_F(DiagnosticsTest, RemoveArtifactProviderIgnoresUnknownID) {
+  Diagnostics diagnostics;
+  Diagnostics::ArtifactProviderID id =
+      diagnostics.AddArtifactProvider("kept.txt", [] { return "kept"; });
+
+  // Removing an id that was never handed out must not disturb other providers.
+  diagnostics.RemoveArtifactProvider(id + 1);
+
+  DebuggerSP debugger_sp = Debugger::CreateInstance();
+  ASSERT_TRUE(debugger_sp);
+  ExecutionContext exe_ctx;
+  llvm::Expected<FileSpec> dir = Diagnostics::CreateUniqueDirectory();
+  ASSERT_THAT_EXPECTED(dir, llvm::Succeeded());
+  llvm::Expected<Diagnostics::Report> report =
+      diagnostics.Collect(*debugger_sp, exe_ctx, *dir);
+  ASSERT_THAT_EXPECTED(report, llvm::Succeeded());
+
+  EXPECT_TRUE(llvm::is_contained(report->attachments.files, "kept.txt"));
+
+  llvm::sys::fs::remove_directories(dir->GetPath());
+  Debugger::Destroy(debugger_sp);
+}