[lldb][NFC] Move ShouldShow/ShouldSelect logic into Stopinfo #134160
Merged
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
This NFC patch simplifies the main loop in HandleProcessStateChanged event by moving duplicated code into the StopInfo class, also allowing StopInfo subclasses to override behavior. More specifically, two functions are created: * ShouldShow: should a Thread with such StopInfo should be printed when the debugger stops? Currently, no StopInfo subclasses override this, but a subsequent patch will fix a bug by making StopInfoBreakpoint check whether the breakpoint is internal. * ShouldSelect: should a Thread with such a StopInfo be selected? This is currently overridden by StopInfoUnixSignal but will, in the future, be overridden by StopInfoBreakpoint.
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThis NFC patch simplifies the main loop in HandleProcessStateChanged event by moving duplicated code into the StopInfo class, also allowing StopInfo subclasses to override behavior. More specifically, two functions are created:
Full diff: https://github.com/llvm/llvm-project/pull/134160.diff 3 Files Affected:
diff --git a/lldb/include/lldb/Target/StopInfo.h b/lldb/include/lldb/Target/StopInfo.h
index 9a13371708be5..5fcd3784d6f1d 100644
--- a/lldb/include/lldb/Target/StopInfo.h
+++ b/lldb/include/lldb/Target/StopInfo.h
@@ -118,6 +118,19 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> {
StructuredData::ObjectSP GetExtendedInfo() { return m_extended_info; }
+ /// Returns true if this is a stop reason that should be shown to a user when
+ /// stopping.
+ virtual bool ShouldShow() const { return IsValid(); }
+
+ /// Returns true if this is a stop reason that should cause a thread to be
+ /// selected when stopping.
+ virtual bool ShouldSelect() const {
+ lldb::StopReason reason = GetStopReason();
+ return reason != lldb::eStopReasonNone &&
+ reason != lldb::eStopReasonHistoryBoundary &&
+ reason != lldb::eStopReasonInvalid;
+ }
+
static lldb::StopInfoSP
CreateStopReasonWithBreakpointSiteID(Thread &thread,
lldb::break_id_t break_id);
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 369933234ccca..502f11b5628db 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -808,30 +808,11 @@ bool Process::HandleProcessStateChangedEvent(
std::lock_guard<std::recursive_mutex> guard(thread_list.GetMutex());
ThreadSP curr_thread(thread_list.GetSelectedThread());
- ThreadSP thread;
- StopReason curr_thread_stop_reason = eStopReasonInvalid;
- bool prefer_curr_thread = false;
- if (curr_thread && curr_thread->IsValid()) {
- curr_thread_stop_reason = curr_thread->GetStopReason();
- switch (curr_thread_stop_reason) {
- case eStopReasonNone:
- case eStopReasonInvalid:
- // Don't prefer the current thread if it didn't stop for a reason.
- break;
- case eStopReasonSignal: {
- // We need to do the same computation we do for other threads
- // below in case the current thread happens to be the one that
- // stopped for the no-stop signal.
- uint64_t signo = curr_thread->GetStopInfo()->GetValue();
- if (process_sp->GetUnixSignals()->GetShouldStop(signo))
- prefer_curr_thread = true;
- } break;
- default:
- prefer_curr_thread = true;
- break;
- }
+
+ if (curr_thread && curr_thread->IsValid())
curr_thread_stop_info_sp = curr_thread->GetStopInfo();
- }
+ bool prefer_curr_thread = curr_thread_stop_info_sp &&
+ curr_thread_stop_info_sp->ShouldSelect();
if (!prefer_curr_thread) {
// Prefer a thread that has just completed its plan over another
@@ -839,54 +820,23 @@ bool Process::HandleProcessStateChangedEvent(
ThreadSP plan_thread;
ThreadSP other_thread;
- const size_t num_threads = thread_list.GetSize();
- size_t i;
- for (i = 0; i < num_threads; ++i) {
- thread = thread_list.GetThreadAtIndex(i);
- StopReason thread_stop_reason = thread->GetStopReason();
- switch (thread_stop_reason) {
- case eStopReasonInvalid:
- case eStopReasonNone:
- case eStopReasonHistoryBoundary:
- break;
-
- case eStopReasonSignal: {
- // Don't select a signal thread if we weren't going to stop at
- // that signal. We have to have had another reason for stopping
- // here, and the user doesn't want to see this thread.
- uint64_t signo = thread->GetStopInfo()->GetValue();
- if (process_sp->GetUnixSignals()->GetShouldStop(signo)) {
- if (!other_thread)
- other_thread = thread;
- }
- break;
- }
- case eStopReasonTrace:
- case eStopReasonBreakpoint:
- case eStopReasonWatchpoint:
- case eStopReasonException:
- case eStopReasonExec:
- case eStopReasonFork:
- case eStopReasonVFork:
- case eStopReasonVForkDone:
- case eStopReasonThreadExiting:
- case eStopReasonInstrumentation:
- case eStopReasonProcessorTrace:
- case eStopReasonInterrupt:
- if (!other_thread)
- other_thread = thread;
- break;
- case eStopReasonPlanComplete:
+ for (ThreadSP thread : thread_list.Threads()) {
+ StopInfoSP stop_info = thread->GetStopInfo();
+ if (!stop_info || !stop_info->ShouldSelect())
+ continue;
+ StopReason thread_stop_reason = stop_info->GetStopReason();
+ if (thread_stop_reason == eStopReasonPlanComplete) {
if (!plan_thread)
plan_thread = thread;
- break;
- }
+ } else if (!other_thread)
+ other_thread = thread;
}
if (plan_thread)
thread_list.SetSelectedThreadByID(plan_thread->GetID());
else if (other_thread)
thread_list.SetSelectedThreadByID(other_thread->GetID());
else {
+ ThreadSP thread;
if (curr_thread && curr_thread->IsValid())
thread = curr_thread;
else
@@ -5826,7 +5776,7 @@ size_t Process::GetThreadStatus(Stream &strm,
if (thread_sp) {
if (only_threads_with_stop_reason) {
StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
- if (!stop_info_sp || !stop_info_sp->IsValid())
+ if (!stop_info_sp || !stop_info_sp->ShouldShow())
continue;
}
thread_sp->GetStatus(strm, start_frame, num_frames,
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index e9e3603e55316..28881bfc471df 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -1080,12 +1080,7 @@ class StopInfoUnixSignal : public StopInfo {
return false;
}
- bool ShouldStop(Event *event_ptr) override {
- ThreadSP thread_sp(m_thread_wp.lock());
- if (thread_sp)
- return thread_sp->GetProcess()->GetUnixSignals()->GetShouldStop(m_value);
- return false;
- }
+ bool ShouldStop(Event *event_ptr) override { return IsShouldStopSignal(); }
// If should stop returns false, check if we should notify of this event
bool DoShouldNotify(Event *event_ptr) override {
@@ -1137,9 +1132,18 @@ class StopInfoUnixSignal : public StopInfo {
return m_description.c_str();
}
+ bool ShouldSelect() const override { return IsShouldStopSignal(); }
+
private:
// In siginfo_t terms, if m_value is si_signo, m_code is si_code.
std::optional<int> m_code;
+
+ bool IsShouldStopSignal() const {
+ ThreadSP thread_sp(m_thread_wp.lock());
+ if (thread_sp)
+ return thread_sp->GetProcess()->GetUnixSignals()->GetShouldStop(m_value);
+ return false;
+ }
};
// StopInfoInterrupt
|
jimingham
reviewed
Apr 2, 2025
lldb/include/lldb/Target/StopInfo.h
Outdated
| @@ -118,6 +118,19 @@ class StopInfo : public std::enable_shared_from_this<StopInfo> { | |||
|
|
|||
| StructuredData::ObjectSP GetExtendedInfo() { return m_extended_info; } | |||
|
|
|||
| /// Returns true if this is a stop reason that should be shown to a user when | |||
| /// stopping. | |||
There was a problem hiding this comment.
stopping -> viewing the thread with this stop info
This happens not just at stop.
JDevlieghere
reviewed
Apr 2, 2025
lldb/source/Target/StopInfo.cpp
Outdated
Comment on lines
1142
to
1144
| ThreadSP thread_sp(m_thread_wp.lock()); | ||
| if (thread_sp) | ||
| return thread_sp->GetProcess()->GetUnixSignals()->GetShouldStop(m_value); |
There was a problem hiding this comment.
Suggested change
| ThreadSP thread_sp(m_thread_wp.lock()); | |
| if (thread_sp) | |
| return thread_sp->GetProcess()->GetUnixSignals()->GetShouldStop(m_value); | |
| ThreadSP thread_sp(m_thread_wp.lock()); | |
| if (ThreadSP thread_sp = m_thread_wp.lock()) | |
| return thread_sp->GetProcess()->GetUnixSignals()->GetShouldStop(m_value); |
|
Addressed review comments |
JDevlieghere
approved these changes
Apr 3, 2025
lldb/source/Target/Process.cpp
Outdated
Comment on lines
831
to
832
| } else if (!other_thread) | ||
| other_thread = thread; |
There was a problem hiding this comment.
The LLVM Style Guide says to "[u]se braces for the else if and else block to keep it uniform with the if block."
Suggested change
| } else if (!other_thread) | |
| other_thread = thread; | |
| } else if (!other_thread) { | |
| other_thread = thread; | |
| } |
felipepiovezan
added a commit
to felipepiovezan/llvm-project
that referenced
this pull request
Apr 4, 2025
…4160) This NFC patch simplifies the main loop in HandleProcessStateChanged event by moving duplicated code into the StopInfo class, also allowing StopInfo subclasses to override behavior. More specifically, two functions are created: * ShouldShow: should a Thread with such StopInfo should be printed when the debugger stops? Currently, no StopInfo subclasses override this, but a subsequent patch will fix a bug by making StopInfoBreakpoint check whether the breakpoint is internal. * ShouldSelect: should a Thread with such a StopInfo be selected? This is currently overridden by StopInfoUnixSignal but will, in the future, be overridden by StopInfoBreakpoint. (cherry-picked from commit c14b6e9)
felipepiovezan
added a commit
to felipepiovezan/llvm-project
that referenced
this pull request
Apr 15, 2025
…4160) This NFC patch simplifies the main loop in HandleProcessStateChanged event by moving duplicated code into the StopInfo class, also allowing StopInfo subclasses to override behavior. More specifically, two functions are created: * ShouldShow: should a Thread with such StopInfo should be printed when the debugger stops? Currently, no StopInfo subclasses override this, but a subsequent patch will fix a bug by making StopInfoBreakpoint check whether the breakpoint is internal. * ShouldSelect: should a Thread with such a StopInfo be selected? This is currently overridden by StopInfoUnixSignal but will, in the future, be overridden by StopInfoBreakpoint. (cherry-picked from commit c14b6e9)
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.
This NFC patch simplifies the main loop in HandleProcessStateChanged event by moving duplicated code into the StopInfo class, also allowing StopInfo subclasses to override behavior.
More specifically, two functions are created:
ShouldShow: should a Thread with such StopInfo should be printed when the debugger stops? Currently, no StopInfo subclasses override this, but a subsequent patch will fix a bug by making StopInfoBreakpoint check whether the breakpoint is internal.
ShouldSelect: should a Thread with such a StopInfo be selected? This is currently overridden by StopInfoUnixSignal but will, in the future, be overridden by StopInfoBreakpoint.