Skip to content
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
0d6a36d
[LLDB][Telemetry]Define TargetInfo for collecting data about a target
oontvoo Feb 19, 2025
19c6181
Merge branch 'llvm:main' into target
oontvoo Feb 24, 2025
f152b3b
introduce helper
oontvoo Feb 24, 2025
4f8555a
undo change
oontvoo Mar 3, 2025
a5bd22e
Merge branch 'main' into target
oontvoo Mar 3, 2025
bb22874
update interface
oontvoo Mar 3, 2025
f49c3e2
formatting
oontvoo Mar 3, 2025
3bd8897
remove unused
oontvoo Mar 3, 2025
d3cb8d8
formatting
oontvoo Mar 3, 2025
267345a
qual
oontvoo Mar 4, 2025
32d949b
use UUID for target_uuid
oontvoo Mar 5, 2025
5c6c83c
Merge branch 'llvm:main' into target
oontvoo Mar 5, 2025
39c39cc
Merge branch 'main' into target
oontvoo Mar 6, 2025
1fdf120
revert dup
oontvoo Mar 8, 2025
1ed73e7
Merge branch 'target' of github.com:oontvoo/llvm-project into target
oontvoo Mar 8, 2025
92e06fa
Merge branch 'main' into target
oontvoo Mar 8, 2025
aab5002
unused include
oontvoo Mar 8, 2025
31c63e0
Update Telemetry.h
oontvoo Mar 8, 2025
20c8f59
Update Telemetry.cpp
oontvoo Mar 8, 2025
9954083
reconcile with main
oontvoo Mar 8, 2025
55aa4ee
Merge branch 'target' of github.com:oontvoo/llvm-project into target
oontvoo Mar 8, 2025
c8ca5b8
format
oontvoo Mar 8, 2025
4783b57
Update lldb/include/lldb/Core/Telemetry.h
oontvoo Mar 11, 2025
ae73ebe
reformatting
oontvoo Mar 12, 2025
0a2505a
Addressed review comments:
oontvoo Mar 12, 2025
4d3d26e
...
oontvoo Mar 12, 2025
b1b7a50
addressed review comment
oontvoo Mar 13, 2025
f7be9c9
remove unused import
oontvoo Mar 13, 2025
41f0696
Merge branch 'llvm:main' into target
oontvoo Mar 18, 2025
5e99cc7
addressed review comments
oontvoo Mar 18, 2025
a709ac3
Update Telemetry.h
oontvoo Mar 18, 2025
220554a
Update Telemetry.cpp
oontvoo Mar 18, 2025
1ecbe48
Update CommandInterpreter.cpp
oontvoo Mar 18, 2025
1231bc2
Update Target.cpp
oontvoo Mar 18, 2025
5757b41
Merge branch 'llvm:main' into target
oontvoo Mar 18, 2025
a936e2e
Merge branch 'target' of github.com:oontvoo/llvm-project into target
oontvoo Mar 18, 2025
142097a
more fixing
oontvoo Mar 18, 2025
d297813
formating
oontvoo Mar 18, 2025
885675e
Merge branch 'main' into target
oontvoo Mar 18, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 65 additions & 4 deletions lldb/include/lldb/Core/Telemetry.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to say I'm liking this bitfield system less and less every time I see it.

// 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.
Expand Down Expand Up @@ -146,6 +155,58 @@ struct DebuggerInfo : public LLDBBaseTelemetryInfo {
void serialize(llvm::telemetry::Serializer &serializer) const override;
};

