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

Fix linking Mac Catalyst by including LC_BUILD_VERSION in object files #111384

Merged
merged 2 commits into from
May 26, 2023

Conversation

bmisiak
Copy link
Contributor

@bmisiak bmisiak commented May 9, 2023

Hello.

Issue #106021 prevents Rust code from being linked into Mac Catalyst applications. Apple's LD has started requiring object files to contain version information about the platform they were built for, such as:

  • the "deployment target" (minimum supported OS version),
  • the SDK version
  • the type of the platform (macOS/iOS/catalyst/tvOS/watchOS all have a different number).

This is currently only enforced when building for Mac Catalyst.

Rust uses the object crate which added support for including this information starting with 0.31.0. I upgraded it along with thorin-dwp so that everything depends on 0.31.
Apparently 0.31 pulls in ruzstd due to a new ELF standard because its compression feature is enabled by thorin. If you find this objectionable, let me know what the best way to avoid pulling in those dependencies might be.

(object upgraded in #111413)

I then added two commits:

  • The first one adds very basic, hard-coded support for calling set_macho_build_version for -macabi (Catalyst) targets, where it claims deployment target of Catalyst 14.0 and SDK of 16.2.
  • The second weaves the versioning through rust_target::spec::TargetOptions, so that we can stick to specifying all target-related info in one place.

Kudos to @ara4n for writing this gist.

@rustbot
Copy link
Collaborator

rustbot commented May 9, 2023

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

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 9, 2023
@rustbot
Copy link
Collaborator

rustbot commented May 9, 2023

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

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@bmisiak bmisiak force-pushed the issue-106021-fix branch from 37e8370 to 9012e50 Compare May 10, 2023 05:44
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

It would be best to also bump object in rustc_codegen_llvm as otherwise rustc will be using two different versions of the object crate during compilation which could have unfortunate effects.

It is not necessary to change std's dependencies here because std needs backtrace to be updated first.

I have opened #111413 which does this but otherwise has some conflicting changes with your PR, so I'm quite happy to drop mine.

@petrochenkov
Copy link
Contributor

Blocked on #111413.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2023
@bmisiak
Copy link
Contributor Author

bmisiak commented May 10, 2023

@workingjubilee I did bump it here as well, happy to proceed either way. 👌

@rust-log-analyzer

This comment has been minimized.

@bmisiak bmisiak force-pushed the issue-106021-fix branch from a5c399d to df5cd56 Compare May 10, 2023 15:22
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

typo

compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/metadata.rs Outdated Show resolved Hide resolved
@ara4n
Copy link

ara4n commented May 12, 2023

(thanks for picking up my gist @bmisiak!)

@bors

This comment was marked as resolved.

@bmisiak bmisiak force-pushed the issue-106021-fix branch 2 times, most recently from 06b345f to 9e89441 Compare May 20, 2023 17:32
@bmisiak
Copy link
Contributor Author

bmisiak commented May 20, 2023

Rebased on top of #111413 and removed the dependency changes except for the bump of ar_archive_writer, which already resolved to 0.1.4.

Two commits remain: the first one is a more basic support of injecting BUILD_VERSION for -macabi targets, the second one adds more complex changes to the Catalyst targets so that versioning can be specified in them.

@rustbot label -S-blocked +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 20, 2023
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+ rollup

@bors
Copy link
Contributor

bors commented May 25, 2023

📌 Commit d816b8b 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 25, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 25, 2023
…henkov

Fix linking Mac Catalyst by including LC_BUILD_VERSION in object files

Hello. My first rustc PR!

Issue rust-lang#106021 prevents Rust code from being linked into Mac Catalyst applications. Apple's LD has started requiring object files to contain version information about the platform they were built for, such as:
* the "deployment target" (minimum supported OS version),
* the SDK version
* the type of the platform (macOS/iOS/catalyst/tvOS/watchOS all have a different number).

This is currently only enforced when building for Mac Catalyst.

Rust uses the `object` crate which added support for including this information starting with `0.31.0`. ~~I upgraded it along with `thorin-dwp` so that everything depends on 0.31.
Apparently 0.31 [pulls in](gimli-rs/object#463) `ruzstd` due to a [new ELF standard](https://maskray.me/blog/2022-09-09-zstd-compressed-debug-sections) because its `compression` feature is enabled by thorin. If you find this objectionable, let me know what the best way to avoid pulling in those dependencies might be.~~

**(`object` upgraded in rust-lang#111413

I then added two commits:
* The first one adds very basic, hard-coded support for calling `set_macho_build_version` for `-macabi` (Catalyst) targets, where it claims deployment target of Catalyst 14.0 and SDK of 16.2.
* The second weaves the versioning through `rust_target::spec::TargetOptions`, so that we can stick to specifying all target-related info in one place.

Kudos to `@ara4n` for writing [this gist](https://gist.github.com/ara4n/320a53ea768aba51afad4c9ed2168536).
@bmisiak
Copy link
Contributor Author

bmisiak commented May 25, 2023

@petrochenkov Hmmm I'm finding that my std fails to link for Catalyst when llvm_target does not specify ios14.0 instead of just ios. Not quite sure why yet.

It chokes on addr2line:

Building stage1 library artifacts (aarch64-apple-darwin -> aarch64-apple-ios-macabi)
   Compiling addr2line v0.19.0
error: linking with `clang` failed: exit status: 1
 note: ld: building for Mac Catalyst, but linking in object file built for , file '/Users/bm/code/compiler-rust/build/aarch64-apple-darwin/stage1-std/aarch64-apple-ios-macabi/release/deps/std-b3e24de59aa9f617.17s2hqx9bqxs3ar4.rcgu.o' for architecture arm64
          clang: error: linker command failed with exit code 1 (use -v to see invocation)
error: could not compile `std` (lib) due to previous error

But works fine when llvm_target is arm64-apple-ios14.0-macabi.

#106925 removed the version from llvm_target because newer clang dropped support for the 13.0 catalyst target.

I might add it back in, bumping it from 13.0 to 14.0. We do specify the target when building for iOS as well.

@bmisiak
Copy link
Contributor Author

bmisiak commented May 25, 2023

@rustbot label -S-waiting-on-bors +S-waiting-on-review

@rustbot rustbot 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 May 25, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 26, 2023

📌 Commit a61f026 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 May 26, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request May 26, 2023
…henkov

Fix linking Mac Catalyst by including LC_BUILD_VERSION in object files

Hello. My first rustc PR!

Issue rust-lang#106021 prevents Rust code from being linked into Mac Catalyst applications. Apple's LD has started requiring object files to contain version information about the platform they were built for, such as:
* the "deployment target" (minimum supported OS version),
* the SDK version
* the type of the platform (macOS/iOS/catalyst/tvOS/watchOS all have a different number).

This is currently only enforced when building for Mac Catalyst.

Rust uses the `object` crate which added support for including this information starting with `0.31.0`. ~~I upgraded it along with `thorin-dwp` so that everything depends on 0.31.
Apparently 0.31 [pulls in](gimli-rs/object#463) `ruzstd` due to a [new ELF standard](https://maskray.me/blog/2022-09-09-zstd-compressed-debug-sections) because its `compression` feature is enabled by thorin. If you find this objectionable, let me know what the best way to avoid pulling in those dependencies might be.~~

**(`object` upgraded in rust-lang#111413

I then added two commits:
* The first one adds very basic, hard-coded support for calling `set_macho_build_version` for `-macabi` (Catalyst) targets, where it claims deployment target of Catalyst 14.0 and SDK of 16.2.
* The second weaves the versioning through `rust_target::spec::TargetOptions`, so that we can stick to specifying all target-related info in one place.

Kudos to `@ara4n` for writing [this gist](https://gist.github.com/ara4n/320a53ea768aba51afad4c9ed2168536).
bors added a commit to rust-lang-ci/rust that referenced this pull request May 26, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#111384 (Fix linking Mac Catalyst by including LC_BUILD_VERSION in object files)
 - rust-lang#111899 (CGU cleanups)
 - rust-lang#111940 (Clarify safety concern of `io::Read::read` is only relevant in unsafe code)
 - rust-lang#111947 (Add test for RPIT defined with different hidden types with different substs)
 - rust-lang#111951 (Correct comment on privately uninhabited pattern.)

Failed merges:

 - rust-lang#111954 (improve error message for calling a method on a raw pointer with an unknown pointee)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 42c7b8a into rust-lang:master May 26, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 26, 2023
@bmisiak bmisiak deleted the issue-106021-fix branch May 30, 2023 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.

9 participants