https://github.com/felipepiovezan created
https://github.com/llvm/llvm-project/pull/157498
In this commit:
9c8e71644227 [lldb] Make StackID call Fix{Code,Data} pointers (#152796)
We made StackID keep track of the CFA without any pointer metadata in it. This
is necessary when comparing two StackIDs to determine which one is "younger".
However, the CFA inside StackIDs is also used in other contexts through the
method StackID::GetCallFrameAddress. One notable case is DWARFExpression: the
computation of `DW_OP_call_frame_address` is done using StackID. This feeds
into many other places, e.g. expression evaluation (which may require a signed
pointer). As such, StackID must be able to provide both versions of the CFA.
In the spirit of allowing consumers of pointers to decide what to do with
pointer metadata, this patch changes StackID to store both versions of the cfa
pointer. Two getter methods are provided, and all call sites except
DWARFExpression preserve their existing behavior (stripped pointer). Other
alternatives were considered:
* Just store the raw pointer. This would require changing the comparisong
operator `<` to also receive a Process, as the comparison requires stripped
pointers. It wasn't clear if all call-sites had a non-null process, whereas we
know we have a process when creating a StackID.
* Store a weak pointer to the process inside the class, and then strip metadata
as needed. This would require a `weak_ptr::lock` in many operations of LLDB,
and it felt wasteful. It also prevents stripping of the pointer if the process
has gone away.
>From afed92d55eefce1ad681c4e2d9df3b754c01dc2a Mon Sep 17 00:00:00 2001
From: Felipe de Azevedo Piovezan <fpiove...@apple.com>
Date: Mon, 8 Sep 2025 08:30:19 -0700
Subject: [PATCH] [lldb] Track CFA pointer metadata in StackID
In this commit:
9c8e71644227 [lldb] Make StackID call Fix{Code,Data} pointers (#152796)
We made StackID keep track of the CFA without any pointer metadata in
it. This is necessary when comparing two StackIDs to determine which one
is "younger".
However, the CFA inside StackIDs is also used in other contexts through
the method StackID::GetCallFrameAddress. One notable case is
DWARFExpression: the computation of `DW_OP_call_frame_address` is done
using StackID. This feeds into many other places, e.g. expression
evaluation (which may require a signed pointer). As such, StackID must
be able to provide both versions of the CFA.
In the spirit of allowing consumers of pointers to decide what to do
with pointer metadata, this patch changes StackID to store both versions
of the cfa pointer. Two getter methods are provided, and all call sites
except DWARFExpression preserve their existing behavior (stripped
pointer). Other alternatives were considered:
* Just store the raw pointer. This would require changing the
comparisong operator `<` to also receive a Process, as the comparison
requires stripped pointers. It wasn't clear if all call-sites had a
non-null process, whereas we know we have a process when creating a
StackID.
* Store a weak pointer to the process inside the class, and then strip
metadata as needed. This would require a `weak_ptr::lock` in many
operations of LLDB, and it felt wasteful. It also prevents stripping
of the pointer if the process has gone away.
---
lldb/include/lldb/Target/StackID.h | 11 +++++++++--
lldb/source/API/SBFrame.cpp | 2 +-
lldb/source/Expression/DWARFExpression.cpp | 2 +-
lldb/source/Target/StackFrameList.cpp | 2 +-
lldb/source/Target/StackID.cpp | 11 +++++++----
5 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/lldb/include/lldb/Target/StackID.h
b/lldb/include/lldb/Target/StackID.h
index c2a5d733dcd69..754f80d001647 100644
--- a/lldb/include/lldb/Target/StackID.h
+++ b/lldb/include/lldb/Target/StackID.h
@@ -26,7 +26,11 @@ class StackID {
lldb::addr_t GetPC() const { return m_pc; }
- lldb::addr_t GetCallFrameAddress() const { return m_cfa; }
+ lldb::addr_t GetCallFrameAddressWithMetadata() const {
+ return m_cfa_with_metadata;
+ }
+
+ lldb::addr_t GetCallFrameAddressWithoutMetadata() const { return m_cfa; }
SymbolContextScope *GetSymbolContextScope() const { return m_symbol_scope; }
@@ -59,9 +63,12 @@ class StackID {
/// The call frame address (stack pointer) value at the beginning of the
/// function that uniquely identifies this frame (along with m_symbol_scope
- /// below)
+ /// below).
lldb::addr_t m_cfa = LLDB_INVALID_ADDRESS;
+ /// The cfa with metadata (i.e. prior to Process::FixAddress).
+ lldb::addr_t m_cfa_with_metadata = LLDB_INVALID_ADDRESS;
+
/// If nullptr, there is no block or symbol for this frame. If not nullptr,
/// this will either be the scope for the lexical block for the frame, or the
/// scope for the symbol. Symbol context scopes are always be unique pointers
diff --git a/lldb/source/API/SBFrame.cpp b/lldb/source/API/SBFrame.cpp
index b6724bb0c4119..42dbed490a33d 100644
--- a/lldb/source/API/SBFrame.cpp
+++ b/lldb/source/API/SBFrame.cpp
@@ -267,7 +267,7 @@ lldb::addr_t SBFrame::GetCFA() const {
}
if (StackFrame *frame = exe_ctx->GetFramePtr())
- return frame->GetStackID().GetCallFrameAddress();
+ return frame->GetStackID().GetCallFrameAddressWithoutMetadata();
return LLDB_INVALID_ADDRESS;
}
diff --git a/lldb/source/Expression/DWARFExpression.cpp
b/lldb/source/Expression/DWARFExpression.cpp
index 332cf2c86024a..5040351f4975b 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -2195,7 +2195,7 @@ llvm::Expected<Value> DWARFExpression::Evaluate(
// Note that we don't have to parse FDEs because this DWARF expression
// is commonly evaluated with a valid stack frame.
StackID id = frame->GetStackID();
- addr_t cfa = id.GetCallFrameAddress();
+ addr_t cfa = id.GetCallFrameAddressWithMetadata();
if (cfa != LLDB_INVALID_ADDRESS) {
stack.push_back(Scalar(cfa));
stack.back().SetValueType(Value::ValueType::LoadAddress);
diff --git a/lldb/source/Target/StackFrameList.cpp
b/lldb/source/Target/StackFrameList.cpp
index d83c90582ba9c..3ed73a3b0afdb 100644
--- a/lldb/source/Target/StackFrameList.cpp
+++ b/lldb/source/Target/StackFrameList.cpp
@@ -449,7 +449,7 @@ bool StackFrameList::FetchFramesUpTo(uint32_t end_idx,
}
} else {
unwind_frame_sp = m_frames.front();
- cfa = unwind_frame_sp->m_id.GetCallFrameAddress();
+ cfa = unwind_frame_sp->m_id.GetCallFrameAddressWithoutMetadata();
}
} else {
// Check for interruption when building the frames.
diff --git a/lldb/source/Target/StackID.cpp b/lldb/source/Target/StackID.cpp
index f879276527dda..137c776a84d2f 100644
--- a/lldb/source/Target/StackID.cpp
+++ b/lldb/source/Target/StackID.cpp
@@ -17,7 +17,8 @@ using namespace lldb_private;
StackID::StackID(lldb::addr_t pc, lldb::addr_t cfa,
SymbolContextScope *symbol_scope, Process *process)
- : m_pc(pc), m_cfa(cfa), m_symbol_scope(symbol_scope) {
+ : m_pc(pc), m_cfa(cfa), m_cfa_with_metadata(cfa),
+ m_symbol_scope(symbol_scope) {
if (process) {
m_pc = process->FixCodeAddress(m_pc);
m_cfa = process->FixDataAddress(m_cfa);
@@ -29,6 +30,7 @@ void StackID::SetPC(lldb::addr_t pc, Process *process) {
}
void StackID::SetCFA(lldb::addr_t cfa, Process *process) {
+ m_cfa_with_metadata = cfa;
m_cfa = process ? process->FixDataAddress(cfa) : cfa;
}
@@ -49,7 +51,8 @@ void StackID::Dump(Stream *s) {
}
bool lldb_private::operator==(const StackID &lhs, const StackID &rhs) {
- if (lhs.GetCallFrameAddress() != rhs.GetCallFrameAddress())
+ if (lhs.GetCallFrameAddressWithoutMetadata() !=
+ rhs.GetCallFrameAddressWithoutMetadata())
return false;
SymbolContextScope *lhs_scope = lhs.GetSymbolContextScope();
@@ -67,8 +70,8 @@ bool lldb_private::operator!=(const StackID &lhs, const
StackID &rhs) {
}
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();
+ const lldb::addr_t lhs_cfa = lhs.GetCallFrameAddressWithoutMetadata();
+ const lldb::addr_t rhs_cfa = rhs.GetCallFrameAddressWithoutMetadata();
// FIXME: We are assuming that the stacks grow downward in memory. That's
not
// necessary, but true on
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