struct ExecModuleInfo : public LLDBBaseTelemetryInfo {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
struct ExecModuleInfo : public LLDBBaseTelemetryInfo {
struct ExecutableModuleInfo : public LLDBBaseTelemetryInfo {

lldb::ModuleSP exec_mod;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop the exec_ prefix as this is relatively obvious from the class it belongs to and we don't consistently prefix all the members.

Suggested change
lldb::ModuleSP exec_mod;
lldb::ModuleSP exec_mod;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


/// The same as the executable-module's UUID.
UUID exec_uuid;
/// PID of the process owned by this target.
lldb::pid_t pid;
std::string arch_name;

/// If true, this entry was emitted at the beginning of an event (eg., before
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// If true, this entry was emitted at the beginning of an event (eg., before
/// If true, this entry was emitted at the beginning of an event (e.g. 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;

ExecModuleInfo() = default;

llvm::telemetry::KindType getKind() const override {
return LLDBEntryKind::ExecModuleInfo;
}

static bool classof(const TelemetryInfo *T) {
// Subclasses of this is also acceptable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Subclasses of this is also acceptable
// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contrary to my other comments I think we can keep the exit_ prefix here as "exit code" is a fairly well understood term.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

std::string description;
};

struct ProcessExitInfo : public LLDBBaseTelemetryInfo {
UUID exec_uuid;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
UUID exec_uuid;
UUID uuid;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

lldb::pid_t pid;
bool is_start_entry;
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Subclasses of this is also acceptable
// 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.
Expand Down
4 changes: 4 additions & 0 deletions lldb/include/lldb/Target/Target.h
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,8 @@ class Target : public std::enable_shared_from_this<Target>,
return GetStaticBroadcasterClass();
}

UUID GetExecModuleUUID() const { return m_current_exec_mod_uuid; }

// This event data class is for use by the TargetList to broadcast new target
// notifications.
class TargetEventData : public EventData {
Expand Down Expand Up @@ -1640,6 +1642,8 @@ class Target : public std::enable_shared_from_this<Target>,
private:
// Target metrics storage.
TargetStats m_stats;
// The current executable module UUID.
UUID m_current_exec_mod_uuid;

public:
/// Get metrics associated with this target in JSON format.
Expand Down
26 changes: 24 additions & 2 deletions lldb/source/Core/Telemetry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#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>
Expand Down Expand Up @@ -76,15 +77,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); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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); }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


void DebuggerInfo::serialize(Serializer &serializer) const {
LLDBBaseTelemetryInfo::serialize(serializer);

serializer.write("lldb_version", lldb_version);
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 ExecModuleInfo::serialize(Serializer &serializer) const {
LLDBBaseTelemetryInfo::serialize(serializer);

serializer.write("exec_uuid", exec_uuid.GetAsString());
serializer.write("pid", pid);
serializer.write("arch_name", arch_name);
serializer.write("is_start_entry", is_start_entry);
}

void ProcessExitInfo::serialize(Serializer &serializer) const {
LLDBBaseTelemetryInfo::serialize(serializer);

serializer.write("exec_uuid", exec_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()) {}
Expand Down
24 changes: 24 additions & 0 deletions lldb/source/Target/Process.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1064,6 +1065,29 @@ 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.
UUID exec_uuid;
// Check if there is (still) a valid target and get the debugger and exec_uuid
// from it.
TargetSP target_sp(Debugger::FindTargetWithProcessID(m_pid));
if (target_sp) {
helper.SetDebugger(&(target_sp->GetDebugger()));
exec_uuid = target_sp->GetExecModuleUUID();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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();
   }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()) {
  ...
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok - thanks! will apply this in the rollforward patch.

}

helper.DispatchNow([&](telemetry::ProcessExitInfo *info) {
info->exec_uuid = exec_uuid;
info->pid = m_pid;
info->is_start_entry = true;
info->exit_desc = {status, exit_string.str()};
});

helper.DispatchOnExit([&](telemetry::ProcessExitInfo *info) {
info->exec_uuid = exec_uuid;
info->pid = m_pid;
});

Log *log(GetLog(LLDBLog::State | LLDBLog::Process));
LLDB_LOG(log, "(plugin = {0} status = {1} ({1:x8}), description=\"{2}\")",
Expand Down
20 changes: 20 additions & 0 deletions lldb/source/Target/Target.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -1559,10 +1560,29 @@ void Target::DidExec() {

void Target::SetExecutableModule(ModuleSP &executable_sp,
LoadDependentFiles load_dependent_files) {
telemetry::ScopedDispatcher<telemetry::ExecModuleInfo> helper(&m_debugger);
Log *log = GetLog(LLDBLog::Target);
ClearModules(false);

if (executable_sp) {
m_current_exec_mod_uuid = executable_sp->GetUUID();
lldb::pid_t pid;
if (ProcessSP proc = GetProcessSP())
pid = proc->GetID();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

helper.DispatchNow([&](telemetry::ExecModuleInfo *info) {
info->exec_mod = executable_sp;
info->exec_uuid = m_current_exec_mod_uuid;
info->pid = pid;
info->arch_name = executable_sp->GetArchitecture().GetArchitectureName();
info->is_start_entry = true;
});

helper.DispatchOnExit([&](telemetry::ExecModuleInfo *info) {
info->exec_mod = executable_sp;
info->exec_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());
Expand Down