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 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
