Skip to content

[lldb] Call FixUpPointer in WritePointerToMemory #152798

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.

Sign up for GitHub

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 1 commit into from
Aug 11, 2025
Merged

[lldb] Call FixUpPointer in WritePointerToMemory #152798

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions lldb/source/Expression/IRMemoryMap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not put this deeper down, in WriteScalarToMemory or even WriteMemory ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Putting it here might fix tests but the other code paths might be lacking coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only pointers need to be fixed though. I'm not sure I understand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


Scalar scalar(address);

WriteScalarToMemory(process_address, scalar, GetAddressByteSize(), error);
Expand Down
Loading