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

Move Apple linker args from rustc_target to rustc_codegen_ssa #130435

Merged
merged 1 commit into from
Sep 27, 2024

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Sep 16, 2024

They are dependent on the deployment target and SDK version, but having these in rustc_target makes it hard to introduce that dependency. Part of the work needed to do #118204, see #129342 for some discussion.

Tested using:

./x test tests/run-make/apple-deployment-target --target="aarch64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-macabi,aarch64-apple-ios-sim,aarch64-apple-tvos,aarch64-apple-tvos-sim,aarch64-apple-visionos,aarch64-apple-visionos-sim,aarch64-apple-watchos,aarch64-apple-watchos-sim,arm64_32-apple-watchos,armv7k-apple-watchos,armv7s-apple-ios,x86_64-apple-darwin,x86_64-apple-ios,x86_64-apple-ios-macabi,x86_64-apple-tvos,x86_64-apple-watchos-sim,x86_64h-apple-darwin"
IPHONEOS_DEPLOYMENT_TARGET=10.0 ./x test tests/run-make/apple-deployment-target --target=i386-apple-ios

arm64e-apple-darwin and arm64e-apple-ios have not been tested, see #130085, neither is i686-apple-darwin, since that requires using an x86_64 macbook, and I currently can't get mine to work, see #130434.

CC @petrochenkov

@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) 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 Sep 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 16, 2024

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

@petrochenkov petrochenkov self-assigned this Sep 16, 2024
@arttet
Copy link
Contributor

arttet commented Sep 16, 2024

Hey @madsmtm,

I will appreciate it if you test arm64e-apple-darwin and arm64e-apple-ios. Here is a fix for arm64e-apple-darwin. Today, I have built arm64e-apple-ios successfully for 15.2 Xcode on macOS 13.

@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 16, 2024

Hey @madsmtm,

I will appreciate it if you test arm64e-apple-darwin and arm64e-apple-ios. Here is a fix for arm64e-apple-darwin. Today, I have built arm64e-apple-ios successfully for 15.2 Xcode on macOS 13.

Nice! I will in the future once that lands (though it shouldn't block this and other PRs)

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2024
@bors

This comment was marked as resolved.

They are dependent on the deployment target and SDK version, but having
these in `rustc_target` makes it hard to introduce that dependency.
@madsmtm madsmtm force-pushed the move-apple-link-args branch from 72af021 to fb10eeb Compare September 26, 2024 14:44
@madsmtm
Copy link
Contributor Author

madsmtm commented Sep 26, 2024

Rebased and resolved review

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 26, 2024
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 27, 2024

📌 Commit fb10eeb has been approved by petrochenkov

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 Sep 27, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#129087 (Stabilize `option_get_or_insert_default`)
 - rust-lang#130435 (Move Apple linker args from `rustc_target` to `rustc_codegen_ssa`)
 - rust-lang#130459 (delete sub build directory "debug" to not delete the change-id file)
 - rust-lang#130873 (rustc_target: Add powerpc64 atomic-related features)
 - rust-lang#130916 (Use `&raw` in the compiler)
 - rust-lang#130917 (Fix error span if arg to `asm!()` is a macro call)
 - rust-lang#130927 (update outdated comments)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f9cd81f into rust-lang:master Sep 27, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 27, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2024
Rollup merge of rust-lang#130435 - madsmtm:move-apple-link-args, r=petrochenkov

Move Apple linker args from `rustc_target` to `rustc_codegen_ssa`

They are dependent on the deployment target and SDK version, but having these in `rustc_target` makes it hard to introduce that dependency. Part of the work needed to do rust-lang#118204, see rust-lang#129342 for some discussion.

Tested using:
```console
./x test tests/run-make/apple-deployment-target --target="aarch64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-macabi,aarch64-apple-ios-sim,aarch64-apple-tvos,aarch64-apple-tvos-sim,aarch64-apple-visionos,aarch64-apple-visionos-sim,aarch64-apple-watchos,aarch64-apple-watchos-sim,arm64_32-apple-watchos,armv7k-apple-watchos,armv7s-apple-ios,x86_64-apple-darwin,x86_64-apple-ios,x86_64-apple-ios-macabi,x86_64-apple-tvos,x86_64-apple-watchos-sim,x86_64h-apple-darwin"
IPHONEOS_DEPLOYMENT_TARGET=10.0 ./x test tests/run-make/apple-deployment-target --target=i386-apple-ios
```

`arm64e-apple-darwin` and `arm64e-apple-ios` have not been tested, see rust-lang#130085, neither is `i686-apple-darwin`, since that requires using an x86_64 macbook, and I currently can't get mine to work, see rust-lang#130434.

CC `@petrochenkov`
@madsmtm madsmtm deleted the move-apple-link-args branch September 28, 2024 05:47
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 2, 2024
…r=jieyouxu

Apple: Do not specify an SDK version in `rlib` object files

This was added in rust-lang#114114, but is unnecessary, since it ends up being overwritten when linking anyhow, and it feels wrong to embed some arbitrary SDK version in here. The object files produced by LLVM also do not set this, and the tooling shows `n/a` when it's `0`, so it seems to genuinely be optional in object files.

I've also added a test for the different places the SDK version shows up, and documented a bit more in the code how SDK versions work.

See rust-lang#129432 for the bigger picture.

Tested with (excludes the same few targets as in rust-lang#130435):
```console
./x test tests/run-make/apple-sdk-version --target aarch64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-macabi,aarch64-apple-ios-sim,aarch64-apple-tvos,aarch64-apple-tvos-sim,aarch64-apple-visionos,aarch64-apple-visionos-sim,aarch64-apple-watchos,aarch64-apple-watchos-sim,arm64_32-apple-watchos,armv7k-apple-watchos,armv7s-apple-ios,x86_64-apple-darwin,x86_64-apple-ios,x86_64-apple-ios-macabi,x86_64-apple-tvos,x86_64-apple-watchos-sim,x86_64h-apple-darwin
IPHONEOS_DEPLOYMENT_TARGET=10.0 ./x test tests/run-make/apple-sdk-version --target=i386-apple-ios
```

CC `@BlackHoleFox,` you [originally commented on these values](rust-lang#114114 (comment)).

`@rustbot` label O-apple
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2024
Rollup merge of rust-lang#131016 - madsmtm:no-sdk-version-in-object, r=jieyouxu

Apple: Do not specify an SDK version in `rlib` object files

This was added in rust-lang#114114, but is unnecessary, since it ends up being overwritten when linking anyhow, and it feels wrong to embed some arbitrary SDK version in here. The object files produced by LLVM also do not set this, and the tooling shows `n/a` when it's `0`, so it seems to genuinely be optional in object files.

I've also added a test for the different places the SDK version shows up, and documented a bit more in the code how SDK versions work.

See rust-lang#129432 for the bigger picture.

Tested with (excludes the same few targets as in rust-lang#130435):
```console
./x test tests/run-make/apple-sdk-version --target aarch64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-macabi,aarch64-apple-ios-sim,aarch64-apple-tvos,aarch64-apple-tvos-sim,aarch64-apple-visionos,aarch64-apple-visionos-sim,aarch64-apple-watchos,aarch64-apple-watchos-sim,arm64_32-apple-watchos,armv7k-apple-watchos,armv7s-apple-ios,x86_64-apple-darwin,x86_64-apple-ios,x86_64-apple-ios-macabi,x86_64-apple-tvos,x86_64-apple-watchos-sim,x86_64h-apple-darwin
IPHONEOS_DEPLOYMENT_TARGET=10.0 ./x test tests/run-make/apple-sdk-version --target=i386-apple-ios
```

CC `@BlackHoleFox,` you [originally commented on these values](rust-lang#114114 (comment)).

`@rustbot` label O-apple
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? `@petrochenkov`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ```@petrochenkov```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ````@petrochenkov````
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? `````@petrochenkov`````
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ``````@petrochenkov``````
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
Rollup merge of rust-lang#131037 - madsmtm:move-llvm-target-versioning, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ``````@petrochenkov``````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) 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.

6 participants