[Lldb-commits] [lldb] [lldb][NFC] Create StackID::IsYounger (PR #209402)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

<details>
<summary>Changes</summary>

Currently,  StackIDs are compared with a custom operator <. There are some issues with that:
1. It's not clear what "<" means in this context. It really is a test of "frame A is on top of frame B or not comparable". But this notion is not expressed through "<".
2. This is not a real operator "<" in the sense that if "!<"  does not imply ">=". In particular, frames may not always be comparable (i.e. (they are not part of the same stack).

To address these issues, this commit replaces `<` with a custom function "IsYounger", which makes explicit what is being tested.

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


7 Files Affected:

- (modified) lldb/include/lldb/Target/StackID.h (+5-4) 
- (modified) lldb/source/Target/StackFrameList.cpp (+1-1) 
- (modified) lldb/source/Target/StackID.cpp (+1-1) 
- (modified) lldb/source/Target/ThreadPlanStepInstruction.cpp (+3-2) 
- (modified) lldb/source/Target/ThreadPlanStepOut.cpp (+4-4) 
- (modified) lldb/source/Target/ThreadPlanStepRange.cpp (+1-1) 
- (modified) lldb/source/Target/ThreadPlanStepUntil.cpp (+2-2) 


``````````diff
diff --git a/lldb/include/lldb/Target/StackID.h b/lldb/include/lldb/Target/StackID.h
index 3f6a83b5e2fa5..2292bef5e49be 100644
--- a/lldb/include/lldb/Target/StackID.h
+++ b/lldb/include/lldb/Target/StackID.h
@@ -50,6 +50,11 @@ class StackID {
 
   void Dump(Stream *s);
 
+  /// Returns true if `lhs` corresponds to a frame younger (i.e. higher on the
+  /// call stack) than `rhs`, and false otherwise (including when the frames are
+  /// not comparable).
+  static bool IsYounger(const StackID &lhs, const StackID &rhs);
+
 protected:
   friend class StackFrame;
   friend class SyntheticStackFrameList;
@@ -80,10 +85,6 @@ class StackID {
 
 bool operator==(const StackID &lhs, const StackID &rhs);
 bool operator!=(const StackID &lhs, const StackID &rhs);
-
-// frame_id_1 < frame_id_2 means "frame_id_1 is YOUNGER than frame_id_2"
-bool operator<(const StackID &lhs, const StackID &rhs);
-
 } // namespace lldb_private
 
 #endif // LLDB_TARGET_STACKID_H
diff --git a/lldb/source/Target/StackFrameList.cpp b/lldb/source/Target/StackFrameList.cpp
index 465d1af11add0..b124ef1c199c9 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -762,7 +762,7 @@ StackFrameList::GetFrameWithConcreteFrameIndex(uint32_t unwind_idx) {
 
 static bool CompareStackID(const StackFrameSP &stack_sp,
                            const StackID &stack_id) {
-  return stack_sp->GetStackID() < stack_id;
+  return StackID::IsYounger(stack_sp->GetStackID(), stack_id);
 }
 
 StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
diff --git a/lldb/source/Target/StackID.cpp b/lldb/source/Target/StackID.cpp
index 137c776a84d2f..c5d8e5a3e6644 100644
--- a/lldb/source/Target/StackID.cpp
+++ b/lldb/source/Target/StackID.cpp
@@ -69,7 +69,7 @@ bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
   return !(lhs == rhs);
 }
 
-bool lldb_private::operator<(const StackID &lhs, const StackID &rhs) {
+bool StackID::IsYounger(const StackID &lhs, const StackID &rhs) {
   const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddressWithoutMetadata();
   const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddressWithoutMetadata();
 
diff --git a/lldb/source/Target/ThreadPlanStepInstruction.cpp b/lldb/source/Target/ThreadPlanStepInstruction.cpp
index e9db3171a5bef..0e062bf6d7a34 100644
--- a/lldb/source/Target/ThreadPlanStepInstruction.cpp
+++ b/lldb/source/Target/ThreadPlanStepInstruction.cpp
@@ -110,7 +110,7 @@ bool ThreadPlanStepInstruction::IsPlanStale() {
       SetPlanComplete();
     }
     return (thread.GetRegisterContext()->GetPC(0) != m_instruction_addr);
-  } else if (cur_frame_id < m_stack_id) {
+  } else if (StackID::IsYounger(cur_frame_id, m_stack_id)) {
     // If the current frame is younger than the start frame and we are stepping
     // over, then we need to continue, but if we are doing just one step, we're
     // done.
@@ -137,7 +137,8 @@ bool ThreadPlanStepInstruction::ShouldStop(Event *event_ptr) {
 
     StackID cur_frame_zero_id = cur_frame_sp->GetStackID();
 
-    if (cur_frame_zero_id == m_stack_id || m_stack_id < cur_frame_zero_id) {
+    if (cur_frame_zero_id == m_stack_id ||
+        StackID::IsYounger(m_stack_id, cur_frame_zero_id)) {
       if (thread.GetRegisterContext()->GetPC(0) != m_instruction_addr) {
         if (--m_iteration_count <= 0) {
           SetPlanComplete();
diff --git a/lldb/source/Target/ThreadPlanStepOut.cpp b/lldb/source/Target/ThreadPlanStepOut.cpp
index 7af4de6e71ec1..9d830f13947c7 100644
--- a/lldb/source/Target/ThreadPlanStepOut.cpp
+++ b/lldb/source/Target/ThreadPlanStepOut.cpp
@@ -341,12 +341,12 @@ bool ThreadPlanStepOut::DoPlanExplainsStop(Event *event_ptr) {
 
         if (m_step_out_to_id == frame_zero_id)
           done = true;
-        else if (m_step_out_to_id < frame_zero_id) {
+        else if (StackID::IsYounger(m_step_out_to_id, frame_zero_id)) {
           // Either we stepped past the breakpoint, or the stack ID calculation
           // was incorrect and we should probably stop.
           done = true;
         } else {
-          done = (m_immediate_step_from_id < frame_zero_id);
+          done = (StackID::IsYounger(m_immediate_step_from_id, frame_zero_id));
         }
 
         if (done) {
@@ -405,7 +405,7 @@ bool ThreadPlanStepOut::ShouldStop(Event *event_ptr) {
     StopInfoSP stop_info_sp = GetPrivateStopInfo();
     if (stop_info_sp && stop_info_sp->GetStopReason() == eStopReasonBreakpoint) {
       StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
-      done = !(frame_zero_id < m_step_out_to_id);
+      done = !(StackID::IsYounger(frame_zero_id, m_step_out_to_id));
     }
   }
 
@@ -565,5 +565,5 @@ bool ThreadPlanStepOut::IsPlanStale() {
   // then there's something for us to do.  Otherwise, we're stale.
 
   StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
-  return !(frame_zero_id < m_step_out_to_id);
+  return !(StackID::IsYounger(frame_zero_id, m_step_out_to_id));
 }
diff --git a/lldb/source/Target/ThreadPlanStepRange.cpp b/lldb/source/Target/ThreadPlanStepRange.cpp
index b57754a180e72..00f5d70ee00ff 100644
--- a/lldb/source/Target/ThreadPlanStepRange.cpp
+++ b/lldb/source/Target/ThreadPlanStepRange.cpp
@@ -223,7 +223,7 @@ lldb::FrameComparison ThreadPlanStepRange::CompareCurrentFrameToStartFrame() {
 
   if (cur_frame_id == m_stack_id) {
     frame_order = eFrameCompareEqual;
-  } else if (cur_frame_id < m_stack_id) {
+  } else if (StackID::IsYounger(cur_frame_id, m_stack_id)) {
     frame_order = eFrameCompareYounger;
   } else {
     StackFrameSP cur_parent_frame = thread.GetStackFrameAtIndex(1);
diff --git a/lldb/source/Target/ThreadPlanStepUntil.cpp b/lldb/source/Target/ThreadPlanStepUntil.cpp
index db2b930c1085c..43d068162c3b2 100644
--- a/lldb/source/Target/ThreadPlanStepUntil.cpp
+++ b/lldb/source/Target/ThreadPlanStepUntil.cpp
@@ -173,7 +173,7 @@ void ThreadPlanStepUntil::AnalyzeStop() {
         bool done;
         StackID cur_frame_zero_id;
 
-        done = (m_stack_id < cur_frame_zero_id);
+        done = (StackID::IsYounger(m_stack_id, cur_frame_zero_id));
 
         if (done) {
           m_stepped_out = true;
@@ -199,7 +199,7 @@ void ThreadPlanStepUntil::AnalyzeStop() {
 
             if (frame_zero_id == m_stack_id)
               done = true;
-            else if (frame_zero_id < m_stack_id)
+            else if (StackID::IsYounger(frame_zero_id, m_stack_id))
               done = false;
             else {
               StackFrameSP older_frame_sp = thread.GetStackFrameAtIndex(1);

``````````

</details>


https://github.com/llvm/llvm-project/pull/209402