[lldb] Call FixUpPointer in WritePointerToMemory #152798
Conversation
In architectures where pointers may contain metadata, such as arm64e, the metadata may need to be cleaned prior to sending this pointer to be used in expression evaluation generated code. This patch is a step towards allowing consumers of pointers to decide whether they want to keep or remove metadata, as opposed to discarding metadata at the moment pointers are created. See llvm#150537. This was tested running the LLDB test suite on arm64e.
|
@llvm/pr-subscribers-lldb Author: Felipe de Azevedo Piovezan (felipepiovezan) ChangesIn architectures where pointers may contain metadata, such as arm64e, the metadata may need to be cleaned prior to sending this pointer to be used in expression evaluation generated code. This patch is a step towards allowing consumers of pointers to decide whether they want to keep or remove metadata, as opposed to discarding metadata at the moment pointers are created. See This was tested running the LLDB test suite on arm64e. Full diff: https://github.com/llvm/llvm-project/pull/152798.diff 1 Files Affected:
diff --git a/lldb/source/Expression/IRMemoryMap.cpp b/lldb/source/Expression/IRMemoryMap.cpp
index 150699352a2e3..8be389973f123 100644
--- a/lldb/source/Expression/IRMemoryMap.cpp
+++ b/lldb/source/Expression/IRMemoryMap.cpp
@@ -640,6 +640,9 @@ void IRMemoryMap::WritePointerToMemory(lldb::addr_t process_address,
lldb::addr_t address, Status &error) {
error.Clear();
+ if (auto process_sp = GetProcessWP().lock())
+ address = process_sp->FixAnyAddress(address);
+
Scalar scalar(address);
WriteScalarToMemory(process_address, scalar, GetAddressByteSize(), error);
|
| error.Clear(); | ||
|
|
||
| if (auto process_sp = GetProcessWP().lock()) | ||
| address = process_sp->FixAnyAddress(address); |
There was a problem hiding this comment.
Why not put this deeper down, in WriteScalarToMemory or even WriteMemory ?
There was a problem hiding this comment.
Putting it here might fix tests but the other code paths might be lacking coverage.
There was a problem hiding this comment.
Only pointers need to be fixed though. I'm not sure I understand?
There was a problem hiding this comment.
OHHH, I see the confusion now: note that this is not fixing the "pointer where the data is written", but rather it is fixing the "pointer written to memory". (see the name of the method: Write**Pointer**ToMemory)
There was a problem hiding this comment.
At least one thing in this method should be actually be called pointer, but that's not your fault.
I'm not 100% sure that all pointer values here do want to be fixed. Looking at the uses of this method inside of lldb I see:
- materialising references to variables
- setting up the stack frame for a function call, in some ABI plugins
Both of which should be ok with removing the current uses of non-address bits. If you remove pointer authentication bits from a function pointer, it should still authenticate, it's just unsigned. So if there was code that checked for that, that's a corner case that won't work.
It's not in the SB API so there's nothing to break there.
Probably some corner case here but short of a major rework of how we handle addresses, this is a good step. We can consider it a bug fix to the current strategy, whether that strategy is ultimately good or bad.
There was a problem hiding this comment.
I will make a note of that possibility and try to make things break when I get the chance, as this is the kind of corner case I want to explore too.
There was a problem hiding this comment.
Pointer authentication is probably ok unless your ABI specifically checks that there is a non-zero authentication code. I'm assuming a code of 0 will always authenticate.
For memory tagging if your memory allocator uses tags you would not be able to write to write via. a pointer to a heap allocation. The top byte would be removed and treated as tag 0 which is often used to mark unallocated areas (though you don't have to, some will randomise it).
(you can read/write a heap allocation with lldb's commands because debug APIs ignore tag mismatches)
I'm not sure how exactly you'd hit this from LLDB. And I wonder if you could work around it with some crafty casting in and out of uintptr_t.
I will make a note to try out memory tagging.
Istr Objective C using the top byte for something, you could look into that.
There was a problem hiding this comment.
In fact the more I think about it, debug APIs are allowed to ignore memory attributes but I'm not so sure expressions should.
We can consider it a bug fix to the current strategy, whether that strategy is ultimately good or bad.
But this still applies. Can't consider a change before we make the existing thing consistent.
There was a problem hiding this comment.
I think the crucial part of your observations is that we really need to delay cleaning those attributes until absolutely necessary (e.g. #150537), so that we then have the chance to decide whether/which bits to clean up. In that sense, the existing patches are a step in that direction.
|
Heads up, this broke |
Looking at this now. If it's not a simple test change, I'll revert and have a look with more time tomorrow. |
|
This test is exercising a corner case when we can't allocate memory in the inferior, so we allocate it in the lldb process and give it a magic address which is, allegedly, never going to be used by the inferior. And this address has some of those bits that would get cleaned set.... |
|
Reverted yesterday in: a203546 20250812 fpiove.. Revert "[lldb] Call FixUpPointer in WritePointerToMemory" |
In architectures where pointers may contain metadata, such as arm64e, the metadata may need to be cleaned prior to sending this pointer to be used in expression evaluation generated code.
This patch is a step towards allowing consumers of pointers to decide whether they want to keep or remove metadata, as opposed to discarding metadata at the moment pointers are created. See
#150537.
This was tested running the LLDB test suite on arm64e.