-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Add __isPlatformVersionAtLeast
and __isOSVersionAtLeast
symbols
#138944
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
base: master
Are you sure you want to change the base?
Conversation
This PR modifies cc @jieyouxu
|
__isOSVersionAtLeast
and __isPlatformVersionAtLeast
symbols__isPlatformVersionAtLeast
and __isOSVersionAtLeast
symbols
This comment has been minimized.
This comment has been minimized.
3908474
to
721f8eb
Compare
r? @tgross35 |
Happy to review the implementation, but @Amanieu mind confirming you are okay providing these symbols from |
721f8eb
to
a0061ac
Compare
This comment has been minimized.
This comment has been minimized.
a0061ac
to
19cccaf
Compare
Based on findings in rust-lang/rust#138944, we were previously incorrectly falling back to the product version of the host system.
It's a bit of a grey area since it's not clear how much it is Rust's responsibility to provide builtin symbols that are only used by C code. Arguably the more "correct" approach is to tell users of C code that we only provide symbols for Rust code and that they need to separately add a dependency on libclang_rt or libgcc, but that is a lot of hassle and thing usually just work without it. I'm not opposed to adding this in libstd for now, but it might be worth looking into a better story for how to handle builtins that are needed by C code but not Rust code (e.g. emulated TLS, clear_cache, enable_execute_stack, etc). |
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.
Some initial comments, I still need to take a closer look at the version_from*
implementations.
@@ -0,0 +1,12 @@ | |||
//! Runtime lookup of operating system / platform version. | |||
//! | |||
//! Related to [RFC 3750](https://github.com/rust-lang/rfcs/pull/3750), which |
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.
Cc RFC author @ChrisDenton just so you know this module exists
library/std/src/sys/platform_version/darwin/compiler_builtins.rs
Outdated
Show resolved
Hide resolved
library/std/src/sys/platform_version/darwin/compiler_builtins.rs
Outdated
Show resolved
Hide resolved
Thanks for the review so far. I've answered or fixed your comments in separate commits, to make it easier for you to review (if you're already halfway through a second review). I can squash once everything is ready. |
Add `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` symbols ## Motivation When Objective-C code uses ``@available(...)`,` Clang inserts a call to [`__isPlatformVersionAtLeast`](https://github.com/llvm/llvm-project/blob/llvmorg-20.1.0/compiler-rt/lib/builtins/os_version_check.c#L276) (`__isOSVersionAtLeast` in older Clang versions). These symbols not being available sometimes ends up causing linker errors. See the new test `tests/run-make/apple-c-available-links` for a minimal reproducer. The workaround is to link `libclang_rt.osx.a`, see e.g. alexcrichton/curl-rust#279. But that's very difficult for users to figure out (and the backreferences to that issue indicates that people are still running into this in their own projects every so often). For another recent example, this is preventing `rustc` from using LLVM assertions on macOS, see #62592 (comment) and #134275 (comment). It is also a blocker for [setting the correct minimum OS version in `cc-rs`](#136113), since fixing this in `cc-rs` might end up introducing linker errors in places where we weren't before (by default, if using e.g. ``@available(macos` 10.15, *)`, the symbol usually happens to be left out, since `clang` defaults to compiling for the host macOS version, and thus things _seem_ to work - but the availability check actually compiles down to nothing, which is a huge correctness footgun for running on older OSes). (My super secret evil agenda is also to expose some variant of ``@available`` in Rust's `std` after rust-lang/rfcs#3750 progresses further, will probably file an ACP for this later. But I believe this PR has value regardless of those future plans, since we'd be making C/Objective-C/Swift interop easier). ## Solution Implement `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` as part of the "public ABI" that `std` exposes. **This is insta-stable**, in the same sense that additions to `compiler-builtins` are insta-stable, though the availability of these symbols can probably be considered a "quality of implementation" detail rather than a stable promise. I originally proposed to implement this in `compiler-builtins`, see rust-lang/compiler-builtins#794, but we discussed moving it to `std` instead ([Zulip thread](https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Provide.20.60__isPlatformVersionAtLeast.60.20in.20.60std.60.3F/with/507880717)), which makes the implementation substantially simpler, and we avoid gnarly issues with requiring the user to link `libSystem.dylib` (since `std` unconditionally does that). Note that this does not solve the linker errors for (pure) `#![no_std]` users, but that's _probably_ fine, if you are using ``@available`` to test the OS version on Apple platforms, you're likely also using `std` (and it is still possible to work around by linking `libclang_rt.*.a`). A thing to note about the implementation, I've choosen to stray a bit from LLVM's upstream implementation, and not use `_availability_version_check` since [it has problems when compiling with an older SDK](llvm/llvm-project#64227). Instead, we use `sysctl kern.osproductversion` when available to still avoid the costly PList lookup in most cases, but still with a fall back to the PList lookup when that is not available (with the PList fallback being is similar to LLVM's implementation). ## Testing Apple has a lot of different "modes" that they can run binaries in, which can be a bit difficult to find your bearings in, but I've tried to be as thorough as I could in testing them all. Tested using roughly the equivalent of `./x test library/std -- platform_version` on the following configurations: - macOS 14.7.3 on a Macbook Pro M2 - `aarch64-apple-darwin` - `x86_64-apple-darwin` (under Rosetta) - `aarch64-apple-ios-macabi` - `x86_64-apple-ios-macabi` (under Rosetta) - `aarch64-apple-ios` (using Xcode's "Designed for iPad" setting) - `aarch64-apple-ios-sim` (in iOS Simulator, as iPhone with iOS 17.5) - `aarch64-apple-ios-sim` (in iOS Simulator, as iPad with iOS 18.2) - `aarch64-apple-tvos-sim` (in tvOS Simulator) - `aarch64-apple-watchos-sim` (in watchOS Simulator) - `aarch64-apple-ios-sim` (in visionOS simulator, using Xcode's "Designed for iPad" setting) - `aarch64-apple-visionos-sim` (in visionOS Simulator) - macOS 15.3.1 VM - `aarch64-apple-darwin` - `aarch64-apple-ios-macabi` - macOS 10.12.6 on an Intel Macbook from 2013 - `x86_64-apple-darwin` - `i686-apple-darwin` - `x86_64-apple-ios` (in iOS Simulator) - iOS 9.3.6 on a 1st generation iPad Mini - `armv7-apple-ios` with an older compiler Along with manually inspecting the output of `version_from_sysctl()` and `version_from_plist()`, and verifying that they actually match what's expected. I believe the only real omissions here would be: - `aarch64-apple-ios` on a newer iPhone that has `sysctl` available (iOS 11.4 or above). - `aarch64-apple-ios` on a Vision Pro using Xcode's "Designed for iPad" setting. But I don't have the hardware available to test those. `@rustbot` label O-apple A-linkage -T-compiler -A-meta -A-run-make try-job: aarch64-apple try-job: x86_64-apple*
💔 Test failed
|
5ec7157
to
779ac2f
Compare
Error was:
Fixed building test with older Clang (modified |
Add `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` symbols ## Motivation When Objective-C code uses ``@available(...)`,` Clang inserts a call to [`__isPlatformVersionAtLeast`](https://github.com/llvm/llvm-project/blob/llvmorg-20.1.0/compiler-rt/lib/builtins/os_version_check.c#L276) (`__isOSVersionAtLeast` in older Clang versions). These symbols not being available sometimes ends up causing linker errors. See the new test `tests/run-make/apple-c-available-links` for a minimal reproducer. The workaround is to link `libclang_rt.osx.a`, see e.g. alexcrichton/curl-rust#279. But that's very difficult for users to figure out (and the backreferences to that issue indicates that people are still running into this in their own projects every so often). For another recent example, this is preventing `rustc` from using LLVM assertions on macOS, see #62592 (comment) and #134275 (comment). It is also a blocker for [setting the correct minimum OS version in `cc-rs`](#136113), since fixing this in `cc-rs` might end up introducing linker errors in places where we weren't before (by default, if using e.g. ``@available(macos` 10.15, *)`, the symbol usually happens to be left out, since `clang` defaults to compiling for the host macOS version, and thus things _seem_ to work - but the availability check actually compiles down to nothing, which is a huge correctness footgun for running on older OSes). (My super secret evil agenda is also to expose some variant of ``@available`` in Rust's `std` after rust-lang/rfcs#3750 progresses further, will probably file an ACP for this later. But I believe this PR has value regardless of those future plans, since we'd be making C/Objective-C/Swift interop easier). ## Solution Implement `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` as part of the "public ABI" that `std` exposes. **This is insta-stable**, in the same sense that additions to `compiler-builtins` are insta-stable, though the availability of these symbols can probably be considered a "quality of implementation" detail rather than a stable promise. I originally proposed to implement this in `compiler-builtins`, see rust-lang/compiler-builtins#794, but we discussed moving it to `std` instead ([Zulip thread](https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Provide.20.60__isPlatformVersionAtLeast.60.20in.20.60std.60.3F/with/507880717)), which makes the implementation substantially simpler, and we avoid gnarly issues with requiring the user to link `libSystem.dylib` (since `std` unconditionally does that). Note that this does not solve the linker errors for (pure) `#![no_std]` users, but that's _probably_ fine, if you are using ``@available`` to test the OS version on Apple platforms, you're likely also using `std` (and it is still possible to work around by linking `libclang_rt.*.a`). A thing to note about the implementation, I've choosen to stray a bit from LLVM's upstream implementation, and not use `_availability_version_check` since [it has problems when compiling with an older SDK](llvm/llvm-project#64227). Instead, we use `sysctl kern.osproductversion` when available to still avoid the costly PList lookup in most cases, but still with a fall back to the PList lookup when that is not available (with the PList fallback being is similar to LLVM's implementation). ## Testing Apple has a lot of different "modes" that they can run binaries in, which can be a bit difficult to find your bearings in, but I've tried to be as thorough as I could in testing them all. Tested using roughly the equivalent of `./x test library/std -- platform_version` on the following configurations: - macOS 14.7.3 on a Macbook Pro M2 - `aarch64-apple-darwin` - `x86_64-apple-darwin` (under Rosetta) - `aarch64-apple-ios-macabi` - `x86_64-apple-ios-macabi` (under Rosetta) - `aarch64-apple-ios` (using Xcode's "Designed for iPad" setting) - `aarch64-apple-ios-sim` (in iOS Simulator, as iPhone with iOS 17.5) - `aarch64-apple-ios-sim` (in iOS Simulator, as iPad with iOS 18.2) - `aarch64-apple-tvos-sim` (in tvOS Simulator) - `aarch64-apple-watchos-sim` (in watchOS Simulator) - `aarch64-apple-ios-sim` (in visionOS simulator, using Xcode's "Designed for iPad" setting) - `aarch64-apple-visionos-sim` (in visionOS Simulator) - macOS 15.3.1 VM - `aarch64-apple-darwin` - `aarch64-apple-ios-macabi` - macOS 10.12.6 on an Intel Macbook from 2013 - `x86_64-apple-darwin` - `i686-apple-darwin` - `x86_64-apple-ios` (in iOS Simulator) - iOS 9.3.6 on a 1st generation iPad Mini - `armv7-apple-ios` with an older compiler Along with manually inspecting the output of `version_from_sysctl()` and `version_from_plist()`, and verifying that they actually match what's expected. I believe the only real omissions here would be: - `aarch64-apple-ios` on a newer iPhone that has `sysctl` available (iOS 11.4 or above). - `aarch64-apple-ios` on a Vision Pro using Xcode's "Designed for iPad" setting. But I don't have the hardware available to test those. `@rustbot` label O-apple A-linkage -T-compiler -A-meta -A-run-make try-job: aarch64-apple try-job: x86_64-apple*
💔 Test failed
|
779ac2f
to
d543096
Compare
Error was:
Implemented differently with an |
Add `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` symbols ## Motivation When Objective-C code uses ``@available(...)`,` Clang inserts a call to [`__isPlatformVersionAtLeast`](https://github.com/llvm/llvm-project/blob/llvmorg-20.1.0/compiler-rt/lib/builtins/os_version_check.c#L276) (`__isOSVersionAtLeast` in older Clang versions). These symbols not being available sometimes ends up causing linker errors. See the new test `tests/run-make/apple-c-available-links` for a minimal reproducer. The workaround is to link `libclang_rt.osx.a`, see e.g. alexcrichton/curl-rust#279. But that's very difficult for users to figure out (and the backreferences to that issue indicates that people are still running into this in their own projects every so often). For another recent example, this is preventing `rustc` from using LLVM assertions on macOS, see #62592 (comment) and #134275 (comment). It is also a blocker for [setting the correct minimum OS version in `cc-rs`](#136113), since fixing this in `cc-rs` might end up introducing linker errors in places where we weren't before (by default, if using e.g. ``@available(macos` 10.15, *)`, the symbol usually happens to be left out, since `clang` defaults to compiling for the host macOS version, and thus things _seem_ to work - but the availability check actually compiles down to nothing, which is a huge correctness footgun for running on older OSes). (My super secret evil agenda is also to expose some variant of ``@available`` in Rust's `std` after rust-lang/rfcs#3750 progresses further, will probably file an ACP for this later. But I believe this PR has value regardless of those future plans, since we'd be making C/Objective-C/Swift interop easier). ## Solution Implement `__isPlatformVersionAtLeast` and `__isOSVersionAtLeast` as part of the "public ABI" that `std` exposes. **This is insta-stable**, in the same sense that additions to `compiler-builtins` are insta-stable, though the availability of these symbols can probably be considered a "quality of implementation" detail rather than a stable promise. I originally proposed to implement this in `compiler-builtins`, see rust-lang/compiler-builtins#794, but we discussed moving it to `std` instead ([Zulip thread](https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Provide.20.60__isPlatformVersionAtLeast.60.20in.20.60std.60.3F/with/507880717)), which makes the implementation substantially simpler, and we avoid gnarly issues with requiring the user to link `libSystem.dylib` (since `std` unconditionally does that). Note that this does not solve the linker errors for (pure) `#![no_std]` users, but that's _probably_ fine, if you are using ``@available`` to test the OS version on Apple platforms, you're likely also using `std` (and it is still possible to work around by linking `libclang_rt.*.a`). A thing to note about the implementation, I've choosen to stray a bit from LLVM's upstream implementation, and not use `_availability_version_check` since [it has problems when compiling with an older SDK](llvm/llvm-project#64227). Instead, we use `sysctl kern.osproductversion` when available to still avoid the costly PList lookup in most cases, but still with a fall back to the PList lookup when that is not available (with the PList fallback being is similar to LLVM's implementation). ## Testing Apple has a lot of different "modes" that they can run binaries in, which can be a bit difficult to find your bearings in, but I've tried to be as thorough as I could in testing them all. Tested using roughly the equivalent of `./x test library/std -- platform_version` on the following configurations: - macOS 14.7.3 on a Macbook Pro M2 - `aarch64-apple-darwin` - `x86_64-apple-darwin` (under Rosetta) - `aarch64-apple-ios-macabi` - `x86_64-apple-ios-macabi` (under Rosetta) - `aarch64-apple-ios` (using Xcode's "Designed for iPad" setting) - `aarch64-apple-ios-sim` (in iOS Simulator, as iPhone with iOS 17.5) - `aarch64-apple-ios-sim` (in iOS Simulator, as iPad with iOS 18.2) - `aarch64-apple-tvos-sim` (in tvOS Simulator) - `aarch64-apple-watchos-sim` (in watchOS Simulator) - `aarch64-apple-ios-sim` (in visionOS simulator, using Xcode's "Designed for iPad" setting) - `aarch64-apple-visionos-sim` (in visionOS Simulator) - macOS 15.3.1 VM - `aarch64-apple-darwin` - `aarch64-apple-ios-macabi` - macOS 10.12.6 on an Intel Macbook from 2013 - `x86_64-apple-darwin` - `i686-apple-darwin` - `x86_64-apple-ios` (in iOS Simulator) - iOS 9.3.6 on a 1st generation iPad Mini - `armv7-apple-ios` with an older compiler Along with manually inspecting the output of `version_from_sysctl()` and `version_from_plist()`, and verifying that they actually match what's expected. I believe the only real omissions here would be: - `aarch64-apple-ios` on a newer iPhone that has `sysctl` available (iOS 11.4 or above). - `aarch64-apple-ios` on a Vision Pro using Xcode's "Designed for iPad" setting. But I don't have the hardware available to test those. `@rustbot` label O-apple A-linkage -T-compiler -A-meta -A-run-make try-job: aarch64-apple try-job: x86_64-apple*
☔ The latest upstream changes (presumably #141061) made this pull request unmergeable. Please resolve the merge conflicts. |
@madsmtm if you're blocking on this, maybe want to start a Zulip discussion? I don't really have any strong opinions here. |
I'm not sure what the status of the above is at this point, seems like there is something worth discussing? @rustbot author |
Reminder, once the PR becomes ready for a review, use |
6910b1d
to
fa56881
Compare
Allows users to link to Objective-C code using `@available(...)`.
fa56881
to
19c266b
Compare
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 have thought about it, and the only real guarantee I want from std
is that Objective-C's @available
and similar will continue to work with whatever minimum Xcode version we currently support. I have tried to encapsulate this in a comment in the code, please verify the wording.
I can still open a Zulip thread if you think it's contentious?
FYI since your last review, apart from a rebase, I also fixed the ABI of the external symbols to use i32
instead of u32
(not that it really matters in practice, the ABI is the same for these types on Apple platforms).
@rustbot ready
//! The presence of these symbols is mostly considered a quality-of-implementation detail, and | ||
//! should not be relied upon to be available. The only guarantee (or at least intended effect) is | ||
//! that linking with code built with Clang's `__builtin_available` (or similar) will continue to | ||
//! work. For example, we may decide to remove `__isOSVersionAtLeast` if support for Clang 11 | ||
//! (Xcode 11) is dropped. |
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.
Right here.
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.
So to be clear, the following C code would not be explicitly supported (though due to that being the ABI, it would work for as long as Clang 11 is supported):
int __isOSVersionAtLeast(int Major, int Minor, int Subminor);
int foo() {
return __isOSVersionAtLeast(10, 12, 0);
}
Whereas the following C code would be (as long as Rust supports interop with LLVM/Clang):
int foo() {
return __builtin_available(macos 10.12, *);
}
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.
Similar to how we do not explicitly support:
int __mulsi3(int a, int b);
int foo() {
return __mulsi3(1, 2);
}
But we do support:
int foo() {
return 1 * 2;
}
Motivation
When Objective-C code uses
@available(...)
, Clang inserts a call to__isPlatformVersionAtLeast
(__isOSVersionAtLeast
in older Clang versions). These symbols not being available sometimes ends up causing linker errors. See the new testtests/run-make/apple-c-available-links
for a minimal reproducer.The workaround is to link
libclang_rt.osx.a
, see e.g. alexcrichton/curl-rust#279. But that's very difficult for users to figure out (and the backreferences to that issue indicates that people are still running into this in their own projects every so often).For another recent example, this is preventing
rustc
from using LLVM assertions on macOS, see #62592 (comment) and #134275 (comment).It is also a blocker for setting the correct minimum OS version in
cc-rs
, since fixing this incc-rs
might end up introducing linker errors in places where we weren't before (by default, if using e.g.@available(macos 10.15, *)
, the symbol usually happens to be left out, sinceclang
defaults to compiling for the host macOS version, and thus things seem to work - but the availability check actually compiles down to nothing, which is a huge correctness footgun for running on older OSes).(My super secret evil agenda is also to expose some variant of
@available
in Rust'sstd
after rust-lang/rfcs#3750 progresses further, will probably file an ACP for this later. But I believe this PR has value regardless of those future plans, since we'd be making C/Objective-C/Swift interop easier).Solution
Implement
__isPlatformVersionAtLeast
and__isOSVersionAtLeast
as part of the "public ABI" thatstd
exposes.This is insta-stable, in the same sense that additions to
compiler-builtins
are insta-stable, though the availability of these symbols can probably be considered a "quality of implementation" detail rather than a stable promise.I originally proposed to implement this in
compiler-builtins
, see rust-lang/compiler-builtins#794, but we discussed moving it tostd
instead (Zulip thread), which makes the implementation substantially simpler, and we avoid gnarly issues with requiring the user to linklibSystem.dylib
(sincestd
unconditionally does that).Note that this does not solve the linker errors for (pure)
#![no_std]
users, but that's probably fine, if you are using@available
to test the OS version on Apple platforms, you're likely also usingstd
(and it is still possible to work around by linkinglibclang_rt.*.a
).A thing to note about the implementation, I've choosen to stray a bit from LLVM's upstream implementation, and not use
_availability_version_check
since it has problems when compiling with an older SDK. Instead, we usesysctl kern.osproductversion
when available to still avoid the costly PList lookup in most cases, but still with a fall back to the PList lookup when that is not available (with the PList fallback being is similar to LLVM's implementation).Testing
Apple has a lot of different "modes" that they can run binaries in, which can be a bit difficult to find your bearings in, but I've tried to be as thorough as I could in testing them all.
Tested using roughly the equivalent of
./x test library/std -- platform_version
on the following configurations:aarch64-apple-darwin
x86_64-apple-darwin
(under Rosetta)aarch64-apple-ios-macabi
x86_64-apple-ios-macabi
(under Rosetta)aarch64-apple-ios
(using Xcode's "Designed for iPad" setting)aarch64-apple-ios-sim
(in iOS Simulator, as iPhone with iOS 17.5)aarch64-apple-ios-sim
(in iOS Simulator, as iPad with iOS 18.2)aarch64-apple-tvos-sim
(in tvOS Simulator)aarch64-apple-watchos-sim
(in watchOS Simulator)aarch64-apple-ios-sim
(in visionOS simulator, using Xcode's "Designed for iPad" setting)aarch64-apple-visionos-sim
(in visionOS Simulator)aarch64-apple-darwin
aarch64-apple-ios-macabi
x86_64-apple-darwin
i686-apple-darwin
x86_64-apple-ios
(in iOS Simulator)armv7-apple-ios
with an older compilerAlong with manually inspecting the output of
version_from_sysctl()
andversion_from_plist()
, and verifying that they actually match what's expected.I believe the only real omissions here would be:
aarch64-apple-ios
on a newer iPhone that hassysctl
available (iOS 11.4 or above).aarch64-apple-ios
on a Vision Pro using Xcode's "Designed for iPad" setting.But I don't have the hardware available to test those.
@rustbot label O-apple A-linkage -T-compiler -A-meta -A-run-make
try-job: aarch64-apple
try-job: x86_64-apple*