Skip to content
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

[6.0.1] SILOptimizer: Allow inlining of transparent functions in @backDeployed thunks #76218

Conversation

tshortli
Copy link
Contributor

@tshortli tshortli commented Sep 3, 2024

  • Explanation: Adjusts an optimizer heuristic to allow inlining of @_transparent functions into @backDeployed thunks so that the codegen for the availability checks in @backDeployed works as expected when compiling for iOS.
  • Scope: Without this fix, unoptimized iOS apps that use newly @backDeployed functions from the _Concurrency library crash when the app runs on macOS, due to the availability condition being misevaluated as checking a macOS version instead of an iOS version.
  • Issue/Radar: rdar://134793410
  • Original PR: SILOptimizer: Allow inlining of transparent functions in @backDeployed thunks #76135
  • Risk: Medium. The optimization adjustment is extremely targeted so that it should only affect optimization and codegen for @backDeployed thunks, but any change of this nature carries some risk of unforeseen consequences.
  • Testing: New tests added to the compiler test suite.
  • Reviewer: @eeckstein @slavapestov @mikeash

@tshortli tshortli requested a review from a team as a code owner September 3, 2024 06:02
@tshortli tshortli added 🍒 release cherry pick Flag: Release branch cherry picks swift 6.0 labels Sep 3, 2024
In the standard library shipped in Apple's SDKs and OSes, the implementation of
`_stdlib_isOSVersionAtLeast()` has diverged in order to solve some tricky
issues related to supporting iOS applications running on macOS. It's now time
to bring that change upstream in order to unblock further changes that depend
on it.

Originally introduced to resolve rdar://83378814.
…d thunks.

In order for availability checks in iOS apps to be evaluated correctly when
running on macOS, the application binary must call a copy of
`_stdlib_isOSVersionAtLeast_AEIC()` that was emitted into the app, instead of
calling the `_stdlib_isOSVersionAtLeast()` function provided by the standard
library. This is because the call to the underlying compiler-rt function
`__isPlatformVersionAtLeast()` must be given the correct platform identifier
argument; if the call is not emitted into the client, then the macOS platform
identifier is used and the iOS version number will be mistakenly interpreted as
a macOS version number at runtime.

The `_stdlib_isOSVersionAtLeast()` function in the standard library is marked
`@_transparent` on iOS so that its call to `_stdlib_isOSVersionAtLeast_AEIC()`
is always inlined into the client. This works for the code generated by normal
`if #available` checks, but for the `@backDeployed` function thunks, the calls
to `_stdlib_isOSVersionAtLeast()` were not being inlined and that was causing
calls to `@backDeployed` functions to crash in iOS apps running on macOS since
their availability checks were being misevaluated.

The SIL optimizer has a heuristic which inhibits mandatory inlining in
functions that are classified as thunks, in order to save code size. This
heuristic needs to be relaxed in `@backDeployed` thunks, so that mandatory
inlining of `_stdlib_isOSVersionAtLeast()` can behave as expected. The change
should be safe since the only `@_transparent` function a `@backDeployed` thunk
is ever expected to call is `_stdlib_isOSVersionAtLeast()`.

Resolves rdar://134793410.
During the toolchain build, when building the Swift standard library for
platforms other than macOS the `libclang_rt.a` needs to be copied out of the
host SDK. That wasn't happening for visionOS.

Resolves rdar://135023111.
@tshortli tshortli force-pushed the backdeployed-ios-apps-on-macos-6.0.1 branch from 28aabab to 1ce8b02 Compare September 4, 2024 14:43
@tshortli
Copy link
Contributor Author

tshortli commented Sep 4, 2024

@swift-ci please test

@tshortli tshortli enabled auto-merge September 4, 2024 14:44
@tshortli
Copy link
Contributor Author

tshortli commented Sep 4, 2024

@swift-ci please build toolchain macOS platform

@tshortli tshortli disabled auto-merge September 4, 2024 14:45
@tshortli tshortli merged commit 4b771a6 into swiftlang:release/6.0.1 Sep 4, 2024
6 checks passed
@tshortli tshortli deleted the backdeployed-ios-apps-on-macos-6.0.1 branch September 4, 2024 23:18
@yusuftor
Copy link

Just to check, we've experienced this crash on some iOS 18 iPhone devices too. In the description for this issue and Xcode RC release notes you talk about it affecting those running on macOS. Can you confirm that this will also fix the issue when running on iOS 18?

@mikeash
Copy link
Contributor

mikeash commented Sep 24, 2024

This issue is specific to macOS, due to availability queries getting confused by the iOS/macOS mix. A crash on iOS is going to be something else. Can you provide more info about the crash you're seeing? A crash log would be good, or a reproducer if you have one. Feel free to open an issue and @ me there, or if you prefer, file feedback at https://feedbackassistant.apple.com and send me the number.

@tshortli
Copy link
Contributor Author

@yusuftor Check whether all the crash reports you are receiving are coming from devices running older betas of iOS 18. The ABI of various concurrency APIs, including this one, changed over the course of the beta period and as a result apps that are compiled against the released SDK for iOS 18 are incompatible with some of the betas. Customers experiencing the crash should be able to avoid it by updating to the released version of iOS 18.

@yusuftor
Copy link

Yes it looks like they're coming from older iOS 18 betas. Thanks for the info!

@kgaidis
Copy link

kgaidis commented Sep 25, 2024

@mikeash (P.S. you don't know me, but this is full-circle for me as ~10 years ago I lived in your NSBlog reading about Objective-C internals, so thank you! 🙏 )

A crash on iOS is going to be something else

My app (iOS and iPadOS) had instant, repeating (5+ per user), withCheckedThrowingContinuation crashes when building off Xcode 16 (non-beta) for users. Had to do an emergency release with Xcode 15.4 and that seems to resolve it.

Here is a popular In App Purchase framework with this issue (you can see many references to this issue from other PR's/discussions):

Here was an attempted fix for that issue:

Some other related links about it:

Customers experiencing the crash should be able to avoid it by updating to the released version of iOS 18.

Oh that is interesting..I wonder if that's why only some experienced it 🤔

@mikeash
Copy link
Contributor

mikeash commented Sep 26, 2024

@kgaidis You're welcome! Glad you enjoyed the articles.

To elaborate a bit on what's going on, there are two unrelated issues that cause similar-looking crashes.

The issue that this PR fixes is a problem with availability checking for iOS apps running on macOS. We try to ask "are we running on iOS 18 or greater?" but end up asking the question, "are we running on macOS 18 or greater?" And the way availability works, asking about a platform that doesn't match your program always gets a "yes." Since this is an iOS app, asking about a macOS version gets a "yes." This general problem with availability isn't new, and it was an issue from macOS 11 (the first Apple Silicon release). We fixed it so that we now ask the correct "are we running on iOS 18 or greater?" question. But @backDeployed functions managed to bypass the fix. As a result, when building with Xcode 16 and running on macOS 14 or earlier, the withCheckedContinuation implementation thinks it has iOS 18 APIs available and tries to call them, and crashes.

The other issue is that the new withCheckedContinuation API in iOS 18 was changed between beta 4 and beta 5. Building against beta 5+ and running on beta 4- will result in trying to call a symbol that doesn't exist, resulting in a crash. There are no ABI stability guarantees during the beta process so this is not exactly a bug, but enough people are running on older betas to produce crashes, and without knowing about the ABI change it certainly looks like a bug.

If you're only seeing crashes on the betas then you're seeing the second one, and the fix is to update to the final 18.0 release. If you're seeing these crashes in iOS apps running on macOS 14 or earlier, then that's the bug fixed by this PR.

@jostster
Copy link

jostster commented Oct 2, 2024

We have experienced this issue when building our app with Xcode Cloud's latest xcode 16. It seems to only affect users on the latest iOS 18 version. We switched to withUnsafeContunation and so far haven't seen crashes, but haven't done extensive testing.

@elmera23
Copy link

elmera23 commented Oct 7, 2024

We have experienced this issue when building our app with Xcode Cloud's latest xcode 16. It seems to only affect users on the latest iOS 18 version. We switched to withUnsafeContunation and so far haven't seen crashes, but haven't done extensive testing.

@jostster Hi! Did this fix help on your end? Just wondering since we are encountering similar issues as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒 release cherry pick Flag: Release branch cherry picks swift 6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants