-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[RemoteInspection] Change RemoteAbsolutePointer (NFC) #82325
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
Conversation
test with swiftlang/llvm-project#10865 |
@@ -1871,7 +1871,7 @@ class TypeRefBuilder { | |||
if (auto symbol = OpaquePointerReader( | |||
remote::RemoteAddress(adjustedProtocolDescriptorTarget), | |||
PointerSize)) { | |||
if (!symbol->getSymbol().empty()) { | |||
if (!symbol->getSymbol().empty() && symbol->getOffset() == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why must the offset be 0 in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the other comments the combination of Symbol+Offset isn't actually implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took this for granted without further investigation, but given that the Offset is never taken here, it seems plausible.
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
test with swiftlang/llvm-project#10865 |
test with swiftlang/llvm-project#10865 |
test with swiftlang/llvm-project#10865 |
This pull broke a couple tests on Android x86_64 and AArch64, which I confirmed by applying this pull alone locally on Android. Looking at both tests, they have long been disabled on linux AArch64, so maybe that's why this wasn't flagged, whereas we've been running them on Android for years without a problem until this pull. Looking at the output for
but I see this instead:
whereas the output from @adrian-prantl, let me know what you think. |
Thanks for letting me know, I'll be taking a look. |
Hmm, this one seems to actually work on linux:
|
|
@finagolfin Is there a tutorial for how to set up the Android cross-compilation toolchain on Linux? |
Hmm, maybe Android-specific then, as they pass on linux x86_64 too but fail on Android x86_64. The easiest way to check this on linux would be to download the Swift source on there as usual, get the latest LTS Android NDK 27c, make sure a prebuilt Swift host compiler is in your
Unfortunately, you can't just use a prebuilt official trunk snapshot toolchain because it doesn't ship with the |
…wiftlang#82620) The tests broke on the community Android CI since swiftlang#82325, and I just noticed the install issue when cross-compiling Testing with a freshly-built compiler, which I'd never done before. Also, fix the NDK path shown in the CMake output.
FWIW, I am seeing two errors in our internal Linux builds which match the ones finagolfin reported in Android. Our Linux is not a public distro, which sometimes introduces problems, but in this case, since they were happening also in some setup, these changes might be something that might only work on the Ubuntus used for testing or something environmental:
|
Thanks for reporting, @drodriguez, are you changing the default linker to lld or doing something else unusual in your linux x86_64 testing? I find it very weird that these two tests pass on all the public linux distro CI but only fail for us: perhaps lld or something like that is the reason. |
It might be the usage of LLD. We use LLD for ELF. |
I was able to reproduce this with LLD, and have a fix #82981 |
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 MetadataReader.
Addresses parts of rdar://146273066.
rdar://153687085