[lldb] Add support for unique target ids #160736
Conversation
qxy11
requested review from
clayborg,
dmpots,
jeffreytan81 and
walter-erquinigo
|
@llvm/pr-subscribers-lldb Author: Janet Yang (qxy11) ChangesSummaryAdd support for unique target ids per Target instance. This is needed for upcoming changes to allow debugger instances to be shared across separate DAP instances for child process/gpu debugging. Each Target instance will have its own unique id, and uses a function-local counter in TestsAdded several unit tests to test basic functionality, uniqueness of targets, and target deletion doesn't affect the uniqueness. Full diff: https://github.com/llvm/llvm-project/pull/160736.diff 8 Files Affected:
diff --git a/lldb/include/lldb/API/SBDebugger.h b/lldb/include/lldb/API/SBDebugger.h
index f77b0c1d7f0ee..efd95677d1d36 100644
--- a/lldb/include/lldb/API/SBDebugger.h
+++ b/lldb/include/lldb/API/SBDebugger.h
@@ -359,6 +359,9 @@ class LLDB_API SBDebugger {
lldb::SBTarget FindTargetWithFileAndArch(const char *filename,
const char *arch);
+ /// Find a target with the specified unique ID
+ lldb::SBTarget FindTargetWithUniqueID(uint32_t id);
+
/// Get the number of targets in the debugger.
uint32_t GetNumTargets();
diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h
index 62cdd342a05e4..1eef9368dceaf 100644
--- a/lldb/include/lldb/API/SBTarget.h
+++ b/lldb/include/lldb/API/SBTarget.h
@@ -357,6 +357,8 @@ class LLDB_API SBTarget {
const char *GetLabel() const;
+ uint32_t GetUniqueID() const;
+
SBError SetLabel(const char *label);
/// Architecture opcode byte size width accessor
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index 14a09f29094d5..14375929688e4 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -600,6 +600,12 @@ class Target : public std::enable_shared_from_this<Target>,
bool IsDummyTarget() const { return m_is_dummy_target; }
+ /// Get the unique ID for this target.
+ ///
+ /// \return
+ /// The unique ID for this target, or 0 if no ID has been assigned.
+ uint32_t GetUniqueID() const { return m_target_unique_id; }
+
const std::string &GetLabel() const { return m_label; }
/// Set a label for a target.
@@ -1651,6 +1657,7 @@ class Target : public std::enable_shared_from_this<Target>,
bool m_suppress_stop_hooks; /// Used to not run stop hooks for expressions
bool m_is_dummy_target;
unsigned m_next_persistent_variable_index = 0;
+ uint32_t m_target_unique_id = 0; /// The unique ID assigned to this target
/// An optional \a lldb_private::Trace object containing processor trace
/// information of this target.
lldb::TraceSP m_trace_sp;
diff --git a/lldb/include/lldb/Target/TargetList.h b/lldb/include/lldb/Target/TargetList.h
index 080a6039c7ff8..343fc1676ec30 100644
--- a/lldb/include/lldb/Target/TargetList.h
+++ b/lldb/include/lldb/Target/TargetList.h
@@ -159,6 +159,8 @@ class TargetList : public Broadcaster {
lldb::TargetSP FindTargetWithProcess(lldb_private::Process *process) const;
+ lldb::TargetSP FindTargetWithUniqueID(uint32_t id) const;
+
lldb::TargetSP GetTargetSP(Target *target) const;
/// Send an async interrupt to one or all processes.
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index 603e306497841..f4b46cc3b1873 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -983,6 +983,16 @@ uint32_t SBDebugger::GetIndexOfTarget(lldb::SBTarget target) {
return m_opaque_sp->GetTargetList().GetIndexOfTarget(target.GetSP());
}
+SBTarget SBDebugger::FindTargetWithUniqueID(uint32_t id) {
+ LLDB_INSTRUMENT_VA(this, id);
+ SBTarget sb_target;
+ if (m_opaque_sp) {
+ // No need to lock, the target list is thread safe
+ sb_target.SetSP(m_opaque_sp->GetTargetList().FindTargetWithUniqueID(id));
+ }
+ return sb_target;
+}
+
SBTarget SBDebugger::FindTargetWithProcessID(lldb::pid_t pid) {
LLDB_INSTRUMENT_VA(this, pid);
diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index eb56337de3c44..affde64a389af 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -1632,6 +1632,14 @@ const char *SBTarget::GetLabel() const {
return nullptr;
}
+uint32_t SBTarget::GetUniqueID() const {
+ LLDB_INSTRUMENT_VA(this);
+
+ if (TargetSP target_sp = GetSP())
+ return target_sp->GetUniqueID();
+ return 0;
+}
+
SBError SBTarget::SetLabel(const char *label) {
LLDB_INSTRUMENT_VA(this, label);
diff --git a/lldb/source/Target/TargetList.cpp b/lldb/source/Target/TargetList.cpp
index 7037dc2bea3cc..3ae61df3460b7 100644
--- a/lldb/source/Target/TargetList.cpp
+++ b/lldb/source/Target/TargetList.cpp
@@ -256,6 +256,8 @@ Status TargetList::CreateTargetInternal(Debugger &debugger,
Status error;
const bool is_dummy_target = false;
+ static uint32_t g_target_unique_id = 0;
+
ArchSpec arch(specified_arch);
if (arch.IsValid()) {
@@ -344,6 +346,8 @@ Status TargetList::CreateTargetInternal(Debugger &debugger,
if (!target_sp)
return error;
+ target_sp->m_target_unique_id = ++g_target_unique_id;
+
// Set argv0 with what the user typed, unless the user specified a
// directory. If the user specified a directory, then it is probably a
// bundle that was resolved and we need to use the resolved bundle path
@@ -428,6 +432,18 @@ TargetSP TargetList::FindTargetWithProcess(Process *process) const {
return target_sp;
}
+TargetSP TargetList::FindTargetWithUniqueID(uint32_t id) const {
+ std::lock_guard<std::recursive_mutex> guard(m_target_list_mutex);
+ auto it = llvm::find_if(m_target_list, [id](const TargetSP &item) {
+ return item->GetUniqueID() == id;
+ });
+
+ if (it != m_target_list.end())
+ return *it;
+
+ return TargetSP();
+}
+
TargetSP TargetList::GetTargetSP(Target *target) const {
TargetSP target_sp;
if (!target)
diff --git a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
index 43f45f330ee2a..4d82fdc83b2cf 100644
--- a/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
+++ b/lldb/test/API/python_api/debugger/TestDebuggerAPI.py
@@ -294,3 +294,106 @@ def test_version(self):
self.assertEqual(instance_str, class_str)
self.assertEqual(class_str, property_str)
+
+ def test_find_target_with_unique_id(self):
+ """Test SBDebugger.FindTargetWithUniqueID() functionality."""
+
+ # Test with invalid ID - should return invalid target
+ invalid_target = self.dbg.FindTargetWithUniqueID(999999)
+ self.assertFalse(invalid_target.IsValid())
+
+ # Test with ID 0 - should return invalid target
+ zero_target = self.dbg.FindTargetWithUniqueID(0)
+ self.assertFalse(zero_target.IsValid())
+
+ # Build a real executable and create target with it
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ target = self.dbg.CreateTarget(exe)
+ self.assertTrue(target.IsValid())
+
+ # Find the target using its unique ID
+ unique_id = target.GetUniqueID()
+ self.assertNotEqual(unique_id, 0)
+ found_target = self.dbg.FindTargetWithUniqueID(unique_id)
+ self.assertTrue(found_target.IsValid())
+ self.assertEqual(
+ self.dbg.GetIndexOfTarget(target), self.dbg.GetIndexOfTarget(found_target)
+ )
+ self.assertEqual(found_target.GetUniqueID(), unique_id)
+
+ def test_target_unique_id_uniqueness(self):
+ """Test that Target.GetUniqueID() returns unique values across multiple targets."""
+
+ # Create multiple targets and verify they all have unique IDs
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ targets = []
+ unique_ids = set()
+
+ for i in range(10):
+ target = self.dbg.CreateTarget(exe)
+ self.assertTrue(target.IsValid())
+
+ unique_id = target.GetUniqueID()
+ self.assertNotEqual(unique_id, 0)
+
+ # Verify this ID hasn't been used before
+ self.assertNotIn(
+ unique_id, unique_ids, f"Duplicate unique ID found: {unique_id}"
+ )
+
+ unique_ids.add(unique_id)
+ targets.append(target)
+
+ # Verify all targets can still be found by their IDs
+ for target in targets:
+ unique_id = target.GetUniqueID()
+ found = self.dbg.FindTargetWithUniqueID(unique_id)
+ self.assertTrue(found.IsValid())
+ self.assertEqual(found.GetUniqueID(), unique_id)
+
+ def test_target_unique_id_uniqueness_after_deletion(self):
+ """Test finding targets have unique ID after target deletion."""
+ # Create two targets
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+ target1 = self.dbg.CreateTarget(exe)
+ target2 = self.dbg.CreateTarget(exe)
+ self.assertTrue(target1.IsValid())
+ self.assertTrue(target2.IsValid())
+
+ unique_id1 = target1.GetUniqueID()
+ unique_id2 = target2.GetUniqueID()
+ self.assertNotEqual(unique_id1, 0)
+ self.assertNotEqual(unique_id2, 0)
+ self.assertNotEqual(unique_id1, unique_id2)
+
+ # Verify we can find them initially
+ found_target1 = self.dbg.FindTargetWithUniqueID(unique_id1)
+ found_target2 = self.dbg.FindTargetWithUniqueID(unique_id2)
+ self.assertTrue(found_target1.IsValid())
+ self.assertTrue(found_target2.IsValid())
+ target2_index = self.dbg.GetIndexOfTarget(target2)
+
+ # Delete target 2
+ deleted = self.dbg.DeleteTarget(target2)
+ self.assertTrue(deleted)
+
+ # Try to find the deleted target - should not be found
+ not_found_target = self.dbg.FindTargetWithUniqueID(unique_id2)
+ self.assertFalse(not_found_target.IsValid())
+
+ # Create a new target
+ target3 = self.dbg.CreateTarget(exe)
+ self.assertTrue(target3.IsValid())
+ # Target list index of target3 should be the same as target2's
+ # since it was deleted, but it should have a distinct unique ID
+ target3_index = self.dbg.GetIndexOfTarget(target3)
+ unique_id3 = target3.GetUniqueID()
+ self.assertEqual(target3_index, target2_index)
+ self.assertNotEqual(unique_id3, unique_id2)
+ self.assertNotEqual(unique_id3, unique_id1)
+ # Make sure we can find the new target
+ found_target3 = self.dbg.FindTargetWithUniqueID(target3.GetUniqueID())
+ self.assertTrue(found_target3.IsValid())
|
lldb/source/Target/TargetList.cpp
Outdated
| if (!target_sp) | ||
| return error; | ||
|
|
||
| target_sp->m_target_unique_id = ++g_target_unique_id; |
There was a problem hiding this comment.
instead of having a temporary state in which the unique_id is 0, better set this ID within the constructor of lldb_private::Target.
The ID should be > 0. This will cause that naturally the dummy target has ID 1.
For the SBTarget case, if the SBTarget is invalid (no valid pointer to *Target), you can create a new define LLDB_INVALID_TARGET_ID=0, just like LLDB_INVALID_ADDRESS. And then you return it in that case.
There was a problem hiding this comment.
Yes, this seems much simpler to me, and I see no reason to delay assigning the Target's unique ID.
|
In the context of MCP support, we have been discussing adding a unique debugger identifier. If we're going to come up with something unique, it may be nice to make it globally unique, even across debuggers. |
I'm confused. Debugger's already have a unique ID: Debugger's already have a unique, monotonically increasing id: That's what gets returned by Debugger::GetID(). The problem here is that we refer to Targets just by their index in the target list. But I wonder if it wouldn't be better to change THAT, and have the targets listed by a Debugger Specific unique ID instead. It's kind of bogus that if you have more than one target, and you remove the middle one, that changes the number you use to refer to the targets above the one deleted in all the command interfaces for them. We have very few other "lists of things" that we refer to by list index rather than a unique ID, and I don't think doing it for Targets was really on purpose. If you did that change, then you wouldn't need a globally unique Target ID, since the tuple {DebuggerID, TargetID} would be unique. |
Within a single process. The |
|
I guess you mean by "Giving the debugger a unique ID" you mean "unique across all instances of lldb past or present"... I'm not sure why you really want that, for all the ones currently extant the combo of {lldb pid, debugger ID} would do. Why do you care about not overlapping with defunct Debuggers? In any case, whatever you call the debugger, {DebuggerID, TargetID} would still be universally unique if the debugger was and the Target ID was unique within a Debugger. So it seems unnecessary work to make the Target's universally unique. |
Yes, that's also an option, but would increase the "hierarchy" in the source URL. Right now, debugger and target resources are listed as: Adding the pid would make that a little bit more complicated, and unnecessarily so for the common case where you only have one instance. |
|
Having something like a session identifier could also work for the mcp use case, e.g. That provides us with enough information to distinguish two lldb instances. |
@jimingham , what about having The latter part of this suggestion isn't a thing. Target's don't currently have a GetID API, they only have their index in the Debugger's target list. That's how you refer to them in all the commands. That was the point of my earlier complaint: Deleting target 1 changes the ID you use to refer to the third created target from 2 to 1... That does seem bogus to me, and I was suggesting fixing those two together. However, I don't think we should show users "All Debuggers wide target ID's" in a command line which is explicitly per debugger. I think it would be kind of weird to see: If you really need a unique to all debugger's Target ID because somehow composing the debugger ID and the target ID is hard, then it's probably better to separate the two issues:
Then solve #2 as this patch does, using a separate Target ID that we never print anywhere and doesn't show up in the command line interfaces at all. Then when somebody gets around to it we can change 1 to be a real but per Debugger ID and not an index. |
Summary: Also use lldb::user_id_t throughout instead of uint32_t. It seems to be the custom around the codebase to use this type for unique ids rather than a uint32_t
|
@jimingham , thanks for the info. It seems you edited my comment instead of replying :) I any case, I'm fine with the existing direction of this patch w.r.t. simply adding a global ID. Any particular feedback about the current implementation? |
The code seems fine. You never say what this ID is Unique with respect to. If it were me I'd call the API GetGloballyUniqueID since this is guaranteeing a unique-ness beyond the Target's natural container - the Debugger. Or you could just more explicitly document the behavior you are implementing. You are also asserting that this is a globally Unique ID, but you don't actually test that, since all your tests have only one debugger. You should probably also make a couple Debuggers and assert that their "UniqueID's" also don't overlap. |
|
@qxy11: Jim gave you good feedback. Try to apply that and I'll do a review |
|
✅ With the latest revision this PR passed the Python code formatter. |
walter-erquinigo
left a comment
walter-erquinigo
left a comment
There was a problem hiding this comment.
pretty nice. Almost there
lldb/include/lldb/API/SBDebugger.h
Outdated
| lldb::SBTarget FindTargetWithFileAndArch(const char *filename, | ||
| const char *arch); | ||
|
|
||
| /// Find a target with the specified unique ID |
There was a problem hiding this comment.
| /// Find a target with the specified unique ID | |
| /// Find a target with the specified unique ID. |
|
|
||
| lldb::TargetSP FindTargetWithProcess(lldb_private::Process *process) const; | ||
|
|
||
| /// Find the target that has a globally unique ID that matches ID \a id |
There was a problem hiding this comment.
| /// Find the target that has a globally unique ID that matches ID \a id | |
| /// Find the target that has a globally unique ID that matches ID \a id. |
6 participants
Summary
Add support for unique target ids per Target instance. This is needed for upcoming changes to allow debugger instances to be shared across separate DAP instances for child process debugging. We want the IDE to be able to attach to existing targets in an already runny lldb-dap session, and having a unique ID per target would make that easier.
Each Target instance will have its own unique id, and uses a function-local counter in
TargetList::CreateTargetInternalto assign incremental unique ids.Tests
Added several unit tests to test basic functionality, uniqueness of targets, and target deletion doesn't affect the uniqueness.