Pass deployment target when linking with CC on Apple targets #129369

madsmtm · 2024-08-21T19:01:57Z

This PR effectively implements what's also being considered in the cc crate here, that is:

When linking macOS targets with CC, pass the -mmacosx-version-min=. option to specify the desired deployment target. Also, no longer pass -m32/-m64, these are redundant since we already pass -arch.
When linking with CC on iOS, tvOS, watchOS and visionOS, only pass -target (we assume for these targets that CC forwards to Clang).

This is required to get the linker to emit the correct LC_BUILD_VERSION of the final binary. See #129432 for more motivation behind this change.

r? compiler

CC @BlackHoleFox

madsmtm · 2024-08-22T15:19:54Z

I'd recommend using @bors rollup=never on this one, we might need to bisect it later on if linking fails using some configuration that I haven't tested / thought of.

bors · 2024-08-23T21:47:06Z

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

BlackHoleFox

Apple-specific stuff makes sense to me. The LLVM target, since we have it, is always going to be the most accurate value to give cc and results in the least amount of duplicate work in rustc for passing the same info.

…jieyouxu Test codegen when setting deployment target Test our codegen in different scenarios when setting the deployment target. There are many places here where this is still incorrect, these will be fixed in rust-lang#129342, rust-lang#129367 and rust-lang#129369. See rust-lang#129432 for the bigger picture. Tested locally 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,armv7s-apple-ios,i386-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" ``` The only Apple targets that aren't tested by the above command are: - `arm64e-apple-darwin`, failed to build, see rust-lang/cc-rs#1205. - `armv7k-apple-watchos`, failed to link, see rust-lang#130071. - `arm64e-apple-ios`, failed to link, see rust-lang#130085. - `i686-apple-darwin`, requires a bit of setup and an older machine, see [the docs](https://doc.rust-lang.org/nightly/rustc/platform-support/i686-apple-darwin.html). - `i386-apple-ios` requires you to set `IPHONEOS_DEPLOYMENT_TARGET=10.0` for the test helpers to work, will be fixed by rust-lang/cc-rs#1030. But all of this is as it was before this PR. Fixes rust-lang#47825, since we now have a test that compiles a `dylib` for `aarch64-apple-ios`. Split out from rust-lang#129342, see that for a little bit of the review that this has gone through already. r? petrochenkov `@rustbot` label O-apple

…jieyouxu Test codegen when setting deployment target Test our codegen in different scenarios when setting the deployment target. There are many places here where this is still incorrect, these will be fixed in rust-lang#129342, rust-lang#129367 and rust-lang#129369. See rust-lang#129432 for the bigger picture. Tested locally 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,armv7s-apple-ios,i386-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" ``` The only Apple targets that aren't tested by the above command are: - `arm64e-apple-darwin`, failed to build, see rust-lang/cc-rs#1205. - `armv7k-apple-watchos`, failed to link, see rust-lang#130071. - `arm64e-apple-ios`, failed to link, see rust-lang#130085. - `i686-apple-darwin`, requires a bit of setup and an older machine, see [the docs](https://doc.rust-lang.org/nightly/rustc/platform-support/i686-apple-darwin.html). - `i386-apple-ios` requires you to set `IPHONEOS_DEPLOYMENT_TARGET=10.0` for the test helpers to work, will be fixed by rust-lang/cc-rs#1030. But all of this is as it was before this PR. Fixes rust-lang#47825, since we now have a test that compiles a `dylib` for `aarch64-apple-ios`. Split out from rust-lang#129342, see that for a little bit of the review that this has gone through already. r? petrochenkov ``@rustbot`` label O-apple

…jieyouxu Test codegen when setting deployment target Test our codegen in different scenarios when setting the deployment target. There are many places here where this is still incorrect, these will be fixed in rust-lang#129342, rust-lang#129367 and rust-lang#129369. See rust-lang#129432 for the bigger picture. Tested locally 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,armv7s-apple-ios,i386-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" ``` The only Apple targets that aren't tested by the above command are: - `arm64e-apple-darwin`, failed to build, see rust-lang/cc-rs#1205. - `armv7k-apple-watchos`, failed to link, see rust-lang#130071. - `arm64e-apple-ios`, failed to link, see rust-lang#130085. - `i686-apple-darwin`, requires a bit of setup and an older machine, see [the docs](https://doc.rust-lang.org/nightly/rustc/platform-support/i686-apple-darwin.html). - `i386-apple-ios` requires you to set `IPHONEOS_DEPLOYMENT_TARGET=10.0` for the test helpers to work, will be fixed by rust-lang/cc-rs#1030. But all of this is as it was before this PR. Fixes rust-lang#47825, since we now have a test that compiles a `dylib` for `aarch64-apple-ios`. Split out from rust-lang#129342, see that for a little bit of the review that this has gone through already. r? petrochenkov ```@rustbot``` label O-apple

Rollup merge of rust-lang#130068 - madsmtm:deployment-target-test, r=jieyouxu Test codegen when setting deployment target Test our codegen in different scenarios when setting the deployment target. There are many places here where this is still incorrect, these will be fixed in rust-lang#129342, rust-lang#129367 and rust-lang#129369. See rust-lang#129432 for the bigger picture. Tested locally 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,armv7s-apple-ios,i386-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" ``` The only Apple targets that aren't tested by the above command are: - `arm64e-apple-darwin`, failed to build, see rust-lang/cc-rs#1205. - `armv7k-apple-watchos`, failed to link, see rust-lang#130071. - `arm64e-apple-ios`, failed to link, see rust-lang#130085. - `i686-apple-darwin`, requires a bit of setup and an older machine, see [the docs](https://doc.rust-lang.org/nightly/rustc/platform-support/i686-apple-darwin.html). - `i386-apple-ios` requires you to set `IPHONEOS_DEPLOYMENT_TARGET=10.0` for the test helpers to work, will be fixed by rust-lang/cc-rs#1030. But all of this is as it was before this PR. Fixes rust-lang#47825, since we now have a test that compiles a `dylib` for `aarch64-apple-ios`. Split out from rust-lang#129342, see that for a little bit of the review that this has gone through already. r? petrochenkov ```@rustbot``` label O-apple

When linking macOS targets with cc, pass the `-mmacosx-version-min=.` option to specify the desired deployment target. Also, no longer pass `-m32`/`-m64`, these are redundant since we already pass `-arch`. When linking with cc on other Apple targets, always pass `-target`. (We assume for these targets that cc => clang).

rustbot · 2024-09-09T11:58:53Z

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

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

madsmtm · 2024-09-09T20:04:36Z

I remembered today that this is similar to #90499, which previously caused issues in #91372, and had to be reverted in #91870, don't know why I didn't think of that sooner as I'm literally the one that was having issues back then...

I think this might be mitigated by:

The issue being a linker bug, and only trigger-able in very specific situations where the user configured their linking incorrectly
The previously affected crates being fixed a long time ago
Our minimum and default deployment target being 10.12 now instead of 10.7
And the deployment target being 11.0 on Aarch64, which is more common these days

Note that this does not affect linking where the linker can figure out the path to the symbol. So normal cases like linking to a symbol that is only available on newer OS versions, for example MPSGetImageType via. the framework MetalPerformanceShaders, are not affected, and will still link fine, even when specifying a low deployment target like we do here.

Test codegen when setting deployment target Test our codegen in different scenarios when setting the deployment target. There are many places here where this is still incorrect, these will be fixed in rust-lang/rust#129342, rust-lang/rust#129367 and rust-lang/rust#129369. See rust-lang/rust#129432 for the bigger picture. Tested locally 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,armv7s-apple-ios,i386-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" ``` The only Apple targets that aren't tested by the above command are: - `arm64e-apple-darwin`, failed to build, see rust-lang/cc-rs#1205. - `armv7k-apple-watchos`, failed to link, see rust-lang/rust#130071. - `arm64e-apple-ios`, failed to link, see rust-lang/rust#130085. - `i686-apple-darwin`, requires a bit of setup and an older machine, see [the docs](https://doc.rust-lang.org/nightly/rustc/platform-support/i686-apple-darwin.html). - `i386-apple-ios` requires you to set `IPHONEOS_DEPLOYMENT_TARGET=10.0` for the test helpers to work, will be fixed by rust-lang/cc-rs#1030. But all of this is as it was before this PR. Fixes rust-lang/rust#47825, since we now have a test that compiles a `dylib` for `aarch64-apple-ios`. Split out from rust-lang/rust#129342, see that for a little bit of the review that this has gone through already. r? petrochenkov ```@rustbot``` label O-apple

jieyouxu

Thanks, the changes seem reasonable. For this change, I'm going to mark this for relnotes to be safe in case something breaks (as this is affects Tier 1 apple targets like x86_64-apple-darwin, even though I think this is technically a bug fix).

jieyouxu · 2024-09-12T02:03:44Z

@bors r+ rollup

bors · 2024-09-12T02:03:47Z

📌 Commit dd35398 has been approved by jieyouxu

It is now in the queue for this repository.

workingjubilee · 2024-09-12T03:36:08Z

per author request to make it easier to bisect:
@bors rollup=iffy

bors · 2024-09-12T06:57:44Z

⌛ Testing commit dd35398 with merge 7c7372b...

bors · 2024-09-12T09:25:06Z

☀️ Test successful - checks-actions
Approved by: jieyouxu
Pushing 7c7372b to master...

rust-timer · 2024-09-12T11:15:42Z

Finished benchmarking commit (7c7372b): 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)

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

Cycles

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

Binary size

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

Bootstrap: 757.698s -> 758.873s (0.16%)
Artifact size: 341.34 MiB -> 341.30 MiB (-0.01%)

rustbot assigned thomcc Aug 21, 2024

madsmtm mentioned this pull request Aug 21, 2024

Improve Xcode support madsmtm/objc2#459

Open

madsmtm mentioned this pull request Aug 22, 2024

Apple deployment target, SDK root and SDK version #129432

Open

madsmtm force-pushed the apple-cc-linker-pass-target branch from e41a543 to 9ac2480 Compare September 7, 2024 09:51

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 7, 2024

This comment has been minimized.

Sign in to view

madsmtm force-pushed the apple-cc-linker-pass-target branch from 9ac2480 to 7727c08 Compare September 7, 2024 10:00

madsmtm mentioned this pull request Sep 7, 2024

Test codegen when setting deployment target #130068

Merged

madsmtm force-pushed the apple-cc-linker-pass-target branch from 7727c08 to b799298 Compare September 7, 2024 13:09

madsmtm mentioned this pull request Sep 7, 2024

Apple arm64e targets fail to link with newer Xcode 15 #130085

Open

BlackHoleFox approved these changes Sep 8, 2024

View reviewed changes

madsmtm force-pushed the apple-cc-linker-pass-target branch from b799298 to dd35398 Compare September 9, 2024 11:57

madsmtm marked this pull request as ready for review September 9, 2024 11:58

rustbot assigned jieyouxu and unassigned thomcc Sep 9, 2024

madsmtm mentioned this pull request Sep 9, 2024

Umbrella framework handling madsmtm/objc2#640

Open

jieyouxu approved these changes Sep 12, 2024

View reviewed changes

jieyouxu added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 12, 2024

rustbot mentioned this pull request Sep 12, 2024

Tracking issue for release notes of #129369: Pass deployment target when linking with CC on Apple targets #130255

Open

3 tasks

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 12, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 12, 2024

bors merged commit 7c7372b into rust-lang:master Sep 12, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Sep 12, 2024

madsmtm deleted the apple-cc-linker-pass-target branch September 12, 2024 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass deployment target when linking with CC on Apple targets #129369

Pass deployment target when linking with CC on Apple targets #129369

madsmtm commented Aug 21, 2024 •

edited

Loading

madsmtm commented Aug 22, 2024

bors commented Aug 23, 2024

This comment has been minimized.

BlackHoleFox left a comment •

edited

Loading

rustbot commented Sep 9, 2024

madsmtm commented Sep 9, 2024 •

edited

Loading

jieyouxu left a comment •

edited

Loading

jieyouxu commented Sep 12, 2024

bors commented Sep 12, 2024

workingjubilee commented Sep 12, 2024

bors commented Sep 12, 2024

bors commented Sep 12, 2024

rust-timer commented Sep 12, 2024

Pass deployment target when linking with CC on Apple targets #129369

Pass deployment target when linking with CC on Apple targets #129369

Conversation

madsmtm commented Aug 21, 2024 • edited Loading

madsmtm commented Aug 22, 2024

bors commented Aug 23, 2024

This comment has been minimized.

BlackHoleFox left a comment • edited Loading

Choose a reason for hiding this comment

rustbot commented Sep 9, 2024

madsmtm commented Sep 9, 2024 • edited Loading

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu commented Sep 12, 2024

bors commented Sep 12, 2024

workingjubilee commented Sep 12, 2024

bors commented Sep 12, 2024

bors commented Sep 12, 2024

rust-timer commented Sep 12, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

madsmtm commented Aug 21, 2024 •

edited

Loading

BlackHoleFox left a comment •

edited

Loading

madsmtm commented Sep 9, 2024 •

edited

Loading

jieyouxu left a comment •

edited

Loading