[Lldb-commits] [lldb] Reapply LLDB-Telemetry TargetInfo branch (pr/127834) (PR #132043)
Vy Nguyen via lldb-commits
lldb-commits at lists.llvm.org
Thu Mar 20 14:41:57 PDT 2025
https://github.com/oontvoo updated https://github.com/llvm/llvm-project/pull/132043
>From 9f0a47af2b7fdb90e4fa4cc7f8f97c840af1d2bc Mon Sep 17 00:00:00 2001
From: Vy Nguyen <vyng at google.com>
Date: Wed, 19 Mar 2025 10:44:12 -0400
Subject: [PATCH 1/7] Reapply "[LLDB][Telemetry]Define TargetInfo for
collecting data about a target (#127834)"
This reverts commit 7dbcdd578cd4d37b1f4094dbd17556be6382f1cc.
---
lldb/include/lldb/Core/Telemetry.h | 74 +++++++++++++++++--
lldb/source/Core/Telemetry.cpp | 25 ++++++-
.../source/Interpreter/CommandInterpreter.cpp | 2 +-
lldb/source/Target/Process.cpp | 21 ++++++
lldb/source/Target/Target.cpp | 21 ++++++
5 files changed, 134 insertions(+), 9 deletions(-)
diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h
index 29ec36b2d64d1..28897f283dc55 100644
--- a/lldb/include/lldb/Core/Telemetry.h
+++ b/lldb/include/lldb/Core/Telemetry.h
@@ -23,9 +23,12 @@
#include <atomic>
#include <chrono>
#include <ctime>
+#include <functional>
#include <memory>
#include <optional>
#include <string>
+#include <type_traits>
+#include <utility>
namespace lldb_private {
namespace telemetry {
@@ -46,12 +49,18 @@ struct LLDBConfig : public ::llvm::telemetry::Config {
// Specifically:
// - Length: 8 bits
// - First two bits (MSB) must be 11 - the common prefix
+// - Last two bits (LSB) are reserved for grand-children of LLDBTelemetryInfo
// If any of the subclass has descendents, those descendents
-// must have their LLDBEntryKind in the similar form (ie., share common prefix)
+// must have their LLDBEntryKind in the similar form (ie., share common prefix
+// and differ by the last two bits)
struct LLDBEntryKind : public ::llvm::telemetry::EntryKind {
- static const llvm::telemetry::KindType BaseInfo = 0b11000000;
- static const llvm::telemetry::KindType CommandInfo = 0b11010000;
- static const llvm::telemetry::KindType DebuggerInfo = 0b11000100;
+ // clang-format off
+ static const llvm::telemetry::KindType BaseInfo = 0b11000000;
+ static const llvm::telemetry::KindType CommandInfo = 0b11010000;
+ static const llvm::telemetry::KindType DebuggerInfo = 0b11001000;
+ static const llvm::telemetry::KindType ExecModuleInfo = 0b11000100;
+ static const llvm::telemetry::KindType ProcessExitInfo = 0b11001100;
+ // clang-format on
};
/// Defines a convenient type for timestamp of various events.
@@ -89,7 +98,7 @@ struct CommandInfo : public LLDBBaseTelemetryInfo {
/// session. Necessary because we'd send off an entry right before a command's
/// execution and another right after. This is to avoid losing telemetry if
/// the command does not execute successfully.
- uint64_t command_id;
+ uint64_t command_id = 0;
/// The command name(eg., "breakpoint set")
std::string command_name;
/// These two fields are not collected by default due to PII risks.
@@ -116,7 +125,7 @@ struct CommandInfo : public LLDBBaseTelemetryInfo {
void serialize(llvm::telemetry::Serializer &serializer) const override;
- static uint64_t GetNextId();
+ static uint64_t GetNextID();
private:
// We assign each command (in the same session) a unique id so that their
@@ -146,6 +155,59 @@ struct DebuggerInfo : public LLDBBaseTelemetryInfo {
void serialize(llvm::telemetry::Serializer &serializer) const override;
};
+struct ExecutableModuleInfo : public LLDBBaseTelemetryInfo {
+ lldb::ModuleSP exec_mod;
+ /// The same as the executable-module's UUID.
+ UUID uuid;
+ /// PID of the process owned by this target.
+ lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
+ /// The triple of this executable module.
+ std::string triple;
+
+ /// If true, this entry was emitted at the beginning of an event (eg., before
+ /// the executable is set). Otherwise, it was emitted at the end of an
+ /// event (eg., after the module and any dependency were loaded.)
+ bool is_start_entry = false;
+
+ ExecutableModuleInfo() = default;
+
+ llvm::telemetry::KindType getKind() const override {
+ return LLDBEntryKind::ExecModuleInfo;
+ }
+
+ static bool classof(const TelemetryInfo *T) {
+ // Subclasses of this is also acceptable
+ return (T->getKind() & LLDBEntryKind::ExecModuleInfo) ==
+ LLDBEntryKind::ExecModuleInfo;
+ }
+ void serialize(llvm::telemetry::Serializer &serializer) const override;
+};
+
+/// Describes an exit status.
+struct ExitDescription {
+ int exit_code;
+ std::string description;
+};
+
+struct ProcessExitInfo : public LLDBBaseTelemetryInfo {
+ // The executable-module's UUID.
+ UUID module_uuid;
+ lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
+ bool is_start_entry = false;
+ std::optional<ExitDescription> exit_desc;
+
+ llvm::telemetry::KindType getKind() const override {
+ return LLDBEntryKind::ProcessExitInfo;
+ }
+
+ static bool classof(const TelemetryInfo *T) {
+ // Subclasses of this is also acceptable
+ return (T->getKind() & LLDBEntryKind::ProcessExitInfo) ==
+ LLDBEntryKind::ProcessExitInfo;
+ }
+ void serialize(llvm::telemetry::Serializer &serializer) const override;
+};
+
/// The base Telemetry manager instance in LLDB.
/// This class declares additional instrumentation points
/// applicable to LLDB.
diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
index be929a644706e..62ebdfc027d81 100644
--- a/lldb/source/Core/Telemetry.cpp
+++ b/lldb/source/Core/Telemetry.cpp
@@ -76,6 +76,9 @@ void CommandInfo::serialize(Serializer &serializer) const {
serializer.write("error_data", error_data.value());
}
+std::atomic<uint64_t> CommandInfo::g_command_id_seed = 1;
+uint64_t CommandInfo::GetNextID() { return g_command_id_seed.fetch_add(1); }
+
void DebuggerInfo::serialize(Serializer &serializer) const {
LLDBBaseTelemetryInfo::serialize(serializer);
@@ -83,8 +86,26 @@ void DebuggerInfo::serialize(Serializer &serializer) const {
serializer.write("is_exit_entry", is_exit_entry);
}
-std::atomic<uint64_t> CommandInfo::g_command_id_seed = 0;
-uint64_t CommandInfo::GetNextId() { return g_command_id_seed.fetch_add(1); }
+void ExecutableModuleInfo::serialize(Serializer &serializer) const {
+ LLDBBaseTelemetryInfo::serialize(serializer);
+
+ serializer.write("uuid", uuid.GetAsString());
+ serializer.write("pid", pid);
+ serializer.write("triple", triple);
+ serializer.write("is_start_entry", is_start_entry);
+}
+
+void ProcessExitInfo::serialize(Serializer &serializer) const {
+ LLDBBaseTelemetryInfo::serialize(serializer);
+
+ serializer.write("module_uuid", module_uuid.GetAsString());
+ serializer.write("pid", pid);
+ serializer.write("is_start_entry", is_start_entry);
+ if (exit_desc.has_value()) {
+ serializer.write("exit_code", exit_desc->exit_code);
+ serializer.write("exit_desc", exit_desc->description);
+ }
+}
TelemetryManager::TelemetryManager(std::unique_ptr<LLDBConfig> config)
: m_config(std::move(config)), m_id(MakeUUID()) {}
diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp
index 8e70922b9bb8d..e0bf5520e16ef 100644
--- a/lldb/source/Interpreter/CommandInterpreter.cpp
+++ b/lldb/source/Interpreter/CommandInterpreter.cpp
@@ -1891,7 +1891,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line,
telemetry::TelemetryManager::GetInstance()
->GetConfig()
->detailed_command_telemetry;
- const int command_id = telemetry::CommandInfo::GetNextId();
+ const int command_id = telemetry::CommandInfo::GetNextID();
std::string command_string(command_line);
std::string original_command_string(command_string);
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 335991018070c..d1962efec98a9 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -22,6 +22,7 @@
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
#include "lldb/Core/Progress.h"
+#include "lldb/Core/Telemetry.h"
#include "lldb/Expression/DiagnosticManager.h"
#include "lldb/Expression/DynamicCheckerFunctions.h"
#include "lldb/Expression/UserExpression.h"
@@ -1066,6 +1067,26 @@ const char *Process::GetExitDescription() {
bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
// Use a mutex to protect setting the exit status.
std::lock_guard<std::mutex> guard(m_exit_status_mutex);
+ telemetry::ScopedDispatcher<telemetry::ProcessExitInfo> helper;
+
+ // Find the executable-module's UUID, if available.
+ Target &target = GetTarget();
+ helper.SetDebugger(&(target.GetDebugger()));
+ UUID module_uuid;
+ if (ModuleSP mod = target.GetExecutableModule())
+ module_uuid = mod->GetUUID();
+
+ helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
+ info->module_uuid = module_uuid;
+ info->pid = m_pid;
+ info->is_start_entry = true;
+ info->exit_desc = {status, exit_string.str()};
+ });
+
+ helper.DispatchOnExit([&](telemetry::ProcessExitInfo *info) {
+ info->module_uuid = module_uuid;
+ info->pid = m_pid;
+ });
Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
LLDB_LOG(log, "(plugin = {0} status = {1} ({1:x8}), description=\"{2}\")",
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index bbc2110dada5b..c26bca546891e 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -24,6 +24,7 @@
#include "lldb/Core/Section.h"
#include "lldb/Core/SourceManager.h"
#include "lldb/Core/StructuredDataImpl.h"
+#include "lldb/Core/Telemetry.h"
#include "lldb/DataFormatters/FormatterSection.h"
#include "lldb/Expression/DiagnosticManager.h"
#include "lldb/Expression/ExpressionVariable.h"
@@ -1559,10 +1560,30 @@ void Target::DidExec() {
void Target::SetExecutableModule(ModuleSP &executable_sp,
LoadDependentFiles load_dependent_files) {
+ telemetry::ScopedDispatcher<telemetry::ExecutableModuleInfo> helper(
+ &m_debugger);
Log *log = GetLog(LLDBLog::Target);
ClearModules(false);
if (executable_sp) {
+ lldb::pid_t pid = LLDB_INVALID_PROCESS_ID;
+ if (ProcessSP proc = GetProcessSP())
+ pid = proc->GetID();
+
+ helper.DispatchNow([&](telemetry::ExecutableModuleInfo *info) {
+ info->exec_mod = executable_sp;
+ info->uuid = executable_sp->GetUUID();
+ info->pid = pid;
+ info->triple = executable_sp->GetArchitecture().GetTriple().getTriple();
+ info->is_start_entry = true;
+ });
+
+ helper.DispatchOnExit([&](telemetry::ExecutableModuleInfo *info) {
+ info->exec_mod = executable_sp;
+ info->uuid = executable_sp->GetUUID();
+ info->pid = pid;
+ });
+
ElapsedTime elapsed(m_stats.GetCreateTime());
LLDB_SCOPED_TIMERF("Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
>From 8a84a2e3328d7058917a63693632bacb0357f639 Mon Sep 17 00:00:00 2001
From: Vy Nguyen <vyng at google.com>
Date: Wed, 19 Mar 2025 10:44:59 -0400
Subject: [PATCH 2/7] add check to avoid accessing invalid obj
---
lldb/source/Target/Process.cpp | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index d1962efec98a9..43d7b4625310b 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -1070,11 +1070,13 @@ bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
telemetry::ScopedDispatcher<telemetry::ProcessExitInfo> helper;
// Find the executable-module's UUID, if available.
- Target &target = GetTarget();
- helper.SetDebugger(&(target.GetDebugger()));
UUID module_uuid;
- if (ModuleSP mod = target.GetExecutableModule())
- module_uuid = mod->GetUUID();
+ TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid));
+ if (target_sp) {
+ helper.SetDebugger(&target_sp->GetDebugger());
+ if (ModuleSP mod = target_sp->GetExecutableModule())
+ module_uuid = mod->GetUUID();
+ }
helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
info->module_uuid = module_uuid;
>From b5a58376cad92f8a0ba11f69df6d7771c828af5a Mon Sep 17 00:00:00 2001
From: Vy Nguyen <vyng at google.com>
Date: Wed, 19 Mar 2025 10:52:13 -0400
Subject: [PATCH 3/7] reformat
---
lldb/source/Target/Process.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 43d7b4625310b..00faa8cc82dee 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -1072,11 +1072,11 @@ bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
// Find the executable-module's UUID, if available.
UUID module_uuid;
TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid));
- if (target_sp) {
+ if (target_sp) {
helper.SetDebugger(&target_sp->GetDebugger());
if (ModuleSP mod = target_sp->GetExecutableModule())
module_uuid = mod->GetUUID();
- }
+ }
helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
info->module_uuid = module_uuid;
>From 10528c98e55980d2110a328cc145170192ba4d31 Mon Sep 17 00:00:00 2001
From: Vy Nguyen <vyng at google.com>
Date: Wed, 19 Mar 2025 14:56:09 -0400
Subject: [PATCH 4/7] only record exit telemetry for non-success case
---
lldb/source/Target/Process.cpp | 42 +++++++++++++++++++---------------
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 00faa8cc82dee..67cec82e7a4ae 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -1069,27 +1069,31 @@ bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
std::lock_guard<std::mutex> guard(m_exit_status_mutex);
telemetry::ScopedDispatcher<telemetry::ProcessExitInfo> helper;
- // Find the executable-module's UUID, if available.
- UUID module_uuid;
- TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid));
- if (target_sp) {
- helper.SetDebugger(&target_sp->GetDebugger());
- if (ModuleSP mod = target_sp->GetExecutableModule())
- module_uuid = mod->GetUUID();
- }
-
- helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
- info->module_uuid = module_uuid;
- info->pid = m_pid;
- info->is_start_entry = true;
- info->exit_desc = {status, exit_string.str()};
- });
+ // Only dispatch telemetry for a non-success case to reduce
+ // chances of slowing down the code.
+ // FIXME: Remove this conditional monitoring as it means we lose the ability
+ // to monitor exit-operations' time for the average case.
+ if (status != 0) {
+ UUID module_uuid;
+ TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid));
+ if (target_sp) {
+ helper.SetDebugger(&target_sp->GetDebugger());
+ if (ModuleSP mod = target_sp->GetExecutableModule())
+ module_uuid = mod->GetUUID();
+ }
- helper.DispatchOnExit([&](telemetry::ProcessExitInfo *info) {
- info->module_uuid = module_uuid;
- info->pid = m_pid;
- });
+ helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
+ info->module_uuid = module_uuid;
+ info->pid = m_pid;
+ info->is_start_entry = true;
+ info->exit_desc = {status, exit_string.str()};
+ });
+ helper.DispatchOnExit([&](telemetry::ProcessExitInfo *info) {
+ info->module_uuid = module_uuid;
+ info->pid = m_pid;
+ });
+ }
Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
LLDB_LOG(log, "(plugin = {0} status = {1} ({1:x8}), description=\"{2}\")",
GetPluginName(), status, exit_string);
>From 0aab7b9194b3d1db6668f701b6fd03d25d98a3fc Mon Sep 17 00:00:00 2001
From: Vy Nguyen <vyng at google.com>
Date: Thu, 20 Mar 2025 10:13:28 -0400
Subject: [PATCH 5/7] check .lock() on the weak ptr Target
---
lldb/source/Target/Process.cpp | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 67cec82e7a4ae..56afe30748357 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -1073,10 +1073,10 @@ bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
// chances of slowing down the code.
// FIXME: Remove this conditional monitoring as it means we lose the ability
// to monitor exit-operations' time for the average case.
- if (status != 0) {
+ //if (status != 0)
+ {
UUID module_uuid;
- TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid));
- if (target_sp) {
+ if (TargetSP target_sp = m_target_wp.lock()) {
helper.SetDebugger(&target_sp->GetDebugger());
if (ModuleSP mod = target_sp->GetExecutableModule())
module_uuid = mod->GetUUID();
>From 5af3c4566815a61a025cc06fdae5871643dd2446 Mon Sep 17 00:00:00 2001
From: Vy Nguyen <vyng at google.com>
Date: Thu, 20 Mar 2025 11:24:10 -0400
Subject: [PATCH 6/7] cleanup
---
lldb/source/Target/Process.cpp | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 56afe30748357..27bd9c7d84f9a 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -1069,13 +1069,8 @@ bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
std::lock_guard<std::mutex> guard(m_exit_status_mutex);
telemetry::ScopedDispatcher<telemetry::ProcessExitInfo> helper;
- // Only dispatch telemetry for a non-success case to reduce
- // chances of slowing down the code.
- // FIXME: Remove this conditional monitoring as it means we lose the ability
- // to monitor exit-operations' time for the average case.
- //if (status != 0)
- {
UUID module_uuid;
+ // Need this check because the pointer may not be valid at this point.
if (TargetSP target_sp = m_target_wp.lock()) {
helper.SetDebugger(&target_sp->GetDebugger());
if (ModuleSP mod = target_sp->GetExecutableModule())
@@ -1093,7 +1088,7 @@ bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
info->module_uuid = module_uuid;
info->pid = m_pid;
});
- }
+
Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
LLDB_LOG(log, "(plugin = {0} status = {1} ({1:x8}), description=\"{2}\")",
GetPluginName(), status, exit_string);
>From 088c87b2ec9ffd880315e190dca3c59eab57d7da Mon Sep 17 00:00:00 2001
From: Vy Nguyen <vyng at google.com>
Date: Thu, 20 Mar 2025 13:28:34 -0400
Subject: [PATCH 7/7] reformat
---
lldb/source/Target/Process.cpp | 36 +++++++++++++++++-----------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 27bd9c7d84f9a..369933234ccca 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -1069,25 +1069,25 @@ bool Process::SetExitStatus(int status, llvm::StringRef exit_string) {
std::lock_guard<std::mutex> guard(m_exit_status_mutex);
telemetry::ScopedDispatcher<telemetry::ProcessExitInfo> helper;
- UUID module_uuid;
- // Need this check because the pointer may not be valid at this point.
- if (TargetSP target_sp = m_target_wp.lock()) {
- helper.SetDebugger(&target_sp->GetDebugger());
- if (ModuleSP mod = target_sp->GetExecutableModule())
- module_uuid = mod->GetUUID();
- }
-
- helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
- info->module_uuid = module_uuid;
- info->pid = m_pid;
- info->is_start_entry = true;
- info->exit_desc = {status, exit_string.str()};
- });
+ UUID module_uuid;
+ // Need this check because the pointer may not be valid at this point.
+ if (TargetSP target_sp = m_target_wp.lock()) {
+ helper.SetDebugger(&target_sp->GetDebugger());
+ if (ModuleSP mod = target_sp->GetExecutableModule())
+ module_uuid = mod->GetUUID();
+ }
+
+ helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
+ info->module_uuid = module_uuid;
+ info->pid = m_pid;
+ info->is_start_entry = true;
+ info->exit_desc = {status, exit_string.str()};
+ });
- helper.DispatchOnExit([&](telemetry::ProcessExitInfo *info) {
- info->module_uuid = module_uuid;
- info->pid = m_pid;
- });
+ helper.DispatchOnExit([&](telemetry::ProcessExitInfo *info) {
+ info->module_uuid = module_uuid;
+ info->pid = m_pid;
+ });
Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
LLDB_LOG(log, "(plugin = {0} status = {1} ({1:x8}), description=\"{2}\")",
More information about the lldb-commits
mailing list