Skip to content

[lldb] Implement bidirectional access for backing<->backed thread relationship #125300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 3, 2025
Merged

[lldb] Implement bidirectional access for backing<->backed thread relationship #125300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0c9d9ed
[lldb] Implement bidirectional access for backing<->backed thread rel…
felipepiovezan Jan 31, 2025
b19eb1c
fixup! Address review comments
felipepiovezan Feb 1, 2025
c54af40
fixup! Fix documentation typo
felipepiovezan Feb 1, 2025
1882d04
fixup! Break circular shared_ptr references
felipepiovezan Feb 3, 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
23 changes: 23 additions & 0 deletions lldb/include/lldb/Target/Thread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,26 @@ class Thread : public std::enable_shared_from_this<Thread>,

virtual void ClearStackFrames();

/// Sets the thread that is backed by this thread.
/// If backed_thread.GetBackedThread() is null, this method also calls
/// backed_thread.SetBackingThread(this).
/// If backed_thread.GetBackedThread() is non-null, asserts that it is equal
/// to `this`.
void SetBackedThread(Thread &backed_thread) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have an assert that's requiring that backed_thread has to have had its backing thread set first, then passed to this API. Would it be a more user-friendly API if it set the backed_thread's backing thread here, if it isn't already set.
It's still fine to assert if somebody set the wrong backing thread, that seems like an unintended error. But if the backed_thread.GetBackingThread was null, is there any reason not to set it for them here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, this also addresses my concern of people calling SetBackedThread without also calling SetBackingThread. Will update.

m_backed_thread = backed_thread.shared_from_this();

// Ensure the bidrectional relationship is preserved.
Thread *backing_thread = backed_thread.GetBackingThread().get();
assert(backing_thread == nullptr || backing_thread == this);
if (backing_thread == nullptr)
backed_thread.SetBackingThread(shared_from_this());
}

void ClearBackedThread() { m_backed_thread.reset(); }

/// Returns the thread that is backed by this thread, if any.
lldb::ThreadSP GetBackedThread() const { return m_backed_thread.lock(); }

virtual bool SetBackingThread(const lldb::ThreadSP &thread_sp) {
return false;
}
Expand Down Expand Up @@ -1349,6 +1369,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
LazyBool m_override_should_notify;
mutable std::unique_ptr<ThreadPlanStack> m_null_plan_stack_up;

/// The Thread backed by this thread, if any.
lldb::ThreadWP m_backed_thread;

private:
bool m_extended_info_fetched; // Have we tried to retrieve the m_extended_info
// for this thread?
Expand Down
2 changes: 0 additions & 2 deletions lldb/include/lldb/Target/ThreadList.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,6 @@ class ThreadList : public ThreadCollection {

lldb::ThreadSP GetThreadSPForThreadPtr(Thread *thread_ptr);

lldb::ThreadSP GetBackingThread(const lldb::ThreadSP &real_thread);

bool ShouldStop(Event *event_ptr);

Vote ShouldReportStop(Event *event_ptr);
Expand Down
7 changes: 6 additions & 1 deletion lldb/source/Plugins/Process/Utility/ThreadMemory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,17 @@ class ThreadMemory : public lldb_private::Thread {

void ClearStackFrames() override;

void ClearBackingThread() override { m_backing_thread_sp.reset(); }
void ClearBackingThread() override {
if (m_backing_thread_sp)
m_backing_thread_sp->ClearBackedThread();
m_backing_thread_sp.reset();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really wanted to make ClearBackedThread a protected member of Thread, but then we can't access it here. Protected only works if you're going through this.


bool SetBackingThread(const lldb::ThreadSP &thread_sp) override {
// printf ("Thread 0x%llx is being backed by thread 0x%llx\n", GetID(),
// thread_sp->GetID());
m_backing_thread_sp = thread_sp;
thread_sp->SetBackedThread(*this);
return (bool)thread_sp;
}

Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1728,7 +1728,7 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
thread_sp->SetStopInfo(StopInfoSP());
// If there's a memory thread backed by this thread, we need to use it to
// calculate StopInfo.
if (ThreadSP memory_thread_sp = m_thread_list.GetBackingThread(thread_sp))
if (ThreadSP memory_thread_sp = thread_sp->GetBackedThread())
thread_sp = memory_thread_sp;

if (exc_type != 0) {
Expand Down
14 changes: 0 additions & 14 deletions lldb/source/Target/ThreadList.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,20 +191,6 @@ ThreadSP ThreadList::GetThreadSPForThreadPtr(Thread *thread_ptr) {
return thread_sp;
}

ThreadSP ThreadList::GetBackingThread(const ThreadSP &real_thread) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());

ThreadSP thread_sp;
const uint32_t num_threads = m_threads.size();
for (uint32_t idx = 0; idx < num_threads; ++idx) {
if (m_threads[idx]->GetBackingThread() == real_thread) {
thread_sp = m_threads[idx];
break;
}
}
return thread_sp;
}

ThreadSP ThreadList::FindThreadByIndexID(uint32_t index_id, bool can_update) {
std::lock_guard<std::recursive_mutex> guard(GetMutex());

Expand Down