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

Remove hardcoded iOS version of clang target for Mac Catalyst #106925

Conversation

imWildCat
Copy link
Contributor

@imWildCat imWildCat commented Jan 16, 2023

Background

From clang 13.x, -target x86_64-apple-ios13.0-macabi fails while linking:

  = note: clang: error: invalid version number in '-target x86_64-apple-ios13.0-macabi'
Verbose output
error: linking with `cc` failed: exit status: 1
  |
  = note: LC_ALL="C" PATH="[removed]" VSLANG="1033" ZERO_AR_DATE="1" "cc" "-Wl,-exported_symbols_list,/var/folders/p8/qpmzbsdn07g5gxykwfxxw7y40000gn/T/rustci8tkvp/list" "-target" "x86_64-apple-ios13.0-macabi" "/var/folders/p8/qpmzbsdn07g5gxykwfxxw7y40000gn/T/rustci8tkvp/symbols.o" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps/[user].[user].a2ccc648-cgu.0.rcgu.o" "-L" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps" "-L" "/path/to/my/[project]/[user]/target/release/deps" "-L" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/build/blake3-74e6ba91506ce712/out" "-L" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/build/blake3-74e6ba91506ce712/out" "-L" "/Users/[user]/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/x86_64-apple-ios-macabi/lib" "/var/folders/p8/qpmzbsdn07g5gxykwfxxw7y40000gn/T/rustci8tkvp/libblake3-343c1616c8f62c66.rlib" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps/libcompiler_builtins-15d4f20b641cf9ef.rlib" "-framework" "Security" "-framework" "CoreFoundation" "-framework" "Security" "-liconv" "-lSystem" "-lobjc" "-framework" "Security" "-framework" "Foundation" "-lc" "-lm" "-isysroot" "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk" "-Wl,-syslibroot" "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk" "-L" "/Users/[user]/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/x86_64-apple-ios-macabi/lib" "-o" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps/lib[user].dylib" "-Wl,-dead_strip" "-dynamiclib" "-Wl,-dylib" "-nodefaultlibs"
  = note: clang: error: invalid version number in '-target x86_64-apple-ios13.0-macabi'
          

warning: `[user]` (lib) generated 6 warnings
error: could not compile `[user]` due to previous error; 6 warnings emitted

Minimal example

C code:

#include <stdio.h>
void main() {
    int a = 1;
    int b = 2;
    int c = a + b;
    printf("%d", c);
}

clang command sample:

