[LLDB][Telemetry]Define TargetInfo for collecting data about a target #127834
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Vy Nguyen (oontvoo) ChangesFull diff: https://github.com/llvm/llvm-project/pull/127834.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Core/Telemetry.h b/lldb/include/lldb/Core/Telemetry.h
index b72556ecaf3c9..4be81951254de 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/StructuredData.h"
#include "lldb/lldb-forward.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/JSON.h"
@@ -29,6 +30,9 @@ namespace telemetry {
struct LLDBEntryKind : public ::llvm::telemetry::EntryKind {
static const llvm::telemetry::KindType BaseInfo = 0b11000;
+ static const KindType TargetInfo = 0b11010;
+ // There are other entries in between (added in separate PRs)
+ static const llvm::telemetry::KindType MiscInfo = 0b11110;
};
/// Defines a convenient type for timestamp of various events.
@@ -56,14 +60,88 @@ struct LLDBBaseTelemetryInfo : public llvm::telemetry::TelemetryInfo {
void serialize(llvm::telemetry::Serializer &serializer) const override;
};
+/// Describes an exit status.
+struct ExitDescription {
+ int exit_code;
+ std::string description;
+};
+
+struct TargetTelemetryInfo : public LldbBaseTelemetryInfo {
+ lldb::ModuleSP exec_mod;
+ Target *target_ptr;
+
+ // The same as the executable-module's UUID.
+ std::string target_uuid;
+ std::string file_format;
+
+ std::string binary_path;
+ size_t binary_size;
+
+ std::optional<ExitDescription> exit_desc;
+ TargetTelemetryInfo() = default;
+
+ TargetTelemetryInfo(const TargetTelemetryInfo &other) {
+ exec_mod = other.exec_mod;
+ target_uuid = other.target_uuid;
+ file_format = other.file_format;
+ binary_path = other.binary_path;
+ binary_size = other.binary_size;
+ exit_desc = other.exit_desc;
+ }
+
+ KindType getKind() const override { return LldbEntryKind::TargetInfo; }
+
+ static bool classof(const TelemetryInfo *T) {
+ if (T == nullptr)
+ return false;
+ return T->getKind() == LldbEntryKind::TargetInfo;
+ }
+
+ void serialize(Serializer &serializer) const override;
+};
+
+/// The "catch-all" entry to store a set of non-standard data, such as
+/// error-messages, etc.
+struct MiscTelemetryInfo : public LLDBBaseTelemetryInfo {
+ /// If the event is/can be associated with a target entry,
+ /// this field contains that target's UUID.
+ /// <EMPTY> otherwise.
+ std::string target_uuid;
+
+ /// Set of key-value pairs for any optional (or impl-specific) data
+ std::map<std::string, std::string> meta_data;
+
+ MiscTelemetryInfo() = default;
+
+ MiscTelemetryInfo(const MiscTelemetryInfo &other) {
+ target_uuid = other.target_uuid;
+ meta_data = other.meta_data;
+ }
+
+ llvm::telemetry::KindType getKind() const override {
+ return LLDBEntryKind::MiscInfo;
+ }
+
+ static bool classof(const llvm::telemetry::TelemetryInfo *T) {
+ return T->getKind() == LLDBEntryKind::MiscInfo;
+ }
+
+ void serialize(llvm::telemetry::Serializer &serializer) const override;
+};
+
/// The base Telemetry manager instance in LLDB.
/// This class declares additional instrumentation points
/// applicable to LLDB.
-class TelemetryManager : public llvm::telemetry::Manager {
+class TelemetryMager : public llvm::telemetry::Manager {
public:
llvm::Error preDispatch(llvm::telemetry::TelemetryInfo *entry) override;
- virtual llvm::StringRef GetInstanceName() const = 0;
+ const llvm::telemetry::Config *getConfig();
+
+ virtual void AtMainExecutableLoadStart(TargetInfo * entry);
+ virtual void AtMainExecutableLoadEnd(TargetInfo *entry);
+
+ virtual llvm::StringRef GetInstanceName() const = 0;
static TelemetryManager *getInstance();
protected:
@@ -73,6 +151,10 @@ class TelemetryManager : public llvm::telemetry::Manager {
private:
std::unique_ptr<llvm::telemetry::Config> m_config;
+ // Each debugger is assigned a unique ID (session_id).
+ // All TelemetryInfo entries emitted for the same debugger instance
+ // will get the same session_id.
+ llvm::DenseMap<Debugger *, std::string> session_ids;
static std::unique_ptr<TelemetryManager> g_instance;
};
diff --git a/lldb/source/Core/Telemetry.cpp b/lldb/source/Core/Telemetry.cpp
index 5222f76704f91..da7aee01680fc 100644
--- a/lldb/source/Core/Telemetry.cpp
+++ b/lldb/source/Core/Telemetry.cpp
@@ -10,14 +10,20 @@
#ifdef LLVM_BUILD_TELEMETRY
-#include "lldb/Core/Telemetry.h"
#include "lldb/Core/Debugger.h"
+#include "lldb/Core/Telemetry.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/Statistics.h"
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/UUID.h"
+#include "lldb/Version/Version.h"
#include "lldb/lldb-enumerations.h"
#include "lldb/lldb-forward.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/RandomNumberGenerator.h"
#include "llvm/Telemetry/Telemetry.h"
#include <chrono>
@@ -35,15 +41,7 @@ static uint64_t ToNanosec(const SteadyTimePoint Point) {
return std::chrono::nanoseconds(Point.time_since_epoch()).count();
}
-void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const {
- serializer.write("entry_kind", getKind());
- serializer.write("session_id", SessionId);
- serializer.write("start_time", ToNanosec(start_time));
- if (end_time.has_value())
- serializer.write("end_time", ToNanosec(end_time.value()));
-}
-
-[[maybe_unused]] static std::string MakeUUID(Debugger *debugger) {
+static std::string MakeUUID(Debugger *debugger) {
uint8_t random_bytes[16];
if (auto ec = llvm::getRandomBytes(random_bytes, 16)) {
LLDB_LOG(GetLog(LLDBLog::Object),
@@ -56,16 +54,91 @@ void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const {
return UUID(random_bytes).GetAsString();
}
+void LLDBBaseTelemetryInfo::serialize(Serializer &serializer) const {
+ serializer.write("entry_kind", getKind());
+ serializer.write("session_id", SessionId);
+ serializer.write("start_time", ToNanosec(start_time));
+ if (end_time.has_value())
+ serializer.write("end_time", ToNanosec(end_time.value()));
+}
+
+void TargetInfo::serialize(Serializer &serializer) const {
+ LLDBBaseTelemetryInfo::serialize(serializer);
+
+ serializer.write("username", username);
+ serializer.write("lldb_git_sha", lldb_git_sha);
+ serializer.write("lldb_path", lldb_path);
+ serializer.write("cwd", cwd);
+ if (exit_desc.has_value()) {
+ serializer.write("exit_code", exit_desc->exit_code);
+ serializer.write("exit_desc", exit_desc->description);
+ }
+}
+
+void MiscTelemetryInfo::serialize(Serializer &serializer) const {
+ LLDBBaseTelemetryInfo::serialize(serializer);
+ serializer.write("target_uuid", target_uuid);
+ serializer.beginObject("meta_data");
+ for (const auto &kv : meta_data)
+ serializer.write(kv.first, kv.second);
+ serializer.endObject();
+}
+
TelemetryManager::TelemetryManager(std::unique_ptr<Config> config)
: m_config(std::move(config)) {}
llvm::Error TelemetryManager::preDispatch(TelemetryInfo *entry) {
- // Do nothing for now.
- // In up-coming patch, this would be where the manager
- // attach the session_uuid to the entry.
+ LLDBBaseTelemetryInfo *lldb_entry =
+ llvm::dyn_cast<LLDBBaseTelemetryInfo>(entry);
+ std::string session_id = "";
+ if (Debugger *debugger = lldb_entry->debugger) {
+ auto session_id_pos = session_ids.find(debugger);
+ if (session_id_pos != session_ids.end())
+ session_id = session_id_pos->second;
+ else
+ session_id_pos->second = session_id = MakeUUID(debugger);
+ }
+ lldb_entry->SessionId = session_id;
+
return llvm::Error::success();
}
+const Config *getConfig() { return m_config.get(); }
+
+void TelemetryManager::AtMainExecutableLoadStart(TargetInfo *entry) {
+ UserIDResolver &resolver = lldb_private::HostInfo::GetUserIDResolver();
+ std::optional<llvm::StringRef> opt_username =
+ resolver.GetUserName(lldb_private::HostInfo::GetUserID());
+ if (opt_username)
+ entry->username = *opt_username;
+
+ entry->lldb_git_sha =
+ lldb_private::GetVersion(); // TODO: find the real git sha?
+
+ entry->lldb_path = HostInfo::GetProgramFileSpec().GetPath();
+
+ llvm::SmallString<64> cwd;
+ if (!llvm::sys::fs::current_path(cwd)) {
+ entry->cwd = cwd.c_str();
+ } else {
+ MiscTelemetryInfo misc_info;
+ misc_info.meta_data["internal_errors"] = "Cannot determine CWD";
+ if (auto er = dispatch(&misc_info)) {
+ LLDB_LOG(GetLog(LLDBLog::Object),
+ "Failed to dispatch misc-info at startup");
+ }
+ }
+
+ if (auto er = dispatch(entry)) {
+ LLDB_LOG(GetLog(LLDBLog::Object), "Failed to dispatch entry at startup");
+ }
+}
+
+void TelemetryManager::AtMainExecutableLoadEnd(TargetInfo *entry) {
+ // ....
+ dispatch(entry);
+}
+
std::unique_ptr<TelemetryManager> TelemetryManager::g_instance = nullptr;
TelemetryManager *TelemetryManager::getInstance() { return g_instance.get(); }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
(This PR assumes that we can rely on PR/128534 - that is, we always build the telemetry library. Whether it is enabled is dictated by Config::EnableTelemetry) |
|
Hi @labath @JDevlieghere , just FYI that both this PR and PR/129354 have common change (which is modifying the |
Okay. So far, so good.
This is where it gets tricky. I know you want that, but I don't think that all you want (if it was, you may not even need this patch since it's kind of implicitly measured by the command interpreter telemetry). I think you also want to handle cases where the target is created through SB API. If that was all you wanted, then the best place to instrument might be something like This is why I suggested way back to focus on SetExecutableModule, as that is (one) place where all of these paths converge, and it covers the most expensive part of the operation. I don't think it's the only way to capture all this information, but I still think it's a way to do it. However, it's definitely not the same as "loading/creating a target".
I can give you at least three:
Neither of these scenarios is particularly common, so I don't think they'll skew the data if you present them as "creating a target". And in a way they're correct, since in e.g., the execve case, you actually have to reparse all of the debug info for the new image so, in a sense, you are creating a new target. However, I think we should be very explicit in the code about what we are doing. |
- Use separate entries for Process and Main Executable module - Rename for clarity
Thanks for the example! THat makes a lot more sense now.
About the |
oontvoo
commented
Mar 12, 2025
lldb/source/Target/Process.cpp
Outdated
| TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid)); | ||
| if (target_sp) { | ||
| helper.SetDebugger(&(target_sp->GetDebugger())); | ||
| exec_uuid = target_sp->GetExecModuleUUID(); |
There was a problem hiding this comment.
We save the Target's exec-module's UUID in the Target object because the Module object is gone by the time we're here.
There was a problem hiding this comment.
Are you sure about that. Modules normally exist long after the process exits:
$ lldb echo
(lldb) target create "echo"
Current executable set to '/bin/echo' (x86_64).
(lldb) r
Process 15996 launched: '/bin/echo' (x86_64)
Process 15996 exited with status = 0 (0x00000000)
(lldb) image list
[ 0] 676F00F7 0x0000555555554000 /bin/echo
[ 1] D626A570 0x00007ffff7fc6000 /lib64/ld-linux-x86-64.so.2
[ 2] 421DCFD2-138A-B321-D6F1-7AFD7B7FC999-F79CA445 0x00007ffff7fc5000 [vdso] (0x00007ffff7fc5000)
[ 3] C88DE6C8 0x00007ffff7d99000 /lib64/libc.so.6
/usr/lib/debug/lib64/libc.so.6.debug
It's possible this happens in some exceptional exit scenarios, but in that case, I'd like to know what they are. In general, you cannot assume that a target will always have an executable module due to the scenarios I mentioned before.
There was a problem hiding this comment.
I can't seem to reproduce it now but I was getting a crash (due to accessing the ModuleSP object here), hence the check on the TargetSP and the code to save it.
But anyway, i'll revert that back to the simple case. We can fix the crash if/when it manifest again
There was a problem hiding this comment.
@labath Ah, I've found the test that crashed! (See the build log on this PR). Without the check for the validity of the Target pointer and the module pointer, the test crashed/timedout. By contrast, if we added this check, the whole test suite passed:
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();
}
There was a problem hiding this comment.
That sort of makes sense. The target would not be available if it was already being destroyed. However, Debugger::FindTargetWithProcessID is still a very roundabout way to check for target validity. You should be able to achieve the same thing with:
if (TargetSP target_sp = m_target_wp.lock()) {
...
}
There was a problem hiding this comment.
Oh, ok - thanks! will apply this in the rollforward patch.
labath
reviewed
Mar 13, 2025
lldb/source/Target/Process.cpp
Outdated
| TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid)); | ||
| if (target_sp) { | ||
| helper.SetDebugger(&(target_sp->GetDebugger())); | ||
| exec_uuid = target_sp->GetExecModuleUUID(); |
There was a problem hiding this comment.
Are you sure about that. Modules normally exist long after the process exits:
$ lldb echo
(lldb) target create "echo"
Current executable set to '/bin/echo' (x86_64).
(lldb) r
Process 15996 launched: '/bin/echo' (x86_64)
Process 15996 exited with status = 0 (0x00000000)
(lldb) image list
[ 0] 676F00F7 0x0000555555554000 /bin/echo
[ 1] D626A570 0x00007ffff7fc6000 /lib64/ld-linux-x86-64.so.2
[ 2] 421DCFD2-138A-B321-D6F1-7AFD7B7FC999-F79CA445 0x00007ffff7fc5000 [vdso] (0x00007ffff7fc5000)
[ 3] C88DE6C8 0x00007ffff7d99000 /lib64/libc.so.6
/usr/lib/debug/lib64/libc.so.6.debug
It's possible this happens in some exceptional exit scenarios, but in that case, I'd like to know what they are. In general, you cannot assume that a target will always have an executable module due to the scenarios I mentioned before.
labath
reviewed
Mar 14, 2025
|
Hi, friendly ping? @JDevlieghere thanks! |
JDevlieghere
approved these changes
Mar 18, 2025
lldb/include/lldb/Core/Telemetry.h
Outdated
Comment on lines
88
to
89
| std::string target_uuid; | ||
| std::string arch_name; |
There was a problem hiding this comment.
Suggested change
| std::string target_uuid; | |
| std::string arch_name; | |
| UUID target_uuid; | |
| ArchSpec arch_name; |
lldb/include/lldb/Core/Telemetry.h
Outdated
| struct TargetInfo : public LLDBBaseTelemetryInfo { | ||
| lldb::ModuleSP exec_mod; | ||
|
|
||
| // The same as the executable-module's UUID. |
There was a problem hiding this comment.
Suggested change
| // The same as the executable-module's UUID. | |
| /// The same as the executable-module's UUID. |
lldb/include/lldb/Core/Telemetry.h
Outdated
Comment on lines
91
to
94
| // If true, this entry was emitted at the beginning of an event (eg., before | ||
| // the executable laod). Otherwise, it was emitted at the end of an event | ||
| // (eg., after the module and any dependency were loaded.) | ||
| bool is_start_entry; |
There was a problem hiding this comment.
Suggested change
| // If true, this entry was emitted at the beginning of an event (eg., before | |
| // the executable laod). Otherwise, it was emitted at the end of an event | |
| // (eg., after the module and any dependency were loaded.) | |
| bool is_start_entry; | |
| /// If true, this entry was emitted at the beginning of an event (eg., before | |
| /// the executable laod). Otherwise, it was emitted at the end of an event | |
| /// (eg., after the module and any dependency were loaded.) | |
| bool is_start_entry; |
lldb/include/lldb/Core/Telemetry.h
Outdated
| // (eg., after the module and any dependency were loaded.) | ||
| bool is_start_entry; | ||
|
|
||
| // Describes the exit of the executable module. |
There was a problem hiding this comment.
Suggested change
| // Describes the exit of the executable module. | |
| /// Describes the exit of the executable module. |
lldb/include/lldb/Core/Telemetry.h
Outdated
| @@ -146,6 +155,59 @@ struct DebuggerInfo : public LLDBBaseTelemetryInfo { | |||
| void serialize(llvm::telemetry::Serializer &serializer) const override; | |||
| }; | |||
|
|
|||
| struct ExecModuleInfo : public LLDBBaseTelemetryInfo { | |||
There was a problem hiding this comment.
Suggested change
| struct ExecModuleInfo : public LLDBBaseTelemetryInfo { | |
| struct ExecutableModuleInfo : public LLDBBaseTelemetryInfo { |
| } | ||
|
|
||
| static bool classof(const TelemetryInfo *T) { | ||
| // Subclasses of this is also acceptable |
There was a problem hiding this comment.
Suggested change
| // Subclasses of this is also acceptable | |
| // Subclasses of this is also acceptable. |
|
|
||
| /// Describes an exit status. | ||
| struct ExitDescription { | ||
| int exit_code; |
There was a problem hiding this comment.
Contrary to my other comments I think we can keep the exit_ prefix here as "exit code" is a fairly well understood term.
lldb/source/Core/Telemetry.cpp
Outdated
| @@ -76,15 +76,36 @@ void CommandInfo::serialize(Serializer &serializer) const { | |||
| serializer.write("error_data", error_data.value()); | |||
| } | |||
|
|
|||
| std::atomic<uint64_t> CommandInfo::g_command_id_seed = 0; | |||
| uint64_t CommandInfo::GetNextId() { return g_command_id_seed.fetch_add(1); } | |||
There was a problem hiding this comment.
Suggested change
| uint64_t CommandInfo::GetNextId() { return g_command_id_seed.fetch_add(1); } | |
| uint64_t CommandInfo::GetNextID() { return g_command_id_seed.fetch_add(1); } |
|
|
||
| // Find the executable-module's UUID, if available. | ||
| Target &target = GetTarget(); | ||
| helper.SetDebugger(&(target.GetDebugger())); |
There was a problem hiding this comment.
Suggested change
| helper.SetDebugger(&(target.GetDebugger())); | |
| helper.SetDebugger(&target.GetDebugger()); |
lldb/source/Target/Target.cpp
Outdated
Comment on lines
1568
to
1570
| lldb::pid_t pid; | ||
| if (ProcessSP proc = GetProcessSP()) | ||
| pid = proc->GetID(); |
There was a problem hiding this comment.
Suggested change
| lldb::pid_t pid; | |
| if (ProcessSP proc = GetProcessSP()) | |
| pid = proc->GetID(); | |
| lldb::pid_t pid = LLDB_INVALID_PROCESS_ID; | |
| if (ProcessSP proc = GetProcessSP()) | |
| pid = proc->GetID(); |
|
Thanks! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/18394 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/14520 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/13132 Here is the relevant piece of the build log for the reference |
oontvoo
added a commit
to oontvoo/llvm-project
that referenced
this pull request
Mar 19, 2025
… a target (llvm#127834)" This reverts commit 7dbcdd5.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.