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 legacy bitcode defaults from all Apple specs #117364

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

BlackHoleFox
Copy link
Contributor

Xcode 14 deprecated bitcode with warnings and now Xcode 15 has dropped it completely. rustc should follow what the platform tooling is doing as well since it just increases binary sizes for no gain at this point.

cc made a similar change last month.

Two things show this should have minimal impact:

  • Apple has stopped accepting apps built with versions of Xcode (<14) that generate bitcode
  • The app store has been stripping bitcode off IPA releases for over 2 years now.

I didn't nuke all the bitcode changes added in #71970 since maybe another target in the future could need mandatory bitcode embedding.

Staticlibs built for iOS still link correctly with XCode 15 against a test app when using a compiler built from this branch.

cc @thomcc @keith

@rustbot
Copy link
Collaborator

rustbot commented Oct 29, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@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 Oct 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 29, 2023

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

@BlackHoleFox
Copy link
Contributor Author

One open question: Should the Apple-specific bitcode_section_name logic be removed here too? I don't know if other uses/tools for bitcode outside of App Store distribution need the Apple-specific name or just the generic LLVM bitcode section name.

Copy link
Contributor

@keith keith left a comment

Choose a reason for hiding this comment

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

awesome cleanup!

@keith
Copy link
Contributor

keith commented Oct 29, 2023

One open question: Should the Apple-specific bitcode_section_name logic be removed here too? I don't know if other uses/tools for bitcode outside of App Store distribution need the Apple-specific name or just the generic LLVM bitcode section name.

I don't think that's safe to remove unless the embed_bitcode logic can be removed entirely. If a Mach-O object ever has embedded bitcode I do think it has to be in the apple specific section.

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Nov 8, 2023

It's better to re-roll it r? compiler

@rustbot rustbot assigned davidtwco and unassigned TaKO8Ki Nov 8, 2023
@bors

This comment was marked as resolved.

@BlackHoleFox
Copy link
Contributor Author

Rebased to remove conflicts.

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, if there are more parts that you can remove and that were only used by these targets, then I think you should feel free to do so - they can always be resurrected from the Git history if reqd - but I'll approve and merge this for now and that can be for a follow-up.

@davidtwco
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 19, 2023

📌 Commit b27c3b7 has been approved by davidtwco

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 Nov 19, 2023
@bors
Copy link
Contributor

bors commented Nov 19, 2023

⌛ Testing commit b27c3b7 with merge 7d0e1bc...

@bors
Copy link
Contributor

bors commented Nov 19, 2023

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 7d0e1bc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 19, 2023
@bors bors merged commit 7d0e1bc into rust-lang:master Nov 19, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Nov 19, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7d0e1bc): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

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

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: 675.196s -> 674.69s (-0.07%)
Artifact size: 313.81 MiB -> 313.81 MiB (-0.00%)

@BlackHoleFox BlackHoleFox deleted the farewell-bitcode-no-remorse branch November 19, 2023 23:19
@arttet arttet mentioned this pull request Dec 19, 2023
2 tasks
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Dec 19, 2023
… r=petrochenkov

Fix arm64e-apple-ios target

- [x] [Remove legacy `bitcode` defaults](rust-lang#117364)
- [x] Use LLVM features

Now we have warnings such as

```
'+paca' is not a recognized feature for this target (ignoring feature)
'+pacg' is not a recognized feature for this target (ignoring feature)
```

Because we should use LLVM features.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2023
…=petrochenkov

Fix arm64e-apple-ios target

- [x] [Remove legacy `bitcode` defaults](rust-lang#117364)
- [x] Use LLVM features

Now we have warnings such as

```
'+paca' is not a recognized feature for this target (ignoring feature)
'+pacg' is not a recognized feature for this target (ignoring feature)
```

Because we should use LLVM features.
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. 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.

7 participants