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

Support Apple tvOS in libstd #103503

Merged
merged 14 commits into from
Jun 22, 2023
Merged

Support Apple tvOS in libstd #103503

merged 14 commits into from
Jun 22, 2023

Conversation

thomcc
Copy link
Member

@thomcc thomcc commented Oct 24, 2022

This target has existed in the compiler for a while, was no_std-only previously (even requiring #![feature(restricted_std)]). Apple tvOS is essentially the same as iOS, down to using the same version numbering, so there's no reason for this to be a no_std-only target the way it is currently.

Not yet tested much (I have an Apple TV, but haven't tested that this can deploy and run programs on it, nor the simulator). Uses the implementation strategy as the watchOS support in #98101 and etc. That is, no std::os:: interfaces aside from those in std::os::unix.

Includes an update to libc in order to pull in rust-lang/libc#2958.

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 24, 2022
@rust-log-analyzer

This comment has been minimized.

@thomcc
Copy link
Member Author

thomcc commented Oct 24, 2022

CI failure is due to the deprecations added in rust-lang/libc#2963... Should be fixed.

@bors
Copy link
Contributor

bors commented Nov 12, 2022

☔ The latest upstream changes (presumably #103150) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 12, 2022
@rustbot rustbot removed the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 18, 2023
@bitwalker
Copy link

I'm interested in moving this along, is there anything I can do to help out @thomcc? Looks like all that is needed right now is a rebase to get the conflicts resolved and then we're just waiting on review

@thomcc
Copy link
Member Author

thomcc commented Jan 24, 2023

Yeah and for me to make sure the tests pass, and do another once-over to make sure there's nothing I missed.

Nothing needs to be done to push it along; this is part of my project grant so I'm pretty motivated to finish it. I'll probably get to it in another week or two, though.

@bcardarella
Copy link

@thomcc thank you!

@bcardarella
Copy link

bcardarella commented Feb 16, 2023

@thomcc if you've got a full plate we can lend a hand to get this merged

@spunkedy
Copy link

Anything I can do to help test or help verify?

@thomcc
Copy link
Member Author

thomcc commented Feb 28, 2023

I'll be working on it more this weekend, sorry for the delay here everybody.

@bors
Copy link
Contributor

bors commented Mar 2, 2023

☔ The latest upstream changes (presumably #106673) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee
Copy link
Member

I tried to take a few items off ya boi's plate here, so he should be able to get to this soon. Don't worry about this PR too much. Best way to help improve tvOS support at this point is probably hunting down one of the extant iOS bugs and swatting them (due to tvOS's "totally-not-iOS" qualities), or by fixing up something that's been implemented for other OS but not Apple's, like raw-dylib. Easier to work in parallel on (related) issues that way. :3

@bcardarella
Copy link

bcardarella commented Mar 14, 2023

At the risk of sounding annoying, can we help get this done? We really need it

@thomcc
Copy link
Member Author

thomcc commented Mar 15, 2023

@bcardarella Yeah, yeah. I've been working on it, sorry for the delay.

This is ready for review, and if anybody wants to test they can knock themselves out too. I expect some amount of bugginess but didn't hit it in manual testing (more on that later).

The current version seems to work on aarch64 tvOS (aarch64-apple-tvos) and the x86_64 tvOS simulator (x86_64-apple-tvos). I would like to add compiler support for aarch64-apple-tvos-simulator soon (which should be compiler-only, and not need any special handling in the standard library).


The target definitions in the compiler needed some work in addition to the library work.

  • The existing x86_64 simulator definition had bitrotted and no longer matched LLVM's (or never did), leading to a panic if you ever try to build with it.
  • Neither of them respected TVOS_DEPLOYMENT_TARGET, which is not really optional.
  • Neither of them set a reasonable llvm_target, leading to binaries claiming that they were compatible with tvOS version 0.0.0.

In general this is basically just getting them up to snuff with the other Apple targets, and following the normal rustc_target conventions for them.

I've also gone and added an entry to the platform support docs with myself as the target maintainer. These targets already existed with no maintainer, so I don't think I need to recite the new target oath or whatever, but I'm happy to if that would help.

CC @rust-lang/compiler


The bad news: There isn't a way to run the test suites for these target, so I've just done hello world and some smoke testing. I have some thoughts on how to improve things here in general (for all the apple targets with simulators) but that will be a bit more involved. Such is life for tier3 platforms...

Thankfully, this target is very straightforward, mostly just adding cfgs to match iOS. I did take care to ensure the system types (std::os::* and others) entirely match up, and that we're only using functions which should be available on our supported versions of the target. All that came up good, probably because tvOS 7.0 and iOS 7.0 are basically the same OS as far as the functionality std cares about is concern.

Things that are less likely to work:

  • Unwinding: there are a few rare issues on macOS and iOS that cause problems with unwinding. The tvOS targets do not have the fixes for those issues. They may not have the issues either -- they're quite old issues and my testing didn't hit these, so it's possible they're fixed.
  • std::ffi::VaList and friends, especially under the simulator. I am guessing things are the same as on the relevant iOS/macOS targets, but nobody really uses this so who knows.
  • Any random API from std that I haven't tested. I have some ideas about getting the stdlib test suite inside a binary on there, but have not done so.

Anyone who hits anything like that should file bugs and CC me, of course.


I talked about this with @workingjubilee and she indicated was willing/interested in reviewing it when it's ready.

r? @workingjubilee

@thomcc thomcc marked this pull request as ready for review March 15, 2023 04:40
@rustbot
Copy link
Collaborator

rustbot commented Mar 15, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

These commits modify compiler targets.
(See the Target Tier Policy.)

@thomcc thomcc changed the title wip: Support Apple tvOS in libstd Support Apple tvOS in libstd Mar 15, 2023
@thomcc thomcc added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 15, 2023
@thomcc
Copy link
Member Author

thomcc commented Jun 21, 2023

@bors r=workingjubilee

@bors
Copy link
Contributor

bors commented Jun 21, 2023

📌 Commit af0662f has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2023
@bors
Copy link
Contributor

bors commented Jun 21, 2023

⌛ Testing commit af0662f with merge f272fc3...

@bors
Copy link
Contributor

bors commented Jun 22, 2023

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing f272fc3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 22, 2023
@bors bors merged commit f272fc3 into rust-lang:master Jun 22, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 22, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f272fc3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.6% [-1.7%, -1.5%] 2
Improvements ✅
(secondary)
-2.4% [-2.8%, -2.0%] 2
All ❌✅ (primary) -1.6% [-1.7%, -1.5%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.1% [-2.1%, -2.1%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 658.022s -> 659.342s (0.20%)

@hakonk
Copy link

hakonk commented Jun 25, 2023

@thomcc This is great! Thanks for merging!

@thomcc thomcc deleted the tvos-support branch June 25, 2023 08:12
@Blankwonder
Copy link

Thank so much for your work.

I tried to compile a static library for tvOS with the nightly toolchain (rustc 1.72.0-nightly (5ea6668 2023-06-27)), but encountered a lot of errors when compiling libc. Can you please advise me on how to resolve this?

Error example:

error[E0412]: cannot find type `c_char` in the crate root
    --> /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.112/src/unix/mod.rs:1358:49
     |
6    | pub type c_schar = i8;
     | ---------------------- similarly named type alias `c_schar` defined here
...
1358 |     pub fn tmpnam(ptr: *mut ::c_char) -> *mut ::c_char;
     |                                                 ^^^^^^
     |
help: a type alias with a similar name exists
     |
1358 |     pub fn tmpnam(ptr: *mut ::c_char) -> *mut ::c_schar;
     |                                                 ~~~~~~~
help: consider importing this type alias
     |
1485 +         use ffi::c_char;
     |
help: if you import `c_char`, refer to it directly
     |
1358 -     pub fn tmpnam(ptr: *mut ::c_char) -> *mut ::c_char;
1358 +     pub fn tmpnam(ptr: *mut ::c_char) -> *mut c_char;
     |

@thomcc
Copy link
Member Author

thomcc commented Jun 28, 2023

Interesting. This wasn't an issue when landing it. Just to check, you're building with -Zbuild-std or similar right?

@Blankwonder
Copy link

Blankwonder commented Jun 28, 2023

Correct. Since rustup doesn't support to install the pre-built std currently, I had to compile it myself.

~ rustup target add aarch64-apple-tvos --toolchain nightly
error: toolchain 'nightly-aarch64-apple-darwin' does not contain component 'rust-std' for target 'aarch64-apple-tvos'; did you mean 'aarch64-apple-ios'?
note: not all platforms have the standard library pre-compiled: https://doc.rust-lang.org/nightly/rustc/platform-support.html
help: consider using `cargo build -Z build-std` instead

The build command is :

cargo +nightly build -Z build-std --target aarch64-apple-tvos

@thomcc
Copy link
Member Author

thomcc commented Jun 28, 2023

Yeah, that (rustup target add not working) is normal. The issue is concerning then. It's likely some patch is needed to the libc crate. I can look at it tomorrow (hopefully).

@workingjubilee
Copy link
Member

Note that the libc version in question is 0.2.118, not our main branch's 0.2.146 or the latest 0.2.147 release, so any fix applied to that crate will not affect the version in question. I am forced to wonder if it replicates with a newer version at all?

@thomcc
Copy link
Member Author

thomcc commented Jun 28, 2023

Oh, that's a good point. I had assumed this error was occurring during the std-building part of build-std. If this is in another crate, you'll need to ensure your dependency graph is pulling in a suitably new version of libc. You can probably force this by running something like cargo update -p libc --precise 0.2.147 (to update your Cargo.lock), or adding libc = "0.2.147" in your Cargo.toml.

@workingjubilee
Copy link
Member

If both of those foil you there is also the "heavy" solution of using [patch.crates-io] which can, rarely, be necessary to override a dependency specified by another dependency when an... incorrectly precise version is specified. But hopefully such is not needed.

@Blankwonder
Copy link

Blankwonder commented Jun 28, 2023

Thanks again to both of you. I was not aware that the libc crate also needed to be upgraded in order to compile. After adjusting the dependency version, the build was successfully completed.

But when compiling the ring crate, I encountered other problems. It seems to be due to some reasons of the project itself. I will submit an issue to the project later to confirm.

error: failed to run custom build command for `ring v0.16.20`

Caused by:
  process didn't exit successfully: `/Users/blankwonder/Workspaces/src/target/release/build/ring-45c08debc9a67511/build-script-build` (exit status: 101)
  --- stdout
  OPT_LEVEL = Some("3")
  TARGET = Some("aarch64-apple-tvos")
  HOST = Some("aarch64-apple-darwin")
  cargo:rerun-if-env-changed=CC_aarch64-apple-tvos
  CC_aarch64-apple-tvos = None
  cargo:rerun-if-env-changed=CC_aarch64_apple_tvos
  CC_aarch64_apple_tvos = None
  cargo:rerun-if-env-changed=TARGET_CC
  TARGET_CC = None
  cargo:rerun-if-env-changed=CC
  CC = None
  RUSTC_LINKER = None
  cargo:rerun-if-env-changed=CROSS_COMPILE
  CROSS_COMPILE = None
  cargo:rerun-if-env-changed=CFLAGS_aarch64-apple-tvos
  CFLAGS_aarch64-apple-tvos = None
  cargo:rerun-if-env-changed=CFLAGS_aarch64_apple_tvos
  CFLAGS_aarch64_apple_tvos = None
  cargo:rerun-if-env-changed=TARGET_CFLAGS
  TARGET_CFLAGS = None
  cargo:rerun-if-env-changed=CFLAGS
  CFLAGS = None
  cargo:rerun-if-env-changed=CRATE_CC_NO_DEFAULTS
  CRATE_CC_NO_DEFAULTS = None
  DEBUG = Some("false")
  CARGO_CFG_TARGET_FEATURE = Some("aes,neon,pmuv3,sha2")

  --- stderr
  running "cc" "-O3" "-ffunction-sections" "-fdata-sections" "-fPIC" "-I" "include" "-Wall" "-Wextra" "-pedantic" "-pedantic-errors" "-Wall" "-Wextra" "-Wcast-align" "-Wcast-qual" "-Wconversion" "-Wenum-compare" "-Wfloat-equal" "-Wformat=2" "-Winline" "-Winvalid-pch" "-Wmissing-field-initializers" "-Wmissing-include-dirs" "-Wredundant-decls" "-Wshadow" "-Wsign-compare" "-Wsign-conversion" "-Wundef" "-Wuninitialized" "-Wwrite-strings" "-fno-strict-aliasing" "-fvisibility=hidden" "-fstack-protector" "-g3" "-DNDEBUG" "-c" "-o/Users/blankwonder/Workspaces/src/boringtun/target/aarch64-apple-tvos/release/build/ring-92f391d6db1c5482/out/aesv8-armx-linux64.o" "/Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S"
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:18:17: error: unexpected token in '.section' directive
  .section .rodata
                  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:28:1: error: unknown directive
  .hidden GFp_aes_hw_set_encrypt_key
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:29:1: error: unknown directive
  .type GFp_aes_hw_set_encrypt_key,%function
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:163:1: error: unknown directive
  .size GFp_aes_hw_set_encrypt_key,.-GFp_aes_hw_set_encrypt_key
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:165:1: error: unknown directive
  .hidden GFp_aes_hw_encrypt
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:166:1: error: unknown directive
  .type GFp_aes_hw_encrypt,%function
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:194:1: error: unknown directive
  .size GFp_aes_hw_encrypt,.-GFp_aes_hw_encrypt
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:196:1: error: unknown directive
  .hidden GFp_aes_hw_decrypt
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:197:1: error: unknown directive
  .type GFp_aes_hw_decrypt,%function
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:225:1: error: unknown directive
  .size GFp_aes_hw_decrypt,.-GFp_aes_hw_decrypt
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:227:1: error: unknown directive
  .hidden GFp_aes_hw_ctr32_encrypt_blocks
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:228:1: error: unknown directive
  .type GFp_aes_hw_ctr32_encrypt_blocks,%function
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:426:1: error: unknown directive
  .size GFp_aes_hw_ctr32_encrypt_blocks,.-GFp_aes_hw_ctr32_encrypt_blocks
  ^
  /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/pregenerated/aesv8-armx-linux64.S:430:19: error: unexpected token in '.section' directive
  .section .note.GNU-stack,"",%progbits
                    ^
  thread 'main' panicked at 'execution failed', /Users/blankwonder/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ring-0.16.20/build.rs:656:9
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@workingjubilee
Copy link
Member

It looks like maybe ring is using... nonportable assembler directives, perhaps? Or the build was supposed to use a different assembler than the one it actually had access to. Either way, it indeed probably isn't anything to fix in std.

@Blankwonder
Copy link

You are right. I made a patch to fix the issue by adding the missing judgment about the target tvos.

However, in nix cargo, similar problems are more serious. There are a large number of checks in the code to determine whether the target is ios, and it is necessary to add checks for tvos at the same time in order to compile.

@thomcc
Copy link
Member Author

thomcc commented Jun 28, 2023

Yes, there's nothing std can do about this though. It will also have to be updated again when we add support for Apple Vision (which may be target_os = "visionos" or target_os = "xros" depending on upstream, and I would not recommend trying to guess which one it would be in advance).

It's a bit unfortunate, but code wanting to future-proof might want to use target_vendor = "apple". This obviously could cause issues if Apple releases something unrelated, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-tvos Operating system: tvOS (including simulator) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.