➜  202301 clang -target x86_64-apple-ios13.0-macabi main.c
clang: error: invalid version number in '-target x86_64-apple-ios13.0-macabi'
➜  202301 clang -target x86_64-apple-ios14.0-macabi main.c
main.c:2:1: warning: return type of 'main' is not 'int' [-Wmain-return-type]
void main() {
^
main.c:2:1: note: change return type to 'int'
void main() {
^~~~
int
1 warning generated.
➜  202301 clang -target x86_64-apple-ios15.0-macabi main.c
main.c:2:1: warning: return type of 'main' is not 'int' [-Wmain-return-type]
void main() {
^
main.c:2:1: note: change return type to 'int'
void main() {
^~~~
int
1 warning generated.
➜  202301 clang -target x86_64-apple-ios-macabi main.c
main.c:2:1: warning: return type of 'main' is not 'int' [-Wmain-return-type]
void main() {
^
main.c:2:1: note: change return type to 'int'
void main() {
^~~~
int
1 warning generated.

➜  202301 clang --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin22.2.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

This PR is a simplified version of #96392, inspired by rust-lang/cc-rs#727

@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

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

@imWildCat imWildCat changed the title Remove hardcoded clang target: ios13 or ios14 for Mac Catalyst Remove hardcoded iOS version of clang target for Mac Catalyst Jan 16, 2023
@imWildCat
Copy link
Contributor Author

gentle @rustbot ping macos

@rustbot
Copy link
Collaborator

rustbot commented Jan 20, 2023

Error: Only Rust team members can ping teams.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@imWildCat
Copy link
Contributor Author

Hello @thomcc , sorry to bother! Could you help me to ping macos? I saw you did this here: #96392 (comment)

@thomcc thomcc added the O-macos Operating system: macOS label Jan 20, 2023
@thomcc
Copy link
Member

thomcc commented Jan 20, 2023

Sure. I'll also try to take a look later (my project grant covers improving rust support for apple targets).

@rustbot ping macos

@rustbot
Copy link
Collaborator

rustbot commented Jan 20, 2023

Hey MacOS Group! This issue or PR could use some MacOS-specific guidance. Could one
of you weigh in? Thanks <3

cc @hkratz @inflation @nvzqz @shepmaster @thomcc

@imWildCat
Copy link
Contributor Author

Thank you so much! No rush at all!

@imWildCat
Copy link
Contributor Author

Hello @hkratz @inflation @nvzqz @shepmaster @thomcc,

May I have some feedbacks of this PR? Thanks in advance!

@inflation
Copy link

Do you know from which Apple LLVM version that the target triple has changed? Because I think we pass the target to the linker and this change could break on older Xcode or CLT.

@imWildCat
Copy link
Contributor Author

imWildCat commented Jan 26, 2023

Do you know from which Apple LLVM version that the target triple has changed? Because I think we pass the target to the linker and this change could break on older Xcode or CLT.

Good question!

I think the first version to see this regression is Xcode with clang 13. I could only remember that I saw this since Xcode 13.2 or 13.3.
But from this list (https://gist.github.com/yamaya/2924292), it is 13.0.

It won't be too difficult to test supportability of these triples:

  1. x86_64-apple-ios13.0-macabi
  2. x86_64-apple-ios14.0-macabi
  3. x86_64-apple-ios-macabi, without a specific version

Update: it won't make sense if I just talk about it here. So I did some experiments:

Sample build using macOS 11/12 with Xcode 13.0 to 14.2.0: https://github.com/imWildCat-archived/clang-triple-catalyst-test/actions/runs/4012618388/jobs/6891193915
Source: https://github.com/imWildCat-archived/clang-triple-catalyst-test/blob/main/.github/workflows/xcode.yaml
image

To sum up, catalyst triples without specifying iOS version (e.g. x86_64-apple-ios-macabi) can work in all version of clang. And the first clang version which cannot handle x86_64-apple-ios13.0-macabi is 13.1.x.

@thomcc
Copy link
Member

thomcc commented Jan 26, 2023

I'm inclined to suggest that it's a tier3 platform so compatibility with very old toolchains (especially at the cost of new ones) might be too much for users to expect, so we should probably just take this even if it causes issues with some old toolchains (that said it's obviously not my call to make).

That said, if someone with a wider number of XCode toolchains installed (I think @BlackHoleFox might have this setup (or did at one point), but I could also do so if they're too busy) could test on some older ones to see if this does indeed work there, that'd be useful.

@imWildCat
Copy link
Contributor Author

imWildCat commented Jan 26, 2023

Thanks for your review, @thomcc!

To address your concerns, I think we can use relatively old toolchains available on GitHub actions to see the result: https://github.com/imWildCat-archived/clang-triple-catalyst-test/actions/runs/4016723276/jobs/6900131561
(Please ignore failed tests, they are all due to unsupported triple, which is x86_64-apple-ios13.0-macabi)
Details are in the "Build" step.

I also want to test Xcode 11.0, which is the version Apple toolchain that supports Mac Catalyst. However, we don't have macOS 10.14 on GitHub Actions (actions/runner-images#2347 (comment)).
So I can only test Xcode 11.2.1. According to https://gist.github.com/yamaya/2924292, there's no much difference between the clang versions between Xcode 11.0 and Xcode 11.2.1:

# Xcode 11.0 (11A420a)
Apple clang version 11.0.0 (clang-1100.0.33.8)
# Xcode 11.2.1 (11B500)
Apple clang version 11.0.0 (clang-1100.0.33.12)

From my tests, we can see:

  1. x86_64-apple-ios13.0-macabi is not supported anymore starting Xcode 13.3, which was released over a year ago.
    • which means Rust users on macOS with newer version of Xcode cannot target Mac Catalyst.
  2. x86_64-apple-ios-macabi is supported since the very beginning. We don't need to worry about it.

@imWildCat
Copy link
Contributor Author

@thomcc Hello! Let me add another update:
I also created a virtualized macOS 10.14, installed Xcode 11.0 (first version that can target Mac Catalyst), did the same experiment. It works with a little warning line:

ld: warning: OS version (5.0.0) too small, changing to 13.0.0

image
Screen Shot 2023-01-26 at 10 25 41 PM

@imWildCat imWildCat force-pushed the imWildCat/remove-hardcoded-ios-macbi-target-version branch from 0d0a635 to 5209d6f Compare January 27, 2023 07:29
@terhechte
Copy link
Contributor

arm64-apple-ios-macabi is the much better choice. If I remember correctly, arm64-apple-ios13.0-macabi was only chosen as those flag came straight out of Xcode 11. Thanks @imWildCat !

@imWildCat
Copy link
Contributor Author

Hello @thomcc @inflation , may I have another round of review, given the conversation above?

I've got another review from @terhechte .

Thanks in advance!

@thomcc
Copy link
Member

thomcc commented Jan 31, 2023

This looks good to me, but @wesleywiser is the assigned reviewer (I'm not a compiler reviewer).

@inflation
Copy link

LGTM

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a mac setup to test this but the testing in this thread seems sufficient for a Tier 3 target to me.

@wesleywiser
Copy link
Member

Thanks @imWildCat!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 1, 2023

📌 Commit 5209d6f has been approved by wesleywiser

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 Feb 1, 2023
@bors
Copy link
Contributor

bors commented Feb 1, 2023

⌛ Testing commit 5209d6f with merge 9d0db326e679bfb0b7339568d3a55a8e5fc80b63...

@bors
Copy link
Contributor

bors commented Feb 1, 2023

💔 Test failed - checks-actions

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

@bors retry CI was broken

@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 Feb 1, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Feb 2, 2023
…ed-ios-macbi-target-version, r=wesleywiser

Remove hardcoded iOS version of clang target for Mac Catalyst

## Background

From `clang` 13.x, `-target x86_64-apple-ios13.0-macabi` fails while linking:

```
  = note: clang: error: invalid version number in '-target x86_64-apple-ios13.0-macabi'
```

<details>
<summary>Verbose output</summary>

```
error: linking with `cc` failed: exit status: 1
  |
  = note: LC_ALL="C" PATH="[removed]" VSLANG="1033" ZERO_AR_DATE="1" "cc" "-Wl,-exported_symbols_list,/var/folders/p8/qpmzbsdn07g5gxykwfxxw7y40000gn/T/rustci8tkvp/list" "-target" "x86_64-apple-ios13.0-macabi" "/var/folders/p8/qpmzbsdn07g5gxykwfxxw7y40000gn/T/rustci8tkvp/symbols.o" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps/[user].[user].a2ccc648-cgu.0.rcgu.o" "-L" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps" "-L" "/path/to/my/[project]/[user]/target/release/deps" "-L" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/build/blake3-74e6ba91506ce712/out" "-L" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/build/blake3-74e6ba91506ce712/out" "-L" "/Users/[user]/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/x86_64-apple-ios-macabi/lib" "/var/folders/p8/qpmzbsdn07g5gxykwfxxw7y40000gn/T/rustci8tkvp/libblake3-343c1616c8f62c66.rlib" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps/libcompiler_builtins-15d4f20b641cf9ef.rlib" "-framework" "Security" "-framework" "CoreFoundation" "-framework" "Security" "-liconv" "-lSystem" "-lobjc" "-framework" "Security" "-framework" "Foundation" "-lc" "-lm" "-isysroot" "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk" "-Wl,-syslibroot" "/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk" "-L" "/Users/[user]/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/x86_64-apple-ios-macabi/lib" "-o" "/path/to/my/[project]/[user]/target/x86_64-apple-ios-macabi/release/deps/lib[user].dylib" "-Wl,-dead_strip" "-dynamiclib" "-Wl,-dylib" "-nodefaultlibs"
  = note: clang: error: invalid version number in '-target x86_64-apple-ios13.0-macabi'

warning: `[user]` (lib) generated 6 warnings
error: could not compile `[user]` due to previous error; 6 warnings emitted
```
</details>

### Minimal example

C code:

```c
#include <stdio.h>
void main() {
    int a = 1;
    int b = 2;
    int c = a + b;
    printf("%d", c);
}
```

`clang` command sample:

```
➜  202301 clang -target x86_64-apple-ios13.0-macabi main.c
clang: error: invalid version number in '-target x86_64-apple-ios13.0-macabi'
➜  202301 clang -target x86_64-apple-ios14.0-macabi main.c
main.c:2:1: warning: return type of 'main' is not 'int' [-Wmain-return-type]
void main() {
^
main.c:2:1: note: change return type to 'int'
void main() {
^~~~
int
1 warning generated.
➜  202301 clang -target x86_64-apple-ios15.0-macabi main.c
main.c:2:1: warning: return type of 'main' is not 'int' [-Wmain-return-type]
void main() {
^
main.c:2:1: note: change return type to 'int'
void main() {
^~~~
int
1 warning generated.
➜  202301 clang -target x86_64-apple-ios-macabi main.c
main.c:2:1: warning: return type of 'main' is not 'int' [-Wmain-return-type]
void main() {
^
main.c:2:1: note: change return type to 'int'
void main() {
^~~~
int
1 warning generated.

➜  202301 clang --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: arm64-apple-darwin22.2.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
```

This PR is a simplified version of rust-lang#96392, inspired by rust-lang/cc-rs#727
@bors
Copy link
Contributor

bors commented Feb 2, 2023

⌛ Testing commit 5209d6f with merge 821b2a8...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Feb 2, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 821b2a8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 2, 2023
@bors bors merged commit 821b2a8 into rust-lang:master Feb 2, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 2, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (821b2a8): 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)
- - 0
Improvements ✅
(secondary)
-3.0% [-3.0%, -3.0%] 1
All ❌✅ (primary) - - 0

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)
2.9% [2.9%, 2.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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-macos Operating system: macOS 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants