[lldb][nfc] Add customization flags for ThreadPlanStepOut #135866
Conversation
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesThreadPlanStepOut always skips over Hidden/Artificial frames when computing its destination frame, without providing any customization of this behavior. This is problematic for some plans like StepThrough, which may need to step out of a frame without stepping out of a Hidden/Artificial frame. Any first step towards fixing this requires the ability to customize ThreadPlanStepOut, which is what this NFC patch addresses. Since the computation of which frames to skip is done by the constructor, this patch adds a Flags parameter to Full diff: https://github.com/llvm/llvm-project/pull/135866.diff 3 Files Affected:
diff --git a/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h b/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h
index d0094c90b91a5..e712ee8d2d94f 100644
--- a/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h
+++ b/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h
@@ -60,7 +60,9 @@ class ThreadPlanShouldStopHere {
eAvoidInlines = (1 << 0),
eStepInAvoidNoDebug = (1 << 1),
eStepOutAvoidNoDebug = (1 << 2),
- eStepOutPastThunks = (1 << 3)
+ eStepOutPastThunks = (1 << 3),
+ eStepOutPastHiddenFunctions = (1 << 4),
+ eStepOutPastArtificialFunctions = (1 << 5),
};
// Constructors and Destructors
diff --git a/lldb/include/lldb/Target/ThreadPlanStepOut.h b/lldb/include/lldb/Target/ThreadPlanStepOut.h
index 013c675afc33d..c8eb334e4311c 100644
--- a/lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ b/lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -17,12 +17,12 @@ namespace lldb_private {
class ThreadPlanStepOut : public ThreadPlan, public ThreadPlanShouldStopHere {
public:
- ThreadPlanStepOut(Thread &thread, SymbolContext *addr_context,
- bool first_insn, bool stop_others, Vote report_stop_vote,
- Vote report_run_vote, uint32_t frame_idx,
- LazyBool step_out_avoids_code_without_debug_info,
- bool continue_to_next_branch = false,
- bool gather_return_value = true);
+ ThreadPlanStepOut(
+ Thread &thread, SymbolContext *addr_context, bool first_insn,
+ bool stop_others, Vote report_stop_vote, Vote report_run_vote,
+ uint32_t frame_idx, LazyBool step_out_avoids_code_without_debug_info,
+ bool continue_to_next_branch = false, bool gather_return_value = true,
+ lldb_private::Flags flags = ThreadPlanStepOut::s_default_flag_values);
~ThreadPlanStepOut() override;
@@ -87,6 +87,9 @@ class ThreadPlanStepOut : public ThreadPlan, public ThreadPlanShouldStopHere {
void CalculateReturnValue();
+ lldb::StackFrameSP
+ ComputeReturnToFrame(Thread &thread, uint32_t start_frame_idx, Flags flags);
+
ThreadPlanStepOut(const ThreadPlanStepOut &) = delete;
const ThreadPlanStepOut &operator=(const ThreadPlanStepOut &) = delete;
};
diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index a05c46db6b8ca..ef060b376407d 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -29,14 +29,44 @@
using namespace lldb;
using namespace lldb_private;
-uint32_t ThreadPlanStepOut::s_default_flag_values = 0;
+uint32_t ThreadPlanStepOut::s_default_flag_values =
+ eStepOutPastArtificialFunctions | eStepOutPastHiddenFunctions;
+
+StackFrameSP ThreadPlanStepOut::ComputeReturnToFrame(Thread &thread,
+ uint32_t start_frame_idx,
+ Flags flags) {
+ uint32_t frame_idx = start_frame_idx + 1;
+ StackFrameSP return_frame_sp = thread.GetStackFrameAtIndex(frame_idx);
+ if (!return_frame_sp)
+ return nullptr;
+
+ // If asked to, step out past artificial/hidden frames.
+ while ((flags.Test(eStepOutPastArtificialFunctions) &&
+ return_frame_sp->IsArtificial()) ||
+ (flags.Test(eStepOutPastHiddenFunctions) &&
+ return_frame_sp->IsHidden())) {
+ m_stepped_past_frames.push_back(return_frame_sp);
+
+ frame_idx++;
+ return_frame_sp = thread.GetStackFrameAtIndex(frame_idx);
+
+ // We never expect to see an artificial frame without a regular ancestor.
+ // Defensively refuse to step out.
+ if (!return_frame_sp) {
+ LLDB_LOG(GetLog(LLDBLog::Step),
+ "Can't step out of frame with artificial ancestors");
+ return nullptr;
+ }
+ }
+ return return_frame_sp;
+}
// ThreadPlanStepOut: Step out of the current frame
ThreadPlanStepOut::ThreadPlanStepOut(
Thread &thread, SymbolContext *context, bool first_insn, bool stop_others,
Vote report_stop_vote, Vote report_run_vote, uint32_t frame_idx,
LazyBool step_out_avoids_code_without_debug_info,
- bool continue_to_next_branch, bool gather_return_value)
+ bool continue_to_next_branch, bool gather_return_value, Flags flags)
: ThreadPlan(ThreadPlan::eKindStepOut, "Step out", thread, report_stop_vote,
report_run_vote),
ThreadPlanShouldStopHere(this), m_step_from_insn(LLDB_INVALID_ADDRESS),
@@ -44,34 +74,18 @@ ThreadPlanStepOut::ThreadPlanStepOut(
m_return_addr(LLDB_INVALID_ADDRESS), m_stop_others(stop_others),
m_immediate_step_from_function(nullptr),
m_calculate_return_value(gather_return_value) {
- Log *log = GetLog(LLDBLog::Step);
- SetFlagsToDefault();
+ m_flags = flags;
SetupAvoidNoDebug(step_out_avoids_code_without_debug_info);
m_step_from_insn = thread.GetRegisterContext()->GetPC(0);
- uint32_t return_frame_index = frame_idx + 1;
- StackFrameSP return_frame_sp(thread.GetStackFrameAtIndex(return_frame_index));
+ StackFrameSP return_frame_sp =
+ ComputeReturnToFrame(thread, frame_idx, m_flags);
StackFrameSP immediate_return_from_sp(thread.GetStackFrameAtIndex(frame_idx));
if (!return_frame_sp || !immediate_return_from_sp)
return; // we can't do anything here. ValidatePlan() will return false.
- // While stepping out, behave as-if artificial frames are not present.
- while (return_frame_sp->IsArtificial() || return_frame_sp->IsHidden()) {
- m_stepped_past_frames.push_back(return_frame_sp);
-
- ++return_frame_index;
- return_frame_sp = thread.GetStackFrameAtIndex(return_frame_index);
-
- // We never expect to see an artificial frame without a regular ancestor.
- // If this happens, log the issue and defensively refuse to step out.
- if (!return_frame_sp) {
- LLDB_LOG(log, "Can't step out of frame with artificial ancestors");
- return;
- }
- }
-
m_step_out_to_id = return_frame_sp->GetStackID();
m_immediate_step_from_id = immediate_return_from_sp->GetStackID();
@@ -125,6 +139,7 @@ ThreadPlanStepOut::ThreadPlanStepOut(
// Perform some additional validation on the return address.
uint32_t permissions = 0;
+ Log *log = GetLog(LLDBLog::Step);
if (!m_process.GetLoadAddressPermissions(m_return_addr, permissions)) {
LLDB_LOGF(log, "ThreadPlanStepOut(%p): Return address (0x%" PRIx64
") permissions not found.", static_cast<void *>(this),
|
ThreadPlanStepOut always skips over Hidden/Artificial frames when computing its destination frame, without providing any customization of this behavior. This is problematic for some plans like StepThrough, which may need to step out of a frame _without_ stepping out of a Hidden/Artificial frame. Any first step towards fixing this requires the ability to customize ThreadPlanStepOut, which is what this NFC patch addresses. Since the computation of which frames to skip is done by the constructor, this patch adds a Flags parameter to `ThreadPlanStepOut::ThreadPlanStepOut`.
felipepiovezan
force-pushed
the
felipe/configure_step_out
branch
from
166d032 to
3804a62
Compare
There was a problem hiding this comment.
My original notion was that instead of passing in flags to the setup of ThreadPlanStepOut and having it compute where it should go based on that, the plan that was deciding to step out would call ThreadPlanShouldStopHere::CheckShouldStopHereAndQueueStepOut, and that would figure out based on the flags of that plan what frame it should step out to, then it would queue up a ThreadPlanStepOut that would go to that frame.
In that version ComputeTargetFrame would be logic in the StepOutFromHereCallback. I was trying to keep all the strategy decisions about where to go in the ShouldStopHere callbacks so that they could be reused more easily.
In this version, the StepOutFromHere callback figures out which flags it wants to use, and then passes them to the ThreadPlanStepOut, which computes where to go. I'm not sure that makes much difference; once we're deciding to go to older frames on the stack we're really only going to use a ThreadPlanStepOut, so it doesn't much matter where that logic is.
LGTM
To elaborate on this, today there is no constructor for StepOut that allows one to specify a target frame. This is also something we would need to create for that to work, right? |
The main constructor for ThreadStepOut is: The |
I don't think this is quite true, that parameter indicates where we step out from. I know this distinction may look unimportant at first, but it is exactly that distinction that gives the constructor space to skip artificial/hidden frames. The frame we step out to is initially computed as |
|
That's what QueueThreadPlanStepOutNoShouldStop is supposed to do, maybe it's not working? |
|
That one should just do "Go where I tell you, then stop". |
There is only one constructor for step out, and it always skips those frames. So, yes, even in |
In fact that is what the default callback uses... |
ThreadPlanStepOut always skips over Hidden/Artificial frames when computing its destination frame, without providing any customization of this behavior. This is problematic for some plans like StepThrough, which may need to step out of a frame without stepping out of a Hidden/Artificial frame. Any first step towards fixing this requires the ability to customize ThreadPlanStepOut, which is what this NFC patch addresses.
Since the computation of which frames to skip is done by the constructor, this patch adds a Flags parameter to
ThreadPlanStepOut::ThreadPlanStepOut.