Skip to content

[NFC][RemoteInspection] Add an opaque AddressSpace field to RemoteAddress #82997

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

Conversation

augusto2112
Copy link
Contributor

@augusto2112 augusto2112 commented Jul 11, 2025

This patch changes RemoteAbsolutePointer to store both the symbol and
the resolved address. This allows us to retire some ugly workarounds
to deal with non-symbolic addresses and it fixes code paths that would
need these workarounds, but haven't implemented them yet (i.e., the
pack shape handling in the symbolicReferenceResolver in MetadatyaReader.

Addresses parts of rdar://146273066.
rdar://153687085

(cherry picked from commit 9381a54)
(cherry picked from commit a6eafcb)
@augusto2112 augusto2112 force-pushed the change-remote-addr-6.2-3 branch from 3ab899c to 93d855c Compare July 11, 2025 17:09
@augusto2112
Copy link
Contributor Author

1 similar comment
@augusto2112
Copy link
Contributor Author

@augusto2112 augusto2112 force-pushed the change-remote-addr-6.2-3 branch from 61acb0b to 93d855c Compare July 11, 2025 17:52
@augusto2112
Copy link
Contributor Author

The Offset field of a DynamicRelocation is either an offset or a remote
address, but was being treated only as a remote address on
getDynamicSymbol.
…ress

Add an extra opaque field to AddressSpace, which can be used by clients
of RemoteInspection to distinguish between different address spaces.

LLDB employs an optimization where it reads memory from files instead of
the running process whenever it can to speed up memory reads (these can
be slow when debugging something over a network). To do this, it needs
to keep track whether an address originated from a process or a file. It
currently distinguishes addresses by setting an unused high bit on the
address, but because of pointer authentication this is not a reliable
solution. In order to keep this optimization working, this patch adds an
extra opaque AddressSpace field to RemoteAddress, which LLDB can use on
its own implementation of MemoryReader to distinguish between addresses.

This patch is NFC for the other RemoteInspection clients, as it adds
extra information to RemoteAddress, which is entirely optional and if
unused should not change the behavior of the library.

Although this patch is quite big the changes are largely mechanical,
replacing threading StoredPointer with RemoteAddress.

rdar://148361743
(cherry picked from commit 58df553)
(cherry picked from commit 8f3862b)
Sometimes it makes sense to compares addresses from different address
spaces.

rdar://148361743
(cherry picked from commit c97dfd6)
@augusto2112 augusto2112 force-pushed the change-remote-addr-6.2-3 branch from 93d855c to 79824f5 Compare July 12, 2025 03:25
@augusto2112
Copy link
Contributor Author

@augusto2112 augusto2112 marked this pull request as ready for review July 14, 2025 16:24
@augusto2112 augusto2112 requested a review from a team as a code owner July 14, 2025 16:24
return RemoteAddress(Data - rhs, getAddressSpace());
}

RemoteAddress operator-(const RemoteAddress &rhs) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

The difference between two addresses should be an integer, not an address.

Copy link
Contributor Author

@augusto2112 augusto2112 Jul 14, 2025

Choose a reason for hiding this comment

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

It's useful to have these math operations return an address as an API because they're pretty much always used to calculate a new address at some offset from the current address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, you are right Tim, this shouldn't be the return of comparing two addresses. I will update this

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 have a PR on main fixing this #83040

@JDevlieghere JDevlieghere merged commit 3d0ceb7 into swiftlang:release/6.2 Jul 14, 2025
5 checks passed
@augusto2112 augusto2112 deleted the change-remote-addr-6.2-3 branch July 14, 2025 20:24
@augusto2112 augusto2112 restored the change-remote-addr-6.2-3 branch July 14, 2025 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants