https://github.com/oontvoo updated https://github.com/llvm/llvm-project/pull/129354
>From 5a992aff351a93ff820d15ace3ebc2bea59dd5fc Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Fri, 28 Feb 2025 23:03:35 -0500 Subject: [PATCH 01/23] [LLDB][Telemetry]Defind telemetry::CommandInfo and collect telemetry about a command's execution. --- lldb/include/lldb/Core/Telemetry.h | 127 +++++++++++++++++- lldb/source/Core/Telemetry.cpp | 14 ++ .../source/Interpreter/CommandInterpreter.cpp | 20 +++ 3 files changed, 158 insertions(+), 3 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index b72556ecaf3c9..30b8474156124 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -12,11 +12,14 @@ #include "lldb/Core/StructuredDataImpl.h" #include "lldb/Interpreter/CommandReturnObject.h" #include "lldb/Utility/StructuredData.h" +#include "lldb/Utility/LLDBLog.h" #include "lldb/lldb-forward.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" +#include "llvm/ADT/FunctionExtras.h" #include "llvm/Support/JSON.h" #include "llvm/Telemetry/Telemetry.h" +#include <atomic> #include <chrono> #include <ctime> #include <memory> @@ -27,8 +30,16 @@ namespace lldb_private { namespace telemetry { +struct LLDBConfig : public ::llvm::telemetry::Config { + const bool m_collect_original_command; + + explicit LLDBConfig(bool enable_telemetry, bool collect_original_command) + : ::llvm::telemetry::Config(enable_telemetry), m_collect_original_command(collect_original_command) {} +}; + struct LLDBEntryKind : public ::llvm::telemetry::EntryKind { - static const llvm::telemetry::KindType BaseInfo = 0b11000; + static const llvm::telemetry::KindType BaseInfo = 0b11000000; + static const llvm::telemetry::KindType CommandInfo = 0b11010000; }; /// Defines a convenient type for timestamp of various events. @@ -41,6 +52,7 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { std::optional<SteadyTimePoint> end_time; // TBD: could add some memory stats here too? + lldb::user_id_t debugger_id = LLDB_INVALID_UID; Debugger *debugger; // For dyn_cast, isa, etc operations. @@ -56,6 +68,42 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { void serialize(llvm::telemetry::Serializer &serializer) const override; }; + +struct CommandInfo : public LLDBBaseTelemetryInfo { + + // If the command is/can be associated with a target entry this field contains + // that target's UUID. <EMPTY> otherwise. + std::string target_uuid; + // A unique ID for a command so the manager can match the start entry with + // its end entry. These values only need to be unique within the same 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. + int command_id; + + // Eg., "breakpoint set" + std::string command_name; + + // !!NOTE!! These two fields are not collected (upstream) due to PII risks. + // (Downstream impl may add them if needed). + // std::string original_command; + // std::string args; + + lldb::ReturnStatus ret_status; + std::string error_data; + + + CommandInfo() = default; + + llvm::telemetry::KindType getKind() const override { return LLDBEntryKind::CommandInfo; } + + static bool classof(const llvm::telemetry::TelemetryInfo *T) { + return (T->getKind() & LLDBEntryKind::CommandInfo) == LLDBEntryKind::CommandInfo; + } + + void serialize(Serializer &serializer) const override; +}; + /// The base Telemetry manager instance in LLDB. /// This class declares additional instrumentation points /// applicable to LLDB. @@ -63,19 +111,92 @@ class TelemetryManager : public llvm::telemetry::Manager { public: llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override; + int MakeNextCommandId(); + + LLDBConfig* GetConfig() { return m_config.get(); } + virtual llvm::StringRef GetInstanceName() const = 0; static TelemetryManager *getInstance(); protected: - TelemetryManager(std::unique_ptr<llvm::telemetry::Config> config); + TelemetryManager(std::unique_ptr<LLDBConfig> config); static void setInstance(std::unique_ptr<TelemetryManager> manger); private: - std::unique_ptr<llvm::telemetry::Config> m_config; + std::unique_ptr<LLDBConfig> m_config; + const std::string m_id; + // We assign each command (in the same session) a unique id so that their + // "start" and "end" entries can be matched up. + // These values don't need to be unique across runs (because they are + // secondary-key), hence a simple counter is sufficent. + std::atomic<int> command_id_seed = 0; static std::unique_ptr<TelemetryManager> g_instance; }; +/// Helper RAII class for collecting telemetry. +template <typename Info> struct ScopedDispatcher { + // The debugger pointer is optional because we may not have a debugger yet. + // In that case, caller must set the debugger later. + ScopedDispatcher(Debugger *debugger = nullptr) { + // Start the timer. + m_start_time = std::chrono::steady_clock::now(); + debugger = debugger; + } + ScopedDispatcher(llvm::unique_function<void(Info *info)> final_callback, + Debugger *debugger = nullptr) + : m_final_callback(std::move(final_callback)) { + // Start the timer. + m_start_time = std::chrono::steady_clock::now(); + debugger = debugger; + } + + + template typename<T> + T GetIfEnable(llvm::unique_function<T(TelemetryManager*)> callable, + T default_value) { + TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled(); + if (!manager) + return default_value; + return callable(manager); + } + + void SetDebugger(Debugger *debugger) { debugger = debugger; } + + void SetFinalCallback(llvm::unique_function<void(Info *info)> final_callback) { + m_final_callback(std::move(final_callback)); + } + + void DispatchIfEnable(llvm::unique_function<void(Info *info)> populate_fields_cb) { + TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled(); + if (!manager) + return; + Info info; + // Populate the common fields we know aboutl + info.start_time = m_start_time; + info.end_time = std::chrono::steady_clock::now(); + info.debugger = debugger; + // The callback will set the rest. + populate_fields_cb(&info); + // And then we dispatch. + if (llvm::Error er = manager->dispatch(&info)) { + LLDB_LOG_ERROR(GetLog(LLDBLog::Object), std::move(er), + "Failed to dispatch entry of type: {0}", m_info.getKind()); + } + + } + + ~ScopedDispatcher() { + // TODO: check if there's a cb to call? + DispatchIfEnable(std::move(m_final_callback)); + } + +private: + SteadyTimePoint m_start_time; + llvm::unique_function<void(Info *info)> m_final_callback; + Debugger * debugger; +}; + } // namespace telemetry } // namespace lldb_private #endif // LLDB_CORE_TELEMETRY_H diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 5222f76704f91..7fb32f75f474e 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -43,6 +43,16 @@ void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const { serializer.write("end_time", ToNanosec(end_time.value())); } +void CommandInfo::serialize(Serializer &serializer) const { + LLDBBaseTelemetryInfo::serializer(serializer); + + serializer.write("target_uuid", target_uuid); + serializer.write("command_id", command_id); + serializer.write("command_name", command_name); + serializer.write("ret_status", ret_status); + serializer.write("error_data", error_data); +} + [[maybe_unused]] static std::string MakeUUID(Debugger *debugger) { uint8_t random_bytes[16]; if (auto ec = llvm::getRandomBytes(random_bytes, 16)) { @@ -66,6 +76,10 @@ llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) { return llvm::Error::success(); } +int TelemetryManager::MakeNextCommandId() { + +} + std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr; TelemetryManager *TelemetryManager::getInstance() { return g_instance.get(); } diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index c363f20081f9e..aab85145b4c3b 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -47,6 +47,7 @@ #include "lldb/Core/Debugger.h" #include "lldb/Core/PluginManager.h" +#include "lldb/Core/Telemetry.h" #include "lldb/Host/StreamFile.h" #include "lldb/Utility/ErrorMessages.h" #include "lldb/Utility/LLDBLog.h" @@ -88,6 +89,7 @@ #include "llvm/Support/Path.h" #include "llvm/Support/PrettyStackTrace.h" #include "llvm/Support/ScopedPrinter.h" +#include "llvm/Telemetry/Telemetry.h" #if defined(__APPLE__) #include <TargetConditionals.h> @@ -1883,10 +1885,28 @@ bool CommandInterpreter::HandleCommand(const char *command_line, LazyBool lazy_add_to_history, CommandReturnObject &result, bool force_repeat_command) { + lldb_private::telemetry::ScopedDispatcher< + lldb_private::telemetry:CommandInfo> helper; + const int command_id = helper.GetIfEnable<int>([](lldb_private::telemetry::TelemetryManager* ins){ + return ins->MakeNextCommandId(); }, 0); + std::string command_string(command_line); std::string original_command_string(command_string); std::string real_original_command_string(command_string); + helper.DispatchIfEnable([&](lldb_private::telemetry:CommandInfo* info, + lldb_private::telemetry::TelemetryManager* ins){ + info.command_id = command_id; + if (Target* target = GetExecutionContext().GetTargetPtr()) { + // If we have a target attached to this command, then get the UUID. + info.target_uuid = target->GetExecutableModule() != nullptr + ? GetExecutableModule()->GetUUID().GetAsString() + : ""; + } + if (ins->GetConfig()->m_collect_original_command) + info.original_command = original_command_string; + }); + Log *log = GetLog(LLDBLog::Commands); LLDB_LOGF(log, "Processing command: %s", command_line); LLDB_SCOPED_TIMERF("Processing command: %s.", command_line); >From ff1ce49d92821e5a83e66fec7f3dd95b44bcaea0 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Fri, 28 Feb 2025 23:48:20 -0500 Subject: [PATCH 02/23] more data --- lldb/include/lldb/Core/Telemetry.h | 10 ---- .../source/Interpreter/CommandInterpreter.cpp | 49 +++++++++++++------ 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 30b8474156124..8d51963fc3172 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -151,16 +151,6 @@ template <typename Info> struct ScopedDispatcher { debugger = debugger; } - - template typename<T> - T GetIfEnable(llvm::unique_function<T(TelemetryManager*)> callable, - T default_value) { - TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled(); - if (!manager) - return default_value; - return callable(manager); - } - void SetDebugger(Debugger *debugger) { debugger = debugger; } void SetFinalCallback(llvm::unique_function<void(Info *info)> final_callback) { diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index aab85145b4c3b..6e069be26928a 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1886,16 +1886,18 @@ bool CommandInterpreter::HandleCommand(const char *command_line, CommandReturnObject &result, bool force_repeat_command) { lldb_private::telemetry::ScopedDispatcher< - lldb_private::telemetry:CommandInfo> helper; - const int command_id = helper.GetIfEnable<int>([](lldb_private::telemetry::TelemetryManager* ins){ - return ins->MakeNextCommandId(); }, 0); + lldb_private::telemetry:CommandInfo> helper(m_debugger); + lldb_private::telemetry::TelemetryManager *ins = lldb_private::telemetry::TelemetryManager::GetInstanceOrDummy(); + const int command_id = ins->MakeNextCommandId(); + std::string command_string(command_line); std::string original_command_string(command_string); std::string real_original_command_string(command_string); + std::string parsed_command_args; + CommandObject *cmd_obj = nullptr; - helper.DispatchIfEnable([&](lldb_private::telemetry:CommandInfo* info, - lldb_private::telemetry::TelemetryManager* ins){ + helper.DispatchIfEnable([&](lldb_private::telemetry:CommandInfo* info){ info.command_id = command_id; if (Target* target = GetExecutionContext().GetTargetPtr()) { // If we have a target attached to this command, then get the UUID. @@ -1905,6 +1907,26 @@ bool CommandInterpreter::HandleCommand(const char *command_line, } if (ins->GetConfig()->m_collect_original_command) info.original_command = original_command_string; + // The rest (eg., command_name, args, etc) hasn't been parsed yet; + // Those will be collected by the on-exit-callback. + }); + + helper.DispatchOnExit([&](lldb_private::telemetry:CommandInfo* info) { + // TODO: this is logging the time the command-handler finishes. + // But we may want a finer-grain durations too? + // (ie., the execute_time recorded below?) + + info.command_id = command_id; + llvm::StringRef command_name = + cmd_obj ? cmd_obj->GetCommandName() : "<not found>"; + info.command_name = command_name.str(); + info.ret_status = result.GetStatus(); + if (llvm::StringRef error_data = result.GetErrorData(); + !error_data.empty()) + info.error_data = error_data.str(); + + if (ins->GetConfig()->m_collect_original_command) + info.args = parsed_command_args; }); Log *log = GetLog(LLDBLog::Commands); @@ -2011,7 +2033,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, // From 1 above, we can determine whether the Execute function wants raw // input or not. - CommandObject *cmd_obj = ResolveCommandImpl(command_string, result); + cmd_obj = ResolveCommandImpl(command_string, result); // We have to preprocess the whole command string for Raw commands, since we // don't know the structure of the command. For parsed commands, we only @@ -2073,37 +2095,36 @@ bool CommandInterpreter::HandleCommand(const char *command_line, if (add_to_history) m_command_history.AppendString(original_command_string); - std::string remainder; const std::size_t actual_cmd_name_len = cmd_obj->GetCommandName().size(); if (actual_cmd_name_len < command_string.length()) - remainder = command_string.substr(actual_cmd_name_len); + parsed_command_args = command_string.substr(actual_cmd_name_len); // Remove any initial spaces - size_t pos = remainder.find_first_not_of(k_white_space); + size_t pos = parsed_command_args.find_first_not_of(k_white_space); if (pos != 0 && pos != std::string::npos) - remainder.erase(0, pos); + parsed_command_args.erase(0, pos); LLDB_LOGF( log, "HandleCommand, command line after removing command name(s): '%s'", - remainder.c_str()); + parsed_command_args.c_str()); // To test whether or not transcript should be saved, `transcript_item` is // used instead of `GetSaveTranscript()`. This is because the latter will // fail when the command is "settings set interpreter.save-transcript true". if (transcript_item) { transcript_item->AddStringItem("commandName", cmd_obj->GetCommandName()); - transcript_item->AddStringItem("commandArguments", remainder); + transcript_item->AddStringItem("commandArguments", parsed_command_args); } ElapsedTime elapsed(execute_time); cmd_obj->SetOriginalCommandString(real_original_command_string); // Set the indent to the position of the command in the command line. - pos = real_original_command_string.rfind(remainder); + pos = real_original_command_string.rfind(parsed_command_args); std::optional<uint16_t> indent; if (pos != std::string::npos) indent = pos; result.SetDiagnosticIndent(indent); - cmd_obj->Execute(remainder.c_str(), result); + cmd_obj->Execute(parsed_command_args.c_str(), result); } LLDB_LOGF(log, "HandleCommand, command %s", >From 4e9d65e1c3f9c93eacca526f35207eea3b718f4e Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Mon, 3 Mar 2025 10:54:40 -0500 Subject: [PATCH 03/23] formatting + renaming --- lldb/include/lldb/Core/Telemetry.h | 10 +++------- lldb/source/Interpreter/CommandInterpreter.cpp | 6 ++---- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index e0334be5fa7a8..42a62cae6ee37 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -91,20 +91,16 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { // and another right after. This is to avoid losing telemetry if the command // does not execute successfully. int command_id; - // Eg., "breakpoint set" std::string command_name; - // !!NOTE!! These two fields are not collected by default due to PII risks. // Vendor may allow them by setting the LLDBConfig::m_detailed_command_telemetry. std::string original_command; std::string args; - // Return status of a command and any error description in case of error. lldb::ReturnStatus ret_status; std::string error_data; - CommandInfo() = default; llvm::telemetry::KindType getKind() const override { return LLDBEntryKind::CommandInfo; } @@ -115,7 +111,7 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { void serialize(Serializer &serializer) const override; }; - + struct DebuggerInfo : public LLDBBaseTelemetryInfo { std::string lldb_version; @@ -188,11 +184,11 @@ template <typename Info> struct ScopedDispatcher { void SetDebugger(Debugger *debugger) { debugger = debugger; } - void SetFinalCallback(llvm::unique_function<void(Info *info)> final_callback) { + void DispatchOnExit(llvm::unique_function<void(Info *info)> final_callback) { m_final_callback(std::move(final_callback)); } - void DispatchNowIfEnable(llvm::unique_function<void(Info *info)> populate_fields_cb) { + void DispatchNow(llvm::unique_function<void(Info *info)> populate_fields_cb) { TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled(); if (!manager) return; diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 6e069be26928a..4bd78c10e9704 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1887,17 +1887,16 @@ bool CommandInterpreter::HandleCommand(const char *command_line, bool force_repeat_command) { lldb_private::telemetry::ScopedDispatcher< lldb_private::telemetry:CommandInfo> helper(m_debugger); - lldb_private::telemetry::TelemetryManager *ins = lldb_private::telemetry::TelemetryManager::GetInstanceOrDummy(); + lldb_private::telemetry::TelemetryManager *ins = lldb_private::telemetry::TelemetryManager::GetInstance(); const int command_id = ins->MakeNextCommandId(); - std::string command_string(command_line); std::string original_command_string(command_string); std::string real_original_command_string(command_string); std::string parsed_command_args; CommandObject *cmd_obj = nullptr; - helper.DispatchIfEnable([&](lldb_private::telemetry:CommandInfo* info){ + helper.DispatchNow([&](lldb_private::telemetry:CommandInfo* info){ info.command_id = command_id; if (Target* target = GetExecutionContext().GetTargetPtr()) { // If we have a target attached to this command, then get the UUID. @@ -1915,7 +1914,6 @@ bool CommandInterpreter::HandleCommand(const char *command_line, // TODO: this is logging the time the command-handler finishes. // But we may want a finer-grain durations too? // (ie., the execute_time recorded below?) - info.command_id = command_id; llvm::StringRef command_name = cmd_obj ? cmd_obj->GetCommandName() : "<not found>"; >From 22d15d4137ed0c002ff2c8f8d6bb08cab434dfed Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Mon, 3 Mar 2025 11:06:44 -0500 Subject: [PATCH 04/23] renaming for clarity --- lldb/include/lldb/Core/Telemetry.h | 5 ++--- lldb/source/Core/Telemetry.cpp | 2 ++ lldb/source/Interpreter/CommandInterpreter.cpp | 7 ++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 42a62cae6ee37..67412c964190e 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -189,8 +189,8 @@ template <typename Info> struct ScopedDispatcher { } void DispatchNow(llvm::unique_function<void(Info *info)> populate_fields_cb) { - TelemetryManager *manager = TelemetryManager::GetInstanceIfEnabled(); - if (!manager) + TelemetryManager *manager = TelemetryManager::GetInstance(); + if (!manager->GetConfig()->EnableTelemetry) return; Info info; // Populate the common fields we know aboutl @@ -204,7 +204,6 @@ template <typename Info> struct ScopedDispatcher { LLDB_LOG_ERROR(GetLog(LLDBLog::Object), std::move(er), "Failed to dispatch entry of type: {0}", m_info.getKind()); } - } ~ScopedDispatcher() { diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 77b50778fa180..00005a6b19c58 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -65,6 +65,8 @@ void CommandInfo::serialize(Serializer &serializer) const { serializer.write("target_uuid", target_uuid); serializer.write("command_id", command_id); serializer.write("command_name", command_name); + serializer.write("original_command", original_command); + serializer.write("args", args); serializer.write("ret_status", ret_status); serializer.write("error_data", error_data); } diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 4bd78c10e9704..9bab0b3cadd60 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1887,7 +1887,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line, bool force_repeat_command) { lldb_private::telemetry::ScopedDispatcher< lldb_private::telemetry:CommandInfo> helper(m_debugger); - lldb_private::telemetry::TelemetryManager *ins = lldb_private::telemetry::TelemetryManager::GetInstance(); + lldb_private::telemetry::TelemetryManager *ins = + lldb_private::telemetry::TelemetryManager::GetInstance(); const int command_id = ins->MakeNextCommandId(); std::string command_string(command_line); @@ -1896,7 +1897,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, std::string parsed_command_args; CommandObject *cmd_obj = nullptr; - helper.DispatchNow([&](lldb_private::telemetry:CommandInfo* info){ + helper.DispatchNow([&](lldb_private::telemetry : CommandInfo *info) { info.command_id = command_id; if (Target* target = GetExecutionContext().GetTargetPtr()) { // If we have a target attached to this command, then get the UUID. @@ -1910,7 +1911,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, // Those will be collected by the on-exit-callback. }); - helper.DispatchOnExit([&](lldb_private::telemetry:CommandInfo* info) { + helper.DispatchOnExit([&](lldb_private::telemetry : CommandInfo *info) { // TODO: this is logging the time the command-handler finishes. // But we may want a finer-grain durations too? // (ie., the execute_time recorded below?) >From 291e923606bbf7fce128c0a4c48126881156c2e2 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Mon, 3 Mar 2025 11:11:04 -0500 Subject: [PATCH 05/23] remove empty line --- lldb/source/Core/Telemetry.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 00005a6b19c58..d7e2f2ac604a4 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -118,7 +118,6 @@ class NoOpTelemetryManager : final public TelemetryManager { } }; - std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr; TelemetryManager *TelemetryManager::GetInstance() { // If Telemetry is disabled or if there is no default instance, then use the NoOp manager. >From 2d7d713a24a31f71b97f12764f91979e3a85b49e Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Mon, 3 Mar 2025 11:35:09 -0500 Subject: [PATCH 06/23] compile error --- lldb/include/lldb/Core/Telemetry.h | 12 ++++--- .../source/Interpreter/CommandInterpreter.cpp | 32 ++++++++++--------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 67412c964190e..5bcc242ad663c 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -50,7 +50,7 @@ struct LLDBConfig : public ::llvm::telemetry::Config { // must have their LLDBEntryKind in the similar form (ie., share common prefix) struct LLDBEntryKind : public ::llvm::telemetry::EntryKind { static const llvm::telemetry::KindType BaseInfo = 0b11000000; - tatic const llvm::telemetry::KindType CommandInfo = 0b11010000; + static const llvm::telemetry::KindType CommandInfo = 0b11010000; static const llvm::telemetry::KindType DebuggerInfo = 0b11000100; }; @@ -109,7 +109,7 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { return (T->getKind() & LLDBEntryKind::CommandInfo) == LLDBEntryKind::CommandInfo; } - void serialize(Serializer &serializer) const override; + void serialize(llvm::telemetry::Serializer &serializer) const override; }; struct DebuggerInfo : public LLDBBaseTelemetryInfo { @@ -185,7 +185,9 @@ template <typename Info> struct ScopedDispatcher { void SetDebugger(Debugger *debugger) { debugger = debugger; } void DispatchOnExit(llvm::unique_function<void(Info *info)> final_callback) { - m_final_callback(std::move(final_callback)); + // We probably should not be overriding previously set cb. + assert(!m_final_callback); + m_final_callback = std::move(final_callback); } void DispatchNow(llvm::unique_function<void(Info *info)> populate_fields_cb) { @@ -202,13 +204,13 @@ template <typename Info> struct ScopedDispatcher { // And then we dispatch. if (llvm::Error er = manager->dispatch(&info)) { LLDB_LOG_ERROR(GetLog(LLDBLog::Object), std::move(er), - "Failed to dispatch entry of type: {0}", m_info.getKind()); + "Failed to dispatch entry of type: {0}", info.getKind()); } } ~ScopedDispatcher() { if (m_final_callback) - DispatchIfEnable(std::move(m_final_callback)); + DispatchNow(std::move(m_final_callback)); } private: diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 9bab0b3cadd60..07bfce8aa8eab 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1886,7 +1886,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line, CommandReturnObject &result, bool force_repeat_command) { lldb_private::telemetry::ScopedDispatcher< - lldb_private::telemetry:CommandInfo> helper(m_debugger); + lldb_private::telemetry::CommandInfo> + helper(m_debugger); lldb_private::telemetry::TelemetryManager *ins = lldb_private::telemetry::TelemetryManager::GetInstance(); const int command_id = ins->MakeNextCommandId(); @@ -1897,35 +1898,36 @@ bool CommandInterpreter::HandleCommand(const char *command_line, std::string parsed_command_args; CommandObject *cmd_obj = nullptr; - helper.DispatchNow([&](lldb_private::telemetry : CommandInfo *info) { - info.command_id = command_id; + helper.DispatchNow([&](lldb_private::telemetry::CommandInfo *info) { + info->command_id = command_id; if (Target* target = GetExecutionContext().GetTargetPtr()) { // If we have a target attached to this command, then get the UUID. - info.target_uuid = target->GetExecutableModule() != nullptr - ? GetExecutableModule()->GetUUID().GetAsString() - : ""; + info->target_uuid = + target->GetExecutableModule() != nullptr + ? target->GetExecutableModule()->GetUUID().GetAsString() + : ""; } - if (ins->GetConfig()->m_collect_original_command) - info.original_command = original_command_string; + if (ins->GetConfig()->m_detailed_command_telemetry) + info->original_command = original_command_string; // The rest (eg., command_name, args, etc) hasn't been parsed yet; // Those will be collected by the on-exit-callback. }); - helper.DispatchOnExit([&](lldb_private::telemetry : CommandInfo *info) { + helper.DispatchOnExit([&](lldb_private::telemetry::CommandInfo *info) { // TODO: this is logging the time the command-handler finishes. // But we may want a finer-grain durations too? // (ie., the execute_time recorded below?) - info.command_id = command_id; + info->command_id = command_id; llvm::StringRef command_name = cmd_obj ? cmd_obj->GetCommandName() : "<not found>"; - info.command_name = command_name.str(); - info.ret_status = result.GetStatus(); + info->command_name = command_name.str(); + info->ret_status = result.GetStatus(); if (llvm::StringRef error_data = result.GetErrorData(); !error_data.empty()) - info.error_data = error_data.str(); + info->error_data = error_data.str(); - if (ins->GetConfig()->m_collect_original_command) - info.args = parsed_command_args; + if (ins->GetConfig()->m_detailed_command_telemetry) + info->args = parsed_command_args; }); Log *log = GetLog(LLDBLog::Commands); >From f875c85fa7c23538a46daa5903f1afee5ec85fe2 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Mon, 3 Mar 2025 13:18:52 -0500 Subject: [PATCH 07/23] fixed test --- lldb/include/lldb/Core/Telemetry.h | 2 +- lldb/source/Core/Telemetry.cpp | 18 ++++++++++-------- lldb/source/Interpreter/CommandInterpreter.cpp | 8 ++++---- lldb/unittests/Core/TelemetryTest.cpp | 2 +- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 5bcc242ad663c..d5d285f6b2047 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -161,7 +161,7 @@ class TelemetryManager : public llvm::telemetry::Manager { // "start" and "end" entries can be matched up. // These values don't need to be unique across runs (because they are // secondary-key), hence a simple counter is sufficent. - std::atomic<int> command_id_seed = 0; + std::atomic<int> m_command_id_seed = 0; static std::unique_ptr<TelemetryManager> g_instance; }; diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index d7e2f2ac604a4..3c546e94711d0 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -60,7 +60,7 @@ void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const { serializer.write("end_time", ToNanosec(end_time.value())); } void CommandInfo::serialize(Serializer &serializer) const { - LLDBBaseTelemetryInfo::serializer(serializer); + LLDBBaseTelemetryInfo::serialize(serializer); serializer.write("target_uuid", target_uuid); serializer.write("command_id", command_id); @@ -78,7 +78,7 @@ void DebuggerInfo::serialize(Serializer &serializer) const { serializer.write("is_exit_entry", is_exit_entry); } -TelemetryManager::TelemetryManager(std::unique_ptr<Config> config) +TelemetryManager::TelemetryManager(std::unique_ptr<LLDBConfig> config) : m_config(std::move(config)), m_id(MakeUUID()) {} llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) { @@ -94,18 +94,20 @@ int TelemetryManager::MakeNextCommandId() { return m_command_id_seed.fetch_add(1); } -const LLDBConfig *TelemetryManager::GetConfig() { return m_config.get(); } - -class NoOpTelemetryManager : final public TelemetryManager { - public: +class NoOpTelemetryManager : public TelemetryManager { +public: llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override { // Does nothing. return llvm::Error::success(); } - explicit NoOpTelemetryManager() : TelemetryManager(std::make_unique<LLDBConfig(/*EnableTelemetry*/false, /*DetailedCommand*/false)>) {} + explicit NoOpTelemetryManager() + : TelemetryManager(std::make_unique<LLDBConfig>( + /*EnableTelemetry*/ false, /*DetailedCommand*/ false)) {} - virtual llvm::StringRef GetInstanceName() const { return "NoOpTelemetryManager"; } + virtual llvm::StringRef GetInstanceName() const override { + return "NoOpTelemetryManager"; + } llvm::Error dispatch(llvm::telemetry::TelemetryInfo *entry) override { // Does nothing. diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 07bfce8aa8eab..f8b455c12f300 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -46,6 +46,7 @@ #include "Commands/CommandObjectWatchpoint.h" #include "lldb/Core/Debugger.h" +#include "lldb/Core/Module.h" #include "lldb/Core/PluginManager.h" #include "lldb/Core/Telemetry.h" #include "lldb/Host/StreamFile.h" @@ -1887,7 +1888,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, bool force_repeat_command) { lldb_private::telemetry::ScopedDispatcher< lldb_private::telemetry::CommandInfo> - helper(m_debugger); + helper(&m_debugger); lldb_private::telemetry::TelemetryManager *ins = lldb_private::telemetry::TelemetryManager::GetInstance(); const int command_id = ins->MakeNextCommandId(); @@ -1922,9 +1923,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line, cmd_obj ? cmd_obj->GetCommandName() : "<not found>"; info->command_name = command_name.str(); info->ret_status = result.GetStatus(); - if (llvm::StringRef error_data = result.GetErrorData(); - !error_data.empty()) - info->error_data = error_data.str(); + if (std::string error_str = result.GetErrorString(); !error_str.empty()) + info->error_data = std::move(error_str); if (ins->GetConfig()->m_detailed_command_telemetry) info->args = parsed_command_args; diff --git a/lldb/unittests/Core/TelemetryTest.cpp b/lldb/unittests/Core/TelemetryTest.cpp index 0e9f329110872..065a57114181e 100644 --- a/lldb/unittests/Core/TelemetryTest.cpp +++ b/lldb/unittests/Core/TelemetryTest.cpp @@ -52,7 +52,7 @@ class FakePlugin : public telemetry::TelemetryManager { public: FakePlugin() : telemetry::TelemetryManager( - std::make_unique<llvm::telemetry::Config>(true)) {} + std::make_unique<telemetry::LLDBConfig>(true, true)) {} // TelemetryManager interface llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override { >From f025bdb5e40de32e2223a333e8cb96d06673265d Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Mon, 3 Mar 2025 13:24:43 -0500 Subject: [PATCH 08/23] formatting --- lldb/include/lldb/Core/Telemetry.h | 30 ++++++++++--------- lldb/source/Core/Telemetry.cpp | 10 ++++--- .../source/Interpreter/CommandInterpreter.cpp | 2 +- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index d5d285f6b2047..bec75c8f57b9d 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -13,12 +13,10 @@ #include "lldb/Interpreter/CommandReturnObject.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/StructuredData.h" -#include "lldb/Utility/LLDBLog.h" #include "lldb/lldb-forward.h" #include "llvm/ADT/FunctionExtras.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" -#include "llvm/ADT/FunctionExtras.h" #include "llvm/Support/JSON.h" #include "llvm/Telemetry/Telemetry.h" #include <atomic> @@ -31,14 +29,15 @@ namespace lldb_private { namespace telemetry { - struct LLDBConfig : public ::llvm::telemetry::Config { - // If true, we will collect full details about a debug command (eg., args and original command). - // Note: This may contain PII, hence can only be enabled by the vendor while creating the Manager. + // If true, we will collect full details about a debug command (eg., args and + // original command). Note: This may contain PII, hence can only be enabled by + // the vendor while creating the Manager. const bool m_detailed_command_telemetry; explicit LLDBConfig(bool enable_telemetry, bool detailed_command_telemetry) - : ::llvm::telemetry::Config(enable_telemetry), m_detailed_command_telemetry(detailed_command_telemetry) {} + : ::llvm::telemetry::Config(enable_telemetry), + m_detailed_command_telemetry(detailed_command_telemetry) {} }; // We expect each (direct) subclass of LLDBTelemetryInfo to @@ -94,7 +93,8 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { // Eg., "breakpoint set" std::string command_name; // !!NOTE!! These two fields are not collected by default due to PII risks. - // Vendor may allow them by setting the LLDBConfig::m_detailed_command_telemetry. + // Vendor may allow them by setting the + // LLDBConfig::m_detailed_command_telemetry. std::string original_command; std::string args; // Return status of a command and any error description in case of error. @@ -103,14 +103,17 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { CommandInfo() = default; - llvm::telemetry::KindType getKind() const override { return LLDBEntryKind::CommandInfo; } + llvm::telemetry::KindType getKind() const override { + return LLDBEntryKind::CommandInfo; + } static bool classof(const llvm::telemetry::TelemetryInfo *T) { - return (T->getKind() & LLDBEntryKind::CommandInfo) == LLDBEntryKind::CommandInfo; + return (T->getKind() & LLDBEntryKind::CommandInfo) == + LLDBEntryKind::CommandInfo; } void serialize(llvm::telemetry::Serializer &serializer) const override; - }; +}; struct DebuggerInfo : public LLDBBaseTelemetryInfo { std::string lldb_version; @@ -142,7 +145,7 @@ class TelemetryManager : public llvm::telemetry::Manager { /// Returns the next unique ID to assign to a command entry. int MakeNextCommandId(); - const LLDBConfig* GetConfig() { return m_config.get(); } + const LLDBConfig *GetConfig() { return m_config.get(); } virtual llvm::StringRef GetInstanceName() const = 0; @@ -155,7 +158,7 @@ class TelemetryManager : public llvm::telemetry::Manager { private: std::unique_ptr<LLDBConfig> m_config; - // Each instance of a TelemetryManager is assigned a unique ID. + // Each instance of a TelemetryManager is assigned a unique ID. const std::string m_id; // We assign each command (in the same session) a unique id so that their // "start" and "end" entries can be matched up. @@ -216,8 +219,7 @@ template <typename Info> struct ScopedDispatcher { private: SteadyTimePoint m_start_time; llvm::unique_function<void(Info *info)> m_final_callback; - Debugger * debugger; - + Debugger *debugger; }; } // namespace telemetry diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 3c546e94711d0..8c601d3677b4a 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -111,19 +111,21 @@ class NoOpTelemetryManager : public TelemetryManager { llvm::Error dispatch(llvm::telemetry::TelemetryInfo *entry) override { // Does nothing. - return llvm::Error::success(); + return llvm::Error::success(); } static NoOpTelemetryManager *GetInstance() { - static std::unique_ptr<NoOpTelemetryManager> g_ins = std::make_unique<NoOpTelemetryManager>(); + static std::unique_ptr<NoOpTelemetryManager> g_ins = + std::make_unique<NoOpTelemetryManager>(); return g_ins.get(); } }; std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr; TelemetryManager *TelemetryManager::GetInstance() { - // If Telemetry is disabled or if there is no default instance, then use the NoOp manager. - // We use a dummy instance to avoid having to do nullchecks in various places. + // If Telemetry is disabled or if there is no default instance, then use the + // NoOp manager. We use a dummy instance to avoid having to do nullchecks in + // various places. if (!Config::BuildTimeEnableTelemetry || !g_instance) return NoOpTelemetryManager::GetInstance(); return g_instance.get(); diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index f8b455c12f300..d728f3e978285 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1901,7 +1901,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, helper.DispatchNow([&](lldb_private::telemetry::CommandInfo *info) { info->command_id = command_id; - if (Target* target = GetExecutionContext().GetTargetPtr()) { + if (Target *target = GetExecutionContext().GetTargetPtr()) { // If we have a target attached to this command, then get the UUID. info->target_uuid = target->GetExecutableModule() != nullptr >From b859e2d871e66ef7fc53d20c3a42dc99b2c9732f Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 09:59:52 -0500 Subject: [PATCH 09/23] qual --- lldb/include/lldb/Core/Telemetry.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index bec75c8f57b9d..19d2b72241049 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -175,17 +175,17 @@ template <typename Info> struct ScopedDispatcher { ScopedDispatcher(Debugger *debugger = nullptr) { // Start the timer. m_start_time = std::chrono::steady_clock::now(); - debugger = debugger; + this->debugger = debugger; } ScopedDispatcher(llvm::unique_function<void(Info *info)> final_callback, Debugger *debugger = nullptr) : m_final_callback(std::move(final_callback)) { // Start the timer. m_start_time = std::chrono::steady_clock::now(); - debugger = debugger; + this->debugger = debugger; } - void SetDebugger(Debugger *debugger) { debugger = debugger; } + void SetDebugger(Debugger *debugger) { this->debugger = debugger; } void DispatchOnExit(llvm::unique_function<void(Info *info)> final_callback) { // We probably should not be overriding previously set cb. >From 017be757ca436d3ff9ebf958378c8e4c85d93f5d Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 15:46:49 -0500 Subject: [PATCH 10/23] Update lldb/unittests/Core/TelemetryTest.cpp Co-authored-by: Jonas Devlieghere <jo...@devlieghere.com> --- lldb/unittests/Core/TelemetryTest.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/unittests/Core/TelemetryTest.cpp b/lldb/unittests/Core/TelemetryTest.cpp index 065a57114181e..0522abd400626 100644 --- a/lldb/unittests/Core/TelemetryTest.cpp +++ b/lldb/unittests/Core/TelemetryTest.cpp @@ -52,7 +52,7 @@ class FakePlugin : public telemetry::TelemetryManager { public: FakePlugin() : telemetry::TelemetryManager( - std::make_unique<telemetry::LLDBConfig>(true, true)) {} + std::make_unique<telemetry::LLDBConfig>(/*enable_telemetry=*/true, /*detailed_command_telemetry=*/true)) {} // TelemetryManager interface llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override { >From 9d6997b1483e27f2111b288e406e4d0cb18a05d0 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 15:47:04 -0500 Subject: [PATCH 11/23] Update lldb/source/Interpreter/CommandInterpreter.cpp Co-authored-by: Jonas Devlieghere <jo...@devlieghere.com> --- lldb/source/Interpreter/CommandInterpreter.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index d728f3e978285..851b915b86941 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1886,11 +1886,11 @@ bool CommandInterpreter::HandleCommand(const char *command_line, LazyBool lazy_add_to_history, CommandReturnObject &result, bool force_repeat_command) { - lldb_private::telemetry::ScopedDispatcher< - lldb_private::telemetry::CommandInfo> + telemetry::ScopedDispatcher< + telemetry::CommandInfo> helper(&m_debugger); - lldb_private::telemetry::TelemetryManager *ins = - lldb_private::telemetry::TelemetryManager::GetInstance(); + telemetry::TelemetryManager *ins = + telemetry::TelemetryManager::GetInstance(); const int command_id = ins->MakeNextCommandId(); std::string command_string(command_line); >From 714d8038821778c0b09dfb46560d12dfc51b0a5c Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 15:47:51 -0500 Subject: [PATCH 12/23] Update lldb/include/lldb/Core/Telemetry.h Co-authored-by: Jonas Devlieghere <jo...@devlieghere.com> --- lldb/include/lldb/Core/Telemetry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 19d2b72241049..6e7294fd86c0b 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -83,7 +83,7 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { // If the command is/can be associated with a target entry this field contains // that target's UUID. <EMPTY> otherwise. - std::string target_uuid; + UUID target_uuid; // A unique ID for a command so the manager can match the start entry with // its end entry. These values only need to be unique within the same session. // Necessary because we'd send off an entry right before a command's execution >From 481e1413f93a28b8ad2c9f2987946165a610dfc7 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 15:48:04 -0500 Subject: [PATCH 13/23] Update lldb/include/lldb/Core/Telemetry.h Co-authored-by: Jonas Devlieghere <jo...@devlieghere.com> --- lldb/include/lldb/Core/Telemetry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 6e7294fd86c0b..bf4778846b8bb 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -89,7 +89,7 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { // 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. - int command_id; + uint64_t command_id; // Eg., "breakpoint set" std::string command_name; // !!NOTE!! These two fields are not collected by default due to PII risks. >From ec598c6525a3c3b77ffe01fac39a8f14b6fa0040 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 16:02:35 -0500 Subject: [PATCH 14/23] addressed review commeht --- lldb/include/lldb/Core/Telemetry.h | 16 ++++++++++------ lldb/source/Core/Telemetry.cpp | 7 +++---- lldb/source/Interpreter/CommandInterpreter.cpp | 2 +- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index bf4778846b8bb..03bf718837397 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -113,6 +113,15 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { } void serialize(llvm::telemetry::Serializer &serializer) const override; + + static uint64_t GetNextId() {} + +private: + // We assign each command (in the same session) a unique id so that their + // "start" and "end" entries can be matched up. + // These values don't need to be unique across runs (because they are + // secondary-key), hence a simple counter is sufficent. + static std::atomic<uint64_t> g_command_id_seed; }; struct DebuggerInfo : public LLDBBaseTelemetryInfo { @@ -160,11 +169,6 @@ class TelemetryManager : public llvm::telemetry::Manager { std::unique_ptr<LLDBConfig> m_config; // Each instance of a TelemetryManager is assigned a unique ID. const std::string m_id; - // We assign each command (in the same session) a unique id so that their - // "start" and "end" entries can be matched up. - // These values don't need to be unique across runs (because they are - // secondary-key), hence a simple counter is sufficent. - std::atomic<int> m_command_id_seed = 0; static std::unique_ptr<TelemetryManager> g_instance; }; @@ -198,7 +202,7 @@ template <typename Info> struct ScopedDispatcher { if (!manager->GetConfig()->EnableTelemetry) return; Info info; - // Populate the common fields we know aboutl + // Populate the common fields we know about. info.start_time = m_start_time; info.end_time = std::chrono::steady_clock::now(); info.debugger = debugger; diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 8c601d3677b4a..52b7e32450805 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -78,6 +78,9 @@ 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 DebuggerInfo::GetNextId() { return g_command_id_seed.fetch_add(1); } + TelemetryManager::TelemetryManager(std::unique_ptr<LLDBConfig> config) : m_config(std::move(config)), m_id(MakeUUID()) {} @@ -90,10 +93,6 @@ llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) { return llvm::Error::success(); } -int TelemetryManager::MakeNextCommandId() { - return m_command_id_seed.fetch_add(1); -} - class NoOpTelemetryManager : public TelemetryManager { public: llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override { diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 851b915b86941..58d6716dbb987 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1891,7 +1891,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, helper(&m_debugger); telemetry::TelemetryManager *ins = telemetry::TelemetryManager::GetInstance(); - const int command_id = ins->MakeNextCommandId(); + const int command_id = telemetry::CommandInfo::GetNextId(); std::string command_string(command_line); std::string original_command_string(command_string); >From 46225e8701348fecc55bea05603335a33a0120c2 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 20:48:24 -0500 Subject: [PATCH 15/23] formatting --- lldb/include/lldb/Core/Telemetry.h | 2 +- lldb/source/Core/Telemetry.cpp | 5 +++-- lldb/source/Interpreter/CommandInterpreter.cpp | 7 +++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 03bf718837397..c400a79aa217b 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -13,6 +13,7 @@ #include "lldb/Interpreter/CommandReturnObject.h" #include "lldb/Utility/LLDBLog.h" #include "lldb/Utility/StructuredData.h" +#include "lldb/Utility/UUID.h" #include "lldb/lldb-forward.h" #include "llvm/ADT/FunctionExtras.h" #include "llvm/ADT/StringExtras.h" @@ -80,7 +81,6 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { }; struct CommandInfo : public LLDBBaseTelemetryInfo { - // If the command is/can be associated with a target entry this field contains // that target's UUID. <EMPTY> otherwise. UUID target_uuid; diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 52b7e32450805..856ed2ac72db3 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -59,10 +59,11 @@ void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const { if (end_time.has_value()) serializer.write("end_time", ToNanosec(end_time.value())); } + void CommandInfo::serialize(Serializer &serializer) const { LLDBBaseTelemetryInfo::serialize(serializer); - serializer.write("target_uuid", target_uuid); + serializer.write("target_uuid", target_uuid.GetAsString()); serializer.write("command_id", command_id); serializer.write("command_name", command_name); serializer.write("original_command", original_command); @@ -79,7 +80,7 @@ void DebuggerInfo::serialize(Serializer &serializer) const { } std::atomic<uint64_t> CommandInfo::g_command_id_seed = 0; -uint64_t DebuggerInfo::GetNextId() { return g_command_id_seed.fetch_add(1); } +uint64_t CommandInfo::GetNextId() { return g_command_id_seed.fetch_add(1); } 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 58d6716dbb987..2e4c664a0e675 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1903,10 +1903,9 @@ bool CommandInterpreter::HandleCommand(const char *command_line, info->command_id = command_id; if (Target *target = GetExecutionContext().GetTargetPtr()) { // If we have a target attached to this command, then get the UUID. - info->target_uuid = - target->GetExecutableModule() != nullptr - ? target->GetExecutableModule()->GetUUID().GetAsString() - : ""; + info->target_uuid = target->GetExecutableModule() != nullptr + ? target->GetExecutableModule()->GetUUID() + : UUID(); } if (ins->GetConfig()->m_detailed_command_telemetry) info->original_command = original_command_string; >From a5c46a1a3ae09e5bf898ea2145ad5ba90864dd5f Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Tue, 4 Mar 2025 20:53:08 -0500 Subject: [PATCH 16/23] formatting again --- lldb/source/Interpreter/CommandInterpreter.cpp | 7 ++----- lldb/unittests/Core/TelemetryTest.cpp | 4 ++-- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 2e4c664a0e675..7d2d5bbdbc773 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1886,11 +1886,8 @@ bool CommandInterpreter::HandleCommand(const char *command_line, LazyBool lazy_add_to_history, CommandReturnObject &result, bool force_repeat_command) { - telemetry::ScopedDispatcher< - telemetry::CommandInfo> - helper(&m_debugger); - telemetry::TelemetryManager *ins = - telemetry::TelemetryManager::GetInstance(); + telemetry::ScopedDispatcher<telemetry::CommandInfo> helper(&m_debugger); + telemetry::TelemetryManager *ins = telemetry::TelemetryManager::GetInstance(); const int command_id = telemetry::CommandInfo::GetNextId(); std::string command_string(command_line); diff --git a/lldb/unittests/Core/TelemetryTest.cpp b/lldb/unittests/Core/TelemetryTest.cpp index 0522abd400626..aac3032324461 100644 --- a/lldb/unittests/Core/TelemetryTest.cpp +++ b/lldb/unittests/Core/TelemetryTest.cpp @@ -51,8 +51,8 @@ class TestDestination : public llvm::telemetry::Destination { class FakePlugin : public telemetry::TelemetryManager { public: FakePlugin() - : telemetry::TelemetryManager( - std::make_unique<telemetry::LLDBConfig>(/*enable_telemetry=*/true, /*detailed_command_telemetry=*/true)) {} + : telemetry::TelemetryManager(std::make_unique<telemetry::LLDBConfig>( + /*enable_telemetry=*/true, /*detailed_command_telemetry=*/true)) {} // TelemetryManager interface llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override { >From ac03cb0d67895e61606424c9c5f5a0f54a8dbb53 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Wed, 5 Mar 2025 09:59:49 -0500 Subject: [PATCH 17/23] fix dup definition --- lldb/include/lldb/Core/Telemetry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index c400a79aa217b..e9c92734a5e85 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -114,7 +114,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 >From 00bf71157b2890a319b00160dc32232c9095e088 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Wed, 5 Mar 2025 10:10:10 -0500 Subject: [PATCH 18/23] simplified --- lldb/include/lldb/Core/Telemetry.h | 4 ++-- lldb/source/Core/Telemetry.cpp | 6 ++++-- lldb/source/Interpreter/CommandInterpreter.cpp | 9 ++++++--- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index e9c92734a5e85..fc9512475f2f4 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -98,8 +98,8 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { std::string original_command; std::string args; // Return status of a command and any error description in case of error. - lldb::ReturnStatus ret_status; - std::string error_data; + std::optional<lldb::ReturnStatus> ret_status; + std::optional<std::string> error_data; CommandInfo() = default; diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 856ed2ac72db3..39d1c2bc1f1e5 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -68,8 +68,10 @@ void CommandInfo::serialize(Serializer &serializer) const { serializer.write("command_name", command_name); serializer.write("original_command", original_command); serializer.write("args", args); - serializer.write("ret_status", ret_status); - serializer.write("error_data", error_data); + if (ret_status.has_value()) + serializer.write("ret_status", ret_status.value()); + if (error_data.has_value()) + serializer.write("error_data", error_data.value()); } void DebuggerInfo::serialize(Serializer &serializer) const { diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index 7d2d5bbdbc773..a7834c46ffd37 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1887,7 +1887,10 @@ bool CommandInterpreter::HandleCommand(const char *command_line, CommandReturnObject &result, bool force_repeat_command) { telemetry::ScopedDispatcher<telemetry::CommandInfo> helper(&m_debugger); - telemetry::TelemetryManager *ins = telemetry::TelemetryManager::GetInstance(); + const bool detailed_command_telemetry = + telemetry::TelemetryManager::GetInstance() + ->GetConfig() + ->m_detailed_command_telemetry; const int command_id = telemetry::CommandInfo::GetNextId(); std::string command_string(command_line); @@ -1904,7 +1907,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, ? target->GetExecutableModule()->GetUUID() : UUID(); } - if (ins->GetConfig()->m_detailed_command_telemetry) + if (detailed_command_telemetry) info->original_command = original_command_string; // The rest (eg., command_name, args, etc) hasn't been parsed yet; // Those will be collected by the on-exit-callback. @@ -1922,7 +1925,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, if (std::string error_str = result.GetErrorString(); !error_str.empty()) info->error_data = std::move(error_str); - if (ins->GetConfig()->m_detailed_command_telemetry) + if (detailed_command_telemetry) info->args = parsed_command_args; }); >From 031ee14661e8f47dc7596ae98f545c07f34cdd03 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Wed, 5 Mar 2025 12:10:51 -0500 Subject: [PATCH 19/23] addressed review comments --- lldb/include/lldb/Core/Telemetry.h | 28 ++++++++++--------- .../source/Interpreter/CommandInterpreter.cpp | 2 +- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index fc9512475f2f4..bf4a223737571 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -34,7 +34,7 @@ struct LLDBConfig : public ::llvm::telemetry::Config { // If true, we will collect full details about a debug command (eg., args and // original command). Note: This may contain PII, hence can only be enabled by // the vendor while creating the Manager. - const bool m_detailed_command_telemetry; + const bool detailed_command_telemetry; explicit LLDBConfig(bool enable_telemetry, bool detailed_command_telemetry) : ::llvm::telemetry::Config(enable_telemetry), @@ -81,23 +81,25 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo { }; struct CommandInfo : public LLDBBaseTelemetryInfo { - // If the command is/can be associated with a target entry this field contains - // that target's UUID. <EMPTY> otherwise. + /// If the command is/can be associated with a target entry this field + /// contains that target's UUID. <EMPTY> otherwise. UUID target_uuid; - // A unique ID for a command so the manager can match the start entry with - // its end entry. These values only need to be unique within the same 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. + /// A unique ID for a command so the manager can match the start entry with + /// its end entry. These values only need to be unique within the same + /// 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; - // Eg., "breakpoint set" + /// The command name(eg., "breakpoint set") std::string command_name; - // !!NOTE!! These two fields are not collected by default due to PII risks. - // Vendor may allow them by setting the - // LLDBConfig::m_detailed_command_telemetry. + /// These two fields are not collected by default due to PII risks. + /// Vendor may allow them by setting the + /// LLDBConfig::detailed_command_telemetry. + /// @{ std::string original_command; std::string args; - // Return status of a command and any error description in case of error. + /// @} + /// Return status of a command and any error description in case of error. std::optional<lldb::ReturnStatus> ret_status; std::optional<std::string> error_data; diff --git a/lldb/source/Interpreter/CommandInterpreter.cpp b/lldb/source/Interpreter/CommandInterpreter.cpp index a7834c46ffd37..24cf755d1f740 100644 --- a/lldb/source/Interpreter/CommandInterpreter.cpp +++ b/lldb/source/Interpreter/CommandInterpreter.cpp @@ -1890,7 +1890,7 @@ bool CommandInterpreter::HandleCommand(const char *command_line, const bool detailed_command_telemetry = telemetry::TelemetryManager::GetInstance() ->GetConfig() - ->m_detailed_command_telemetry; + ->detailed_command_telemetry; const int command_id = telemetry::CommandInfo::GetNextId(); std::string command_string(command_line); >From 9a0d4cbd4d4befdd0b3f38d5223db1bcf70c1666 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Wed, 5 Mar 2025 12:26:27 -0500 Subject: [PATCH 20/23] remove unused --- lldb/include/lldb/Core/Telemetry.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index bf4a223737571..16e04fbff7857 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -153,9 +153,6 @@ class TelemetryManager : public llvm::telemetry::Manager { public: llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override; - /// Returns the next unique ID to assign to a command entry. - int MakeNextCommandId(); - const LLDBConfig *GetConfig() { return m_config.get(); } virtual llvm::StringRef GetInstanceName() const = 0; >From f55ea59c4b9309a5d4fd2400331006c4eb5cc640 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Wed, 5 Mar 2025 13:41:31 -0500 Subject: [PATCH 21/23] fix build failure --- lldb/include/lldb/Core/Telemetry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 16e04fbff7857..4f9d0aa47f452 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -38,7 +38,7 @@ struct LLDBConfig : public ::llvm::telemetry::Config { explicit LLDBConfig(bool enable_telemetry, bool detailed_command_telemetry) : ::llvm::telemetry::Config(enable_telemetry), - m_detailed_command_telemetry(detailed_command_telemetry) {} + detailed_command_telemetry(detailed_command_telemetry) {} }; // We expect each (direct) subclass of LLDBTelemetryInfo to >From ebbda8db26a39413645f8c9207a9f1d78263216a Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Thu, 6 Mar 2025 20:52:45 -0500 Subject: [PATCH 22/23] make fields optional --- lldb/include/lldb/Core/Telemetry.h | 4 ++-- lldb/source/Core/Telemetry.cpp | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h index 4f9d0aa47f452..56259e4a673bf 100644 --- a/lldb/include/lldb/Core/Telemetry.h +++ b/lldb/include/lldb/Core/Telemetry.h @@ -96,8 +96,8 @@ struct CommandInfo : public LLDBBaseTelemetryInfo { /// Vendor may allow them by setting the /// LLDBConfig::detailed_command_telemetry. /// @{ - std::string original_command; - std::string args; + std::optional<std::string> original_command; + std::optional<std::string> args; /// @} /// Return status of a command and any error description in case of error. std::optional<lldb::ReturnStatus> ret_status; diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 39d1c2bc1f1e5..8c4c87f1f8560 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -66,8 +66,10 @@ void CommandInfo::serialize(Serializer &serializer) const { serializer.write("target_uuid", target_uuid.GetAsString()); serializer.write("command_id", command_id); serializer.write("command_name", command_name); - serializer.write("original_command", original_command); - serializer.write("args", args); + if (original_command.has_value()) + serializer.write("original_command", original_command); + if (args.has_value()) + serializer.write("args", args); if (ret_status.has_value()) serializer.write("ret_status", ret_status.value()); if (error_data.has_value()) >From 42e7a62c5344881c8572e28ffebaa6ffd206a3a1 Mon Sep 17 00:00:00 2001 From: Vy Nguyen <v...@google.com> Date: Thu, 6 Mar 2025 21:13:15 -0500 Subject: [PATCH 23/23] build --- lldb/source/Core/Telemetry.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp index 8c4c87f1f8560..be929a644706e 100644 --- a/lldb/source/Core/Telemetry.cpp +++ b/lldb/source/Core/Telemetry.cpp @@ -67,9 +67,9 @@ void CommandInfo::serialize(Serializer &serializer) const { serializer.write("command_id", command_id); serializer.write("command_name", command_name); if (original_command.has_value()) - serializer.write("original_command", original_command); + serializer.write("original_command", original_command.value()); if (args.has_value()) - serializer.write("args", args); + serializer.write("args", args.value()); if (ret_status.has_value()) serializer.write("ret_status", ret_status.value()); if (error_data.has_value()) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits