Skip to content

[lldb] Defer checking CFA memory region #7877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions lldb/include/lldb/Target/StackID.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class StackID {
void Clear() {
m_pc = LLDB_INVALID_ADDRESS;
m_cfa = LLDB_INVALID_ADDRESS;
m_cfa_on_stack = true;
m_cfa_on_stack = eLazyBoolCalculate;
m_symbol_scope = nullptr;
}

Expand All @@ -57,15 +57,18 @@ class StackID {
return *this;
}

bool IsCFAOnStack() const { return m_cfa_on_stack; }
/// Check if the CFA is on the stack, or elsewhere in the process, such as on
/// the heap.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we comment that the caller is now expected to check for Heap frames?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this meant to be on the IsYounger function? If so, I moved the heap check into IsYounger to avoid having to put that requirement onto callers.

bool IsCFAOnStack(Process &process) const;

/// Determine if the first StackID is "younger" than the second.
static bool IsYounger(const StackID &lhs, const StackID &rhs,
Process &process);

protected:
friend class StackFrame;

explicit StackID(lldb::addr_t pc, lldb::addr_t cfa, lldb::ThreadSP thread_sp)
: m_pc(pc), m_cfa(cfa), m_cfa_on_stack(IsStackAddress(cfa, thread_sp)) {}

bool IsStackAddress(lldb::addr_t addr, lldb::ThreadSP thread_sp) const;
explicit StackID(lldb::addr_t pc, lldb::addr_t cfa) : m_pc(pc), m_cfa(cfa) {}

void SetPC(lldb::addr_t pc) { m_pc = pc; }

Expand All @@ -84,7 +87,7 @@ class StackID {
// below)
// True if the CFA is an address on the stack, false if it's an address
// elsewhere (ie heap).
bool m_cfa_on_stack = true;
mutable LazyBool m_cfa_on_stack = eLazyBoolCalculate;
SymbolContextScope *m_symbol_scope =
nullptr; // If nullptr, there is no block or symbol for this frame.
// If not nullptr, this will either be the scope for the
Expand All @@ -98,9 +101,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
22 changes: 22 additions & 0 deletions lldb/include/lldb/Target/ThreadPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,28 @@ class ThreadPlan : public std::enable_shared_from_this<ThreadPlan>,

bool IsUsuallyUnexplainedStopReason(lldb::StopReason);

/// Determine if the first StackID is younger than the second.
///
/// A callee is considered younger than its caller, and is also younger than
/// all ancestor frames leading up to the caller. Consider this stack:
///
/// +------------------+
/// | Fa |
/// +------------------+
/// | Fb |
/// +------------------+
/// | ... |
/// +------------------+
/// | Fy |
/// +------------------+
/// | Fz |
/// +------------------+
///
/// In this case Fz is younger than each of Fy, ..., Fb, and Fa. Fy is not
/// younger than Fz, but is younger than all frames above it in the stack,
/// including Fa and Fb.
bool IsYounger(const StackID &lhs, const StackID &rhs) const;

Status m_status;
Process &m_process;
lldb::tid_t m_tid;
Expand Down
14 changes: 6 additions & 8 deletions lldb/source/Target/StackFrame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ StackFrame::StackFrame(const ThreadSP &thread_sp, user_id_t frame_idx,
const SymbolContext *sc_ptr)
: m_thread_wp(thread_sp), m_frame_index(frame_idx),
m_concrete_frame_index(unwind_frame_index), m_reg_context_sp(),
m_id(pc, cfa, thread_sp), m_frame_code_addr(pc), m_sc(), m_flags(),
m_frame_base(), m_frame_base_error(), m_cfa_is_valid(cfa_is_valid),
m_id(pc, cfa), m_frame_code_addr(pc), m_sc(), m_flags(), m_frame_base(),
m_frame_base_error(), m_cfa_is_valid(cfa_is_valid),
m_stack_frame_kind(kind),
m_behaves_like_zeroth_frame(behaves_like_zeroth_frame),
m_variable_list_sp(), m_variable_list_value_objects(),
Expand All @@ -83,10 +83,9 @@ StackFrame::StackFrame(const ThreadSP &thread_sp, user_id_t frame_idx,
const SymbolContext *sc_ptr)
: m_thread_wp(thread_sp), m_frame_index(frame_idx),
m_concrete_frame_index(unwind_frame_index),
m_reg_context_sp(reg_context_sp), m_id(pc, cfa, thread_sp),
m_frame_code_addr(pc), m_sc(), m_flags(), m_frame_base(),
m_frame_base_error(), m_cfa_is_valid(true),
m_stack_frame_kind(StackFrame::Kind::Regular),
m_reg_context_sp(reg_context_sp), m_id(pc, cfa), m_frame_code_addr(pc),
m_sc(), m_flags(), m_frame_base(), m_frame_base_error(),
m_cfa_is_valid(true), m_stack_frame_kind(StackFrame::Kind::Regular),
m_behaves_like_zeroth_frame(behaves_like_zeroth_frame),
m_variable_list_sp(), m_variable_list_value_objects(),
m_recognized_frame_sp(), m_disassembly(), m_mutex() {
Expand All @@ -110,8 +109,7 @@ StackFrame::StackFrame(const ThreadSP &thread_sp, user_id_t frame_idx,
: m_thread_wp(thread_sp), m_frame_index(frame_idx),
m_concrete_frame_index(unwind_frame_index),
m_reg_context_sp(reg_context_sp),
m_id(pc_addr.GetLoadAddress(thread_sp->CalculateTarget().get()), cfa,
thread_sp),
m_id(pc_addr.GetLoadAddress(thread_sp->CalculateTarget().get()), cfa),
m_frame_code_addr(pc_addr), m_sc(), m_flags(), m_frame_base(),
m_frame_base_error(), m_cfa_is_valid(true),
m_stack_frame_kind(StackFrame::Kind::Regular),
Expand Down
40 changes: 20 additions & 20 deletions lldb/source/Target/StackID.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,17 @@

using namespace lldb_private;

bool StackID::IsStackAddress(lldb::addr_t addr,
lldb::ThreadSP thread_sp) const {
if (addr == LLDB_INVALID_ADDRESS)
return false;

if (thread_sp)
if (auto process_sp = thread_sp->GetProcess()) {
bool StackID::IsCFAOnStack(Process &process) const {
if (m_cfa_on_stack == eLazyBoolCalculate) {
m_cfa_on_stack = eLazyBoolNo;
if (m_cfa != LLDB_INVALID_ADDRESS) {
MemoryRegionInfo mem_info;
if (process_sp->GetMemoryRegionInfo(addr, mem_info).Success())
return mem_info.IsStackMemory() == MemoryRegionInfo::eYes;
if (process.GetMemoryRegionInfo(m_cfa, mem_info).Success())
if (mem_info.IsStackMemory() == MemoryRegionInfo::eYes)
m_cfa_on_stack = eLazyBoolYes;
}
return true; // assumed
}
return m_cfa_on_stack == eLazyBoolYes;
}

void StackID::Dump(Stream *s) {
Expand Down Expand Up @@ -74,19 +73,20 @@ bool lldb_private::operator!=(const StackID &lhs, const StackID &rhs) {
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();

bool StackID::IsYounger(const StackID &lhs, const StackID &rhs,
Process &process) {
// FIXME: rdar://76119439
// Heuristic: At the boundary between an async parent frame calling a regular
// child frame, the CFA of the parent async function is a heap addresses, and
// the CFA of concrete child function is a stack addresses. Therefore, if lhs
// is on stack, and rhs is not, lhs is considered less than rhs (regardless of
// address values).
if (lhs.IsCFAOnStack() && !rhs.IsCFAOnStack())
// At the boundary between an async parent frame calling a regular child
// frame, the CFA of the parent async function is a heap addresses, and the
// CFA of concrete child function is a stack address. Therefore, if lhs is
// on stack, and rhs is not, lhs is considered less than rhs, independent of
// address values.
if (lhs.IsCFAOnStack(process) && !rhs.IsCFAOnStack(process))
return true;

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
// necessary, but true on
// all the machines we care about at present. If this changes, we'll have to
Expand Down
4 changes: 4 additions & 0 deletions lldb/source/Target/ThreadPlan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ bool ThreadPlan::IsUsuallyUnexplainedStopReason(lldb::StopReason reason) {
}
}

bool ThreadPlan::IsYounger(const StackID &lhs, const StackID &rhs) const {
return StackID::IsYounger(lhs, rhs, m_process);
}

// ThreadPlanNull

ThreadPlanNull::ThreadPlanNull(Thread &thread)
Expand Down
5 changes: 3 additions & 2 deletions lldb/source/Target/ThreadPlanStepInstruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 (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.
Expand Down Expand Up @@ -140,7 +140,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 ||
IsYounger(m_stack_id, cur_frame_zero_id)) {
if (thread.GetRegisterContext()->GetPC(0) != m_instruction_addr) {
if (--m_iteration_count <= 0) {
SetPlanComplete();
Expand Down
8 changes: 4 additions & 4 deletions lldb/source/Target/ThreadPlanStepOut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -334,12 +334,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 (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 = IsYounger(m_immediate_step_from_id, frame_zero_id);
}

if (done) {
Expand Down Expand Up @@ -398,7 +398,7 @@ bool ThreadPlanStepOut::ShouldStop(Event *event_ptr) {

if (!done) {
StackID frame_zero_id = GetThread().GetStackFrameAtIndex(0)->GetStackID();
done = !(frame_zero_id < m_step_out_to_id);
done = !IsYounger(frame_zero_id, m_step_out_to_id);
}

// The normal step out computations think we are done, so all we need to do
Expand Down Expand Up @@ -608,5 +608,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 !IsYounger(frame_zero_id, m_step_out_to_id);
}
2 changes: 1 addition & 1 deletion lldb/source/Target/ThreadPlanStepRange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,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 (IsYounger(cur_frame_id, m_stack_id)) {
frame_order = eFrameCompareYounger;
} else {
StackFrameSP cur_parent_frame = thread.GetStackFrameAtIndex(1);
Expand Down
4 changes: 2 additions & 2 deletions lldb/source/Target/ThreadPlanStepUntil.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ void ThreadPlanStepUntil::AnalyzeStop() {
bool done;
StackID cur_frame_zero_id;

done = (m_stack_id < cur_frame_zero_id);
done = IsYounger(m_stack_id, cur_frame_zero_id);

if (done) {
m_stepped_out = true;
Expand All @@ -200,7 +200,7 @@ void ThreadPlanStepUntil::AnalyzeStop() {

if (frame_zero_id == m_stack_id)
done = true;
else if (frame_zero_id < m_stack_id)
else if (IsYounger(frame_zero_id, m_stack_id))
done = false;
else {
StackFrameSP older_frame_sp = thread.GetStackFrameAtIndex(1);
Expand Down