[lldb] Consider "hidden" frames in ThreadPlanShouldStopHere

[felipepiovezan]

Patch 1 introduced the notion of "hidden" frames, which are recognized by language plugins and can be hidden in backtraces. They also affect stepping out: when stepping out of a frame, if its parent frame is "hidden", then the debugger steps out to the parent's parent.

However, this is problematic when stepping out is done as part of a larger plan. For example, when stepping through, the debugger is often in some frame A, then pushes some frame B, and decides to step out of B and back to A. If A is a hidden frame, LLDB currenly steps out past it, which is not what the step through plan intended.

To address this issue, we create a new flag for the ShouldStopHere base class to allow for stepping past hidden frames. This flag is now the default for stepping out. Furthermore,
ThreadPlanShouldStopHere::DefaultStepFromHereCallback now passes its own set of flags to the constructor of ThreadPlanStepOut.

One potentially controversial choice: all StepOut plans created through ThreadPlanShouldStopHere::DefaultStepFromHereCallback will pass on their set of flags to ThreadPlanStepOut, which will disable the behavior of stepping past hidden frames.

rdar://147065566

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

Patch 1 introduced the notion of "hidden" frames, which are recognized by language plugins and can be hidden in backtraces. They also affect stepping out: when stepping out of a frame, if its parent frame is "hidden", then the debugger steps out to the parent's parent.

However, this is problematic when stepping out is done as part of a larger plan. For example, when stepping through, the debugger is often in some frame A, then pushes some frame B, and decides to step out of B and back to A. If A is a hidden frame, LLDB currenly steps out past it, which is not what the step through plan intended.

To address this issue, we create a new flag for the ShouldStopHere base class to allow for stepping past hidden frames. This flag is now the default for stepping out. Furthermore,
ThreadPlanShouldStopHere::DefaultStepFromHereCallback now passes its own set of flags to the constructor of ThreadPlanStepOut.

One potentially controversial choice: all StepOut plans created through ThreadPlanShouldStopHere::DefaultStepFromHereCallback will pass on their set of flags to ThreadPlanStepOut, which will disable the behavior of stepping past hidden frames.

---

Full diff: https://github.com/llvm/llvm-project/pull/131800.diff

6 Files Affected:

• (modified) lldb/include/lldb/Target/Thread.h (+2-1)
• (modified) lldb/include/lldb/Target/ThreadPlanShouldStopHere.h (+2-1)
• (modified) lldb/include/lldb/Target/ThreadPlanStepOut.h (+2-1)
• (modified) lldb/source/Target/Thread.cpp (+3-2)
• (modified) lldb/source/Target/ThreadPlanShouldStopHere.cpp (+1-1)
• (modified) lldb/source/Target/ThreadPlanStepOut.cpp (+10-4)

diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 6ede7fa301a82..3c6b9791faeee 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -942,7 +942,8 @@ class Thread : public std::enable_shared_from_this<Thread>,
   virtual lldb::ThreadPlanSP QueueThreadPlanForStepOutNoShouldStop(
       bool abort_other_plans, SymbolContext *addr_context, bool first_insn,
       bool stop_other_threads, Vote report_stop_vote, Vote report_run_vote,
-      uint32_t frame_idx, Status &status, bool continue_to_next_branch = false);
+      uint32_t frame_idx, Status &status, bool continue_to_next_branch = false,
+      const Flags *flags = nullptr);
 
   /// Gets the plan used to step through the code that steps from a function
   /// call site at the current PC into the actual function call.
diff --git a/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h b/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h
index d0094c90b91a5..8de53465010a9 100644
--- a/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h
+++ b/lldb/include/lldb/Target/ThreadPlanShouldStopHere.h
@@ -60,7 +60,8 @@ class ThreadPlanShouldStopHere {
     eAvoidInlines = (1 << 0),
     eStepInAvoidNoDebug = (1 << 1),
     eStepOutAvoidNoDebug = (1 << 2),
-    eStepOutPastThunks = (1 << 3)
+    eStepOutPastThunks = (1 << 3),
+    eStepOutPastHiddenFunctions = (1 << 4),
   };
 
   // Constructors and Destructors
diff --git a/lldb/include/lldb/Target/ThreadPlanStepOut.h b/lldb/include/lldb/Target/ThreadPlanStepOut.h
index 013c675afc33d..9f3138686a53e 100644
--- a/lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ b/lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -22,7 +22,8 @@ class ThreadPlanStepOut : public ThreadPlan, public ThreadPlanShouldStopHere {
                     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);
+                    bool gather_return_value = true,
+                    const Flags *flags = nullptr);
 
   ~ThreadPlanStepOut() override;
 
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index accc4708c24e1..191fc05fa7e5a 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -1356,13 +1356,14 @@ ThreadPlanSP Thread::QueueThreadPlanForStepOut(
 ThreadPlanSP Thread::QueueThreadPlanForStepOutNoShouldStop(
     bool abort_other_plans, SymbolContext *addr_context, bool first_insn,
     bool stop_other_threads, Vote report_stop_vote, Vote report_run_vote,
-    uint32_t frame_idx, Status &status, bool continue_to_next_branch) {
+    uint32_t frame_idx, Status &status, bool continue_to_next_branch,
+    const Flags *flags) {
   const bool calculate_return_value =
       false; // No need to calculate the return value here.
   ThreadPlanSP thread_plan_sp(new ThreadPlanStepOut(
       *this, addr_context, first_insn, stop_other_threads, report_stop_vote,
       report_run_vote, frame_idx, eLazyBoolNo, continue_to_next_branch,
-      calculate_return_value));
+      calculate_return_value, flags));
 
   ThreadPlanStepOut *new_plan =
       static_cast<ThreadPlanStepOut *>(thread_plan_sp.get());
diff --git a/lldb/source/Target/ThreadPlanShouldStopHere.cpp b/lldb/source/Target/ThreadPlanShouldStopHere.cpp
index d2cca49987f0f..8c58bb92ac41b 100644
--- a/lldb/source/Target/ThreadPlanShouldStopHere.cpp
+++ b/lldb/source/Target/ThreadPlanShouldStopHere.cpp
@@ -177,7 +177,7 @@ ThreadPlanSP ThreadPlanShouldStopHere::DefaultStepFromHereCallback(
     return_plan_sp =
         current_plan->GetThread().QueueThreadPlanForStepOutNoShouldStop(
             false, nullptr, true, stop_others, eVoteNo, eVoteNoOpinion,
-            frame_index, status, true);
+            frame_index, status, true, &flags);
   return return_plan_sp;
 }
 
diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index a05c46db6b8ca..738c019466acf 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -29,14 +29,15 @@
 using namespace lldb;
 using namespace lldb_private;
 
-uint32_t ThreadPlanStepOut::s_default_flag_values = 0;
+uint32_t ThreadPlanStepOut::s_default_flag_values =
+    ThreadPlanShouldStopHere::eStepOutPastHiddenFunctions;
 
 // 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, const Flags *flags)
     : ThreadPlan(ThreadPlan::eKindStepOut, "Step out", thread, report_stop_vote,
                  report_run_vote),
       ThreadPlanShouldStopHere(this), m_step_from_insn(LLDB_INVALID_ADDRESS),
@@ -45,7 +46,10 @@ ThreadPlanStepOut::ThreadPlanStepOut(
       m_immediate_step_from_function(nullptr),
       m_calculate_return_value(gather_return_value) {
   Log *log = GetLog(LLDBLog::Step);
-  SetFlagsToDefault();
+  if (flags)
+    m_flags = *flags;
+  else
+    SetFlagsToDefault();
   SetupAvoidNoDebug(step_out_avoids_code_without_debug_info);
 
   m_step_from_insn = thread.GetRegisterContext()->GetPC(0);
@@ -58,7 +62,9 @@ ThreadPlanStepOut::ThreadPlanStepOut(
     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()) {
+  while (return_frame_sp->IsArtificial() ||
+         (m_flags.Test(eStepOutPastHiddenFunctions) &&
+          return_frame_sp->IsHidden())) {
     m_stepped_past_frames.push_back(return_frame_sp);
 
     ++return_frame_index;

[jimingham]

I don't think it's necessary to turn off stepping past hidden frames with this patch. You need to get the ThreadPlanStepInRange's SetDefaultFlags to add eStepOutPastHiddenFunctions by or-ing that into the ThreadPlanStepInRange::s_default_flag_values. That will handle the case where we step in through a hidden function, decide we aren't interested in stopping there and step back out again.

Regular user level step-out's will use QueueThreadPlanForStepOut, not the NoShouldStop version. So they will always set the flags to the ThreadPlanStepOut::s_default_flags, though maybe that should be

eStepOutPastHiddenFunctions | eStepOutPastThunks