[lldb] Track CFA pointer metadata in StackID #157498
Conversation
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesIn this commit: 9c8e716 [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 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:
Full diff: https://github.com/llvm/llvm-project/pull/157498.diff 5 Files Affected:
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
|
|
Btw @jasonmolenda has suggested a way of testing this by using a yaml + core file to come up with an example where the fp register would have its metadata bits set. This would allow us to test |
|
I agree with the ultimate goal of being able to control the handling of non-address bits, but help me understand this part:
Is this because this pointer may be used to work out the address of variables and if we had for example, a memory tagged stack variable, its address must preserve that tag so we can access it without faulting. That sort of thing? |
That's exactly right. I've just updated the PR / commit messages to make that more explicit:
|
felipepiovezan
force-pushed
the
felipe/stack_id_cfas
branch
from
afed92d to
176be60
Compare
In this commit: 9c8e716 [lldb] Make StackID call Fix{Code,Data} pointers (llvm#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 may require the address of a variable that is computed from the CFA; to access the variable without faulting, we may need to preserve the pointer metadata. 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. This patch also changes RegisterContextUnwind::ReadFrameAddress, which is the method computing the CFA fed into StackID, to also preserve the signature pointers.
felipepiovezan
force-pushed
the
felipe/stack_id_cfas
branch
from
176be60 to
7ab0dfa
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
|
@DavidSpickett @jasonmolenda I finally managed to write a test for this, but it took some assembly / DWARF bit scrubbing. Swift uses a lot of variable descriptions done in terms of CallFrameAddress, but with plain old C it is a bit hard. I hacked around this compiling a simple C program (see main.s) and editing it to:
This is described in the test itself. I also had to add a few changes in Apologies for the force push here. |
lldb/include/lldb/Target/StackID.h
Outdated
| /// 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). |
There was a problem hiding this comment.
Not sure how this sneaked in. Will revert.
| self.assertTrue(argv_addr.IsValid()) | ||
|
|
||
| argv_addr_uint = argv_addr.GetValueAsUnsigned() | ||
| self.assertNotEqual((argv_addr_uint & (1 << 60)), 0) |
There was a problem hiding this comment.
Maybe you could add a test that self.assertEqual((thread.frames[1].GetCFA() & (1 << 60)), 0) to confirm that fetching the CFA through a method that strips metadata does so?
There was a problem hiding this comment.
Good point. I'm still not sure whether we should make SBFrame::GetCFA return the stripped or unstripped pointer, but at least the test would document the existing behavior.
| # stp x29, x30, [sp, #-16]! | ||
| thread.StepInstruction(False) | ||
| thread.StepInstruction(False) | ||
|
|
There was a problem hiding this comment.
I think it could be clearer that you're (1) adding metadata to the $fp value in foo() before it is stored to stack. Then we (2) navigate to the frame above, main(), and perform tests on its CFA value, which is expressed in terms of main's $fp, which lldb will need to fetch from the foo() stack space, and therefore have the metadata.
# Retrieve the caller frame, main(), now that its $fp has metadata
# added before foo() saved the value to stack.
main_frame = thread.frames[1]
...
There was a problem hiding this comment.
I sort of wrote that in the .S file comments at the top, but it makes sense to have a version of it in the python driver.
|
This looks good to me, I had a couple of small suggestions on the API test but nothing substantive. |
|
I'm going to go ahead and merge this based on Jason's approval and the general agreement in comments, but happy to address any other feedback. |
In this commit: 9c8e716 [lldb] Make StackID call Fix{Code,Data} pointers (llvm#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 may require the address of a variable that is computed from the CFA; to access the variable without faulting, we may need to preserve the pointer metadata. 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. This patch also changes RegisterContextUnwind::ReadFrameAddress, which is the method computing the CFA fed into StackID, to also preserve the signature pointers. (cherry picked from commit 5d088ba)
In this commit: 9c8e716 [lldb] Make StackID call Fix{Code,Data} pointers (llvm#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 may require the address of a variable that is computed from the CFA; to access the variable without faulting, we may need to preserve the pointer metadata. 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. This patch also changes RegisterContextUnwind::ReadFrameAddress, which is the method computing the CFA fed into StackID, to also preserve the signature pointers. (cherry picked from commit 5d088ba) (cherry picked from commit c507ec1)
Uh oh!
There was an error while loading. Please reload this page.