[Lldb-commits] [lldb] [lldb][nfc] Split the constructor of ThreadPlanStepOut (PR #136160)

[Felipe de Azevedo Piovezan via lldb-commits]

https://github.com/felipepiovezan updated https://github.com/llvm/llvm-project/pull/136160

>From 42850d46b705c239b099be8d102a3756a8fd5283 Mon Sep 17 00:00:00 2001
From: Felipe de Azevedo Piovezan <fpiovezan at apple.com>
Date: Tue, 15 Apr 2025 10:20:41 -0700
Subject: [PATCH 1/3] [lldb][nfc] Factor out code from ThreadPlanStepOut ctor

A future patch will need to create a new constructor for this class, and
extracting code out of its sole existing constructor will make this
easier.

This commit creates a helper function for the code computing the target
frame to step out to.
---
 lldb/source/Target/ThreadPlanStepOut.cpp | 47 +++++++++++++++---------
 1 file changed, 29 insertions(+), 18 deletions(-)

diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index a05c46db6b8ca..26c1abe710319 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -31,6 +31,32 @@ using namespace lldb_private;
 
 uint32_t ThreadPlanStepOut::s_default_flag_values = 0;
 
+/// Computes the target frame this plan should step out to.
+static StackFrameSP
+ComputeTargetFrame(Thread &thread, uint32_t start_frame_idx,
+                   std::vector<StackFrameSP> &skipped_frames) {
+  uint32_t frame_idx = start_frame_idx + 1;
+  StackFrameSP return_frame_sp = thread.GetStackFrameAtIndex(frame_idx);
+  if (!return_frame_sp)
+    return nullptr;
+
+  while (return_frame_sp->IsArtificial() || return_frame_sp->IsHidden()) {
+    skipped_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,
@@ -44,34 +70,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();
   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 =
+      ComputeTargetFrame(thread, frame_idx, m_stepped_past_frames);
   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 +135,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),

>From 662baea555175e39ca4f0f170f2ef197c53af60d Mon Sep 17 00:00:00 2001
From: Felipe de Azevedo Piovezan <fpiovezan at apple.com>
Date: Thu, 17 Apr 2025 07:52:19 -0700
Subject: [PATCH 2/3] [lldb][nfc] Split the constructor of ThreadPlanStepOut

A subsequent commit will create a new constructor for ThreadPlanStepOut,
which needs to reuse much of the same logic of the existing constructor.
This commit places all of that reusable logic into a separate function.
---
 lldb/include/lldb/Target/ThreadPlanStepOut.h | 4 ++++
 lldb/source/Target/ThreadPlanStepOut.cpp     | 9 ++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/lldb/include/lldb/Target/ThreadPlanStepOut.h b/lldb/include/lldb/Target/ThreadPlanStepOut.h
index 013c675afc33d..e414a6e0a2d49 100644
--- a/lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ b/lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -82,6 +82,10 @@ class ThreadPlanStepOut : public ThreadPlan, public ThreadPlanShouldStopHere {
       LazyBool step_out_avoids_code_without_debug_info);
 
   void SetupAvoidNoDebug(LazyBool step_out_avoids_code_without_debug_info);
+
+  void SetupReturnAddress(lldb::StackFrameSP return_frame_sp,
+                          lldb::StackFrameSP immediate_return_from_sp,
+                          uint32_t frame_idx, bool continue_to_next_branch);
   // Need an appropriate marker for the current stack so we can tell step out
   // from step in.
 
diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index 26c1abe710319..b3c9a790487d4 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -79,6 +79,13 @@ ThreadPlanStepOut::ThreadPlanStepOut(
       ComputeTargetFrame(thread, frame_idx, m_stepped_past_frames);
   StackFrameSP immediate_return_from_sp(thread.GetStackFrameAtIndex(frame_idx));
 
+  SetupReturnAddress(return_frame_sp, immediate_return_from_sp,
+                     frame_idx, continue_to_next_branch);
+}
+
+void ThreadPlanStepOut::SetupReturnAddress(
+    StackFrameSP return_frame_sp, StackFrameSP immediate_return_from_sp,
+    uint32_t frame_idx, bool continue_to_next_branch) {
   if (!return_frame_sp || !immediate_return_from_sp)
     return; // we can't do anything here.  ValidatePlan() will return false.
 
@@ -94,7 +101,7 @@ ThreadPlanStepOut::ThreadPlanStepOut(
       // First queue a plan that gets us to this inlined frame, and when we get
       // there we'll queue a second plan that walks us out of this frame.
       m_step_out_to_inline_plan_sp = std::make_shared<ThreadPlanStepOut>(
-          thread, nullptr, false, stop_others, eVoteNoOpinion, eVoteNoOpinion,
+          GetThread(), nullptr, false, m_stop_others, eVoteNoOpinion, eVoteNoOpinion,
           frame_idx - 1, eLazyBoolNo, continue_to_next_branch);
       static_cast<ThreadPlanStepOut *>(m_step_out_to_inline_plan_sp.get())
           ->SetShouldStopHereCallbacks(nullptr, nullptr);

>From 0a2dc4a280d83ca35bc4458ecb939f96e4dee8af Mon Sep 17 00:00:00 2001
From: Felipe de Azevedo Piovezan <fpiovezan at apple.com>
Date: Thu, 17 Apr 2025 11:34:39 -0700
Subject: [PATCH 3/3] fixup! Run clang-format

---
 lldb/source/Target/ThreadPlanStepOut.cpp | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index b3c9a790487d4..f2606403016a6 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -79,8 +79,8 @@ ThreadPlanStepOut::ThreadPlanStepOut(
       ComputeTargetFrame(thread, frame_idx, m_stepped_past_frames);
   StackFrameSP immediate_return_from_sp(thread.GetStackFrameAtIndex(frame_idx));
 
-  SetupReturnAddress(return_frame_sp, immediate_return_from_sp,
-                     frame_idx, continue_to_next_branch);
+  SetupReturnAddress(return_frame_sp, immediate_return_from_sp, frame_idx,
+                     continue_to_next_branch);
 }
 
 void ThreadPlanStepOut::SetupReturnAddress(
@@ -101,8 +101,8 @@ void ThreadPlanStepOut::SetupReturnAddress(
       // First queue a plan that gets us to this inlined frame, and when we get
       // there we'll queue a second plan that walks us out of this frame.
       m_step_out_to_inline_plan_sp = std::make_shared<ThreadPlanStepOut>(
-          GetThread(), nullptr, false, m_stop_others, eVoteNoOpinion, eVoteNoOpinion,
-          frame_idx - 1, eLazyBoolNo, continue_to_next_branch);
+          GetThread(), nullptr, false, m_stop_others, eVoteNoOpinion,
+          eVoteNoOpinion, frame_idx - 1, eLazyBoolNo, continue_to_next_branch);
       static_cast<ThreadPlanStepOut *>(m_step_out_to_inline_plan_sp.get())
           ->SetShouldStopHereCallbacks(nullptr, nullptr);
       m_step_out_to_inline_plan_sp->SetPrivate(true);