[5.6][android] Move the string and other tags in pointers to the second byte because Android enabled memory tagging #42178
Conversation
|
@milseman, now that this is in trunk, can we get this in the next 5.6 point release? |
There was a problem hiding this comment.
Code in the stdlib LGTM. You'll want a green checkmark from @jckarter as well.
Note that my review is not a recommendation about what should be in what releases, etc. It's a review of the stdlib code that equally applies to trunk.
|
@jckarter, if you would approve the compiler side of this pull, I'd like to get it in for the next 5.6 point release. |
|
Once 5.6 was released, the 5.6 branch should only be for major regressions or security fixes, so I don't think we can take this change onto that branch. I understand this unlocks important use cases for an architecture, but given support for this platform is still in bringup mode, it's hard to justify merging this (even if there was a release vehicle for another 5.6 compiler, which it's not clear there will be before 5.7 now that that has branched). I suggest bringing this onto the 5.7 branch. |
This isn't a regression or security fix, but it fixes a major incompatibility introduced on Android AArch64 since Android 11 for devices that have memory tagging enabled. In my house, that's two out of three devices running Android 11+ where Swift had stopped working because tagging was enabled.
Historically, the March release ships several point releases till the September release, ie 5.4.3 shipped last Sept. 9. Are you saying that's changing and 5.7 will ship much earlier? The only reasons I submitted this PR is because most of it is exclusive to Android AArch64 so it doesn't affect any other platform, and I distribute an Android SDK that can be used with the official shipping 5.6 compiler for linux, but this fix requires changes to the compiler. If you'd like, I can remove the two KeyPath changes that are applied to all platforms and then this patch only affects Android AArch64, with all changes scoped with That would represent zero risk to the 5.6 branch on existing supported platforms, but fix a major incompatibility on Android AArch64. Please let me know what you think. |
…te because Android enabled memory tagging Starting with Android 11, AArch64 placed a tag in the top byte of pointers to allocations, which has been slowly rolling out to more devices and collides with Swift's tags. Moving these tags to the second byte works around this problem.
| var misalign = Int(bitPattern: baseAddress) % alignment | ||
| #endif |
There was a problem hiding this comment.
@airspeedswift, rebased and made this KeyPath change I mentioned in my last comment, so this pull now only affects Android AArch64 and will have no effect on any other platform.
|
Ping @airspeedswift, can we get this in? It only affects Android AArch64 now, so represents zero risk to other platforms. |
|
@shahmishal, I'd like to get this into 5.6.3, please let me know what you think. |
|
@swift-ci test |
|
Mac CI failure with Swift Docc is unrelated. |
|
@shahmishal, can we get this merged before the release? |
|
Thanks for getting this in, @shahmishal and @milseman, now I need to update my Swift 5.6 Android SDK to use it. |
Cherrypick of #40779
Explanation: Starting with Android 11, AArch64 placed a tag in the top byte of pointers to allocations, which has been slowly rolling out to more devices and collides with Swift's tags. Moving these tags to the second byte works around this problem.
Scope:
With the exception of the two KeyPath changes, which @jckarter suggested and said he wanted for all platforms,this pull does not modify any platform other than Android AArch64.SR Issue: None
Risk: low
Testing: I've been applying most of this pull to the native Swift toolchain which I maintain that runs on Android devices for the last couple months, termux/termux-packages@5d98fde, with the toolchain now finally working on Android 11+ devices with tagging enabled and without any user complaint.
Reviewer: @milseman
Once this is in the official 5.6 compiler build, my Swift 5.6 Android AArch64 SDK should start working on tagged devices too.