llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) <details> <summary>Changes</summary> 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. --- <sub>Stack created with <a href="https://github.com/github/gh-stack">GitHub Stacks CLI</a> • <a href="https://gh.io/stacks-feedback">Give Feedback 💬</a></sub> --- 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 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); +} `````````` </details> https://github.com/llvm/llvm-project/pull/210417 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
