tatyana-krasnukha created this revision.
tatyana-krasnukha added reviewers: labath, jasonmolenda.
tatyana-krasnukha added a project: LLDB.
Herald added a subscriber: JDevlieghere.
tatyana-krasnukha requested review of this revision.
Herald added a subscriber: lldb-commits.
"false" result of the operator can imply not only "the frame is not younger".
When CFAs are equal, StackID's operator "<" can only compare symbol contexts of
the same function. Otherwise, it also returns false.
In the case I described in D114861 <https://reviews.llvm.org/D114861>, two
different frames can have the same CFA, and then the operator's result may be
incorrect. "thread step-*" commands get broken in this case.
This patch replaces the operator that can return only boolean value with the
function that returns a value of lldb::FrameComparison type. It also updates
thread plans to use this function instead of the operator.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D114862
Files:
lldb/include/lldb/Target/StackID.h
lldb/include/lldb/Target/ThreadPlanStepOut.h
lldb/source/Target/StackFrameList.cpp
lldb/source/Target/StackID.cpp
lldb/source/Target/ThreadPlanStepInstruction.cpp
lldb/source/Target/ThreadPlanStepOut.cpp
lldb/source/Target/ThreadPlanStepRange.cpp
lldb/source/Target/ThreadPlanStepUntil.cpp
Index: lldb/source/Target/ThreadPlanStepUntil.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStepUntil.cpp
+++ lldb/source/Target/ThreadPlanStepUntil.cpp
@@ -173,7 +173,7 @@
bool done;
StackID cur_frame_zero_id;
- done = (m_stack_id < cur_frame_zero_id);
+ done = m_stack_id.CompareTo(cur_frame_zero_id) == eFrameCompareYounger;
if (done) {
m_stepped_out = true;
@@ -197,11 +197,13 @@
StackID frame_zero_id =
thread.GetStackFrameAtIndex(0)->GetStackID();
- if (frame_zero_id == m_stack_id)
+ auto frame_compare = frame_zero_id.CompareTo(m_stack_id);
+
+ if (frame_compare == eFrameCompareEqual)
done = true;
- else if (frame_zero_id < m_stack_id)
+ else if (frame_compare == eFrameCompareYounger)
done = false;
- else {
+ else if (frame_compare == eFrameCompareOlder) {
StackFrameSP older_frame_sp = thread.GetStackFrameAtIndex(1);
// But if we can't even unwind one frame we should just get out
@@ -216,7 +218,8 @@
done = (older_context == stack_context);
} else
done = false;
- }
+ } else
+ done = thread.GetRegisterContext()->GetPC(0) == m_return_addr;
if (done)
SetPlanComplete();
Index: lldb/source/Target/ThreadPlanStepRange.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStepRange.cpp
+++ lldb/source/Target/ThreadPlanStepRange.cpp
@@ -213,25 +213,21 @@
// to the current list.
lldb::FrameComparison ThreadPlanStepRange::CompareCurrentFrameToStartFrame() {
- FrameComparison frame_order;
Thread &thread = GetThread();
StackID cur_frame_id = thread.GetStackFrameAtIndex(0)->GetStackID();
- if (cur_frame_id == m_stack_id) {
- frame_order = eFrameCompareEqual;
- } else if (cur_frame_id < m_stack_id) {
- frame_order = eFrameCompareYounger;
- } else {
- StackFrameSP cur_parent_frame = thread.GetStackFrameAtIndex(1);
- StackID cur_parent_id;
- if (cur_parent_frame)
- cur_parent_id = cur_parent_frame->GetStackID();
+ auto frame_order = cur_frame_id.CompareTo(m_stack_id);
+ if (frame_order != eFrameCompareUnknown)
+ return frame_order;
+
+ StackFrameSP cur_parent_frame = thread.GetStackFrameAtIndex(1);
+ if (cur_parent_frame) {
+ StackID cur_parent_id = cur_parent_frame->GetStackID();
if (m_parent_stack_id.IsValid() && cur_parent_id.IsValid() &&
m_parent_stack_id == cur_parent_id)
frame_order = eFrameCompareSameParent;
- else
- frame_order = eFrameCompareOlder;
}
+
return frame_order;
}
Index: lldb/source/Target/ThreadPlanStepOut.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStepOut.cpp
+++ lldb/source/Target/ThreadPlanStepOut.cpp
@@ -293,22 +293,7 @@
BreakpointSiteSP site_sp(
m_process.GetBreakpointSiteList().FindByID(stop_info_sp->GetValue()));
if (site_sp && site_sp->IsBreakpointAtThisSite(m_return_bp_id)) {
- bool done;
-
- StackID frame_zero_id =
- GetThread().GetStackFrameAtIndex(0)->GetStackID();
-
- if (m_step_out_to_id == frame_zero_id)
- done = true;
- else if (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);
- }
-
- if (done) {
+ if (DoesCurrentFrameExplainStop()) {
if (InvokeShouldStopHereCallback(eFrameCompareOlder, m_status)) {
CalculateReturnValue();
SetPlanComplete();
@@ -362,10 +347,8 @@
return m_step_out_further_plan_sp->ShouldStop(event_ptr);
}
- if (!done) {
- StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
- done = !(frame_zero_id < m_step_out_to_id);
- }
+ if (!done)
+ done = DoesCurrentFrameExplainStop();
// The normal step out computations think we are done, so all we need to do
// is consult the ShouldStopHere, and we are done.
@@ -524,5 +507,26 @@
// 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 frame_zero_id.CompareTo(m_step_out_to_id) != eFrameCompareYounger;
+}
+
+bool ThreadPlanStepOut::DoesCurrentFrameExplainStop() {
+ StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
+
+ auto comparison_result = m_step_out_to_id.CompareTo(frame_zero_id);
+
+ if (comparison_result == eFrameCompareYounger ||
+ comparison_result == eFrameCompareEqual)
+ return true;
+
+ if (comparison_result == eFrameCompareOlder)
+ return false;
+
+ // We can reach this point if frame_zero_id and m_step_out_to_id have the same
+ // CFA but belong to different functions.
+
+ if (m_immediate_step_from_id.CompareTo(frame_zero_id) == eFrameCompareYounger)
+ return true;
+
+ return m_return_addr == frame_zero_id.GetPC();
}
Index: lldb/source/Target/ThreadPlanStepInstruction.cpp
===================================================================
--- lldb/source/Target/ThreadPlanStepInstruction.cpp
+++ lldb/source/Target/ThreadPlanStepInstruction.cpp
@@ -99,7 +99,9 @@
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
Thread &thread = GetThread();
StackID cur_frame_id = thread.GetStackFrameAtIndex(0)->GetStackID();
- if (cur_frame_id == m_stack_id) {
+ FrameComparison frame_order = cur_frame_id.CompareTo(m_stack_id);
+
+ if (frame_order == eFrameCompareEqual) {
// Set plan Complete when we reach next instruction
uint64_t pc = thread.GetRegisterContext()->GetPC(0);
uint32_t max_opcode_size =
@@ -109,19 +111,26 @@
if (next_instruction_reached) {
SetPlanComplete();
}
- return (thread.GetRegisterContext()->GetPC(0) != m_instruction_addr);
- } else if (cur_frame_id < m_stack_id) {
+ return (pc != m_instruction_addr);
+ } else if (frame_order == eFrameCompareYounger) {
// 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.
return !m_step_over;
- } else {
+ } else if (frame_order == eFrameCompareOlder) {
if (log) {
LLDB_LOGF(log,
"ThreadPlanStepInstruction::IsPlanStale - Current frame is "
"older than start frame, plan is stale.");
}
return true;
+ } else {
+ if (log)
+ LLDB_LOGF(log,
+ "ThreadPlanStepInstruction::IsPlanStale - Failed to compare "
+ "current frame to the start frame, rely on PC comparison.");
+
+ return (thread.GetRegisterContext()->GetPC(0) != m_instruction_addr);
}
}
@@ -140,7 +149,9 @@
StackID cur_frame_zero_id = cur_frame_sp->GetStackID();
- if (cur_frame_zero_id == m_stack_id || m_stack_id < cur_frame_zero_id) {
+ FrameComparison frame_order = m_stack_id.CompareTo(cur_frame_zero_id);
+
+ if (frame_order == eFrameCompareEqual || frame_order == eFrameCompareYounger) {
if (thread.GetRegisterContext()->GetPC(0) != m_instruction_addr) {
if (--m_iteration_count <= 0) {
SetPlanComplete();
@@ -153,7 +164,7 @@
}
} else
return false;
- } else {
+ } else if (frame_order == eFrameCompareOlder) {
// We've stepped in, step back out again:
StackFrame *return_frame = thread.GetStackFrameAtIndex(1).get();
if (return_frame) {
@@ -216,6 +227,10 @@
SetPlanComplete();
return true;
}
+ } else {
+ LLDB_LOGF(log, "Could not compare frame IDs, stopping.");
+ SetPlanComplete();
+ return true;
}
} else {
lldb::addr_t pc_addr = thread.GetRegisterContext()->GetPC(0);
Index: lldb/source/Target/StackID.cpp
===================================================================
--- lldb/source/Target/StackID.cpp
+++ lldb/source/Target/StackID.cpp
@@ -30,35 +30,10 @@
s->PutCString(") ");
}
-bool lldb_private::operator==(const StackID &lhs, const StackID &rhs) {
- if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
- return false;
-
- SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
- SymbolContextScope *rhs_scope = rhs.GetSymbolContextScope();
-
- // Only compare the PC values if both symbol context scopes are nullptr
- if (lhs_scope == nullptr && rhs_scope == nullptr)
- return lhs.GetPC() == rhs.GetPC();
-
- return lhs_scope == rhs_scope;
-}
-
-bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
- if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
- return true;
-
- SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
- SymbolContextScope *rhs_scope = rhs.GetSymbolContextScope();
-
- if (lhs_scope == nullptr && rhs_scope == nullptr)
- return lhs.GetPC() != rhs.GetPC();
+lldb::FrameComparison StackID::CompareTo(const StackID &rhs) const {
+ if (*this == rhs)
+ return lldb::eFrameCompareEqual;
- return lhs_scope != rhs_scope;
-}
-
-bool lldb_private::operator<(const StackID &lhs, const StackID &rhs) {
- const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddress();
const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddress();
// FIXME: We are assuming that the stacks grow downward in memory. That's not
@@ -70,28 +45,60 @@
// constructor. But I'm not going to waste a bool per StackID on this till we
// need it.
- if (lhs_cfa != rhs_cfa)
- return lhs_cfa < rhs_cfa;
+ if (m_cfa != rhs_cfa)
+ return m_cfa < rhs_cfa ? lldb::eFrameCompareYounger
+ : lldb::eFrameCompareOlder;
- SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
SymbolContextScope *rhs_scope = rhs.GetSymbolContextScope();
- if (lhs_scope != nullptr && rhs_scope != nullptr) {
- // Same exact scope, lhs is not less than (younger than rhs)
- if (lhs_scope == rhs_scope)
- return false;
+ if (m_symbol_scope != nullptr && rhs_scope != nullptr) {
+ // Same CFA and same exact scope, StackIDs are equal.
+ if (m_symbol_scope == rhs_scope)
+ return lldb::eFrameCompareEqual;
SymbolContext lhs_sc;
SymbolContext rhs_sc;
- lhs_scope->CalculateSymbolContext(&lhs_sc);
+ m_symbol_scope->CalculateSymbolContext(&lhs_sc);
rhs_scope->CalculateSymbolContext(&rhs_sc);
// Items with the same function can only be compared
if (lhs_sc.function == rhs_sc.function && lhs_sc.function != nullptr &&
lhs_sc.block != nullptr && rhs_sc.function != nullptr &&
rhs_sc.block != nullptr) {
- return rhs_sc.block->Contains(lhs_sc.block);
+ if (rhs_sc.block->Contains(lhs_sc.block))
+ return lldb::eFrameCompareYounger;
+
+ if (lhs_sc.block->Contains(rhs_sc.block))
+ return lldb::eFrameCompareOlder;
}
}
- return false;
+
+ return lldb::eFrameCompareUnknown;
+}
+
+bool lldb_private::operator==(const StackID &lhs, const StackID &rhs) {
+ if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
+ return false;
+
+ SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
+ SymbolContextScope *rhs_scope = rhs.GetSymbolContextScope();
+
+ // Only compare the PC values if both symbol context scopes are nullptr
+ if (lhs_scope == nullptr && rhs_scope == nullptr)
+ return lhs.GetPC() == rhs.GetPC();
+
+ return lhs_scope == rhs_scope;
+}
+
+bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
+ if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
+ return true;
+
+ SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
+ SymbolContextScope *rhs_scope = rhs.GetSymbolContextScope();
+
+ if (lhs_scope == nullptr && rhs_scope == nullptr)
+ return lhs.GetPC() != rhs.GetPC();
+
+ return lhs_scope != rhs_scope;
}
Index: lldb/source/Target/StackFrameList.cpp
===================================================================
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -730,7 +730,7 @@
static bool CompareStackID(const StackFrameSP &stack_sp,
const StackID &stack_id) {
- return stack_sp->GetStackID() < stack_id;
+ return stack_sp->GetStackID().CompareTo(stack_id) == eFrameCompareYounger;
}
StackFrameSP StackFrameList::GetFrameWithStackID(const StackID &stack_id) {
Index: lldb/include/lldb/Target/ThreadPlanStepOut.h
===================================================================
--- lldb/include/lldb/Target/ThreadPlanStepOut.h
+++ lldb/include/lldb/Target/ThreadPlanStepOut.h
@@ -86,6 +86,8 @@
void CalculateReturnValue();
+ bool DoesCurrentFrameExplainStop();
+
ThreadPlanStepOut(const ThreadPlanStepOut &) = delete;
const ThreadPlanStepOut &operator=(const ThreadPlanStepOut &) = delete;
};
Index: lldb/include/lldb/Target/StackID.h
===================================================================
--- lldb/include/lldb/Target/StackID.h
+++ lldb/include/lldb/Target/StackID.h
@@ -62,6 +62,8 @@
return *this;
}
+lldb::FrameComparison CompareTo(const StackID &rhs) const;
+
protected:
friend class StackFrame;
@@ -93,9 +95,6 @@
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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
