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

Always add LC_BUILD_VERSION for metadata object files #114114

Conversation

keith
Copy link
Contributor

@keith keith commented Jul 27, 2023

As of Xcode 15 Apple's linker has become a bit more strict about the warnings it produces. One of those new warnings requires all valid Mach-O object files in an archive to have a LC_BUILD_VERSION load command:

ld: warning: no platform load command found in 'ARCHIVE[arm64][2106](lib.rmeta)', assuming: iOS-simulator

This was already being done for Mac Catalyst so this change expands this logic to include it for all Apple platforms. I filed this behavior change as FB12546320 and was told it was the new intentional behavior.

@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2023

r? @cjgillot

(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 Jul 27, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2023

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

@cjgillot
Copy link
Contributor

I won't have time to review this before a week.
r? compiler

@rustbot rustbot assigned wesleywiser and unassigned cjgillot Jul 28, 2023
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.

Changes seem fine to me but I don't have much knowledge of Apple platforms. @thomcc I think you might have more context here. Does this seem ok to you?

Copy link
Contributor

@BlackHoleFox BlackHoleFox left a comment

Choose a reason for hiding this comment

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

I am not the reviewer here, or @thomcc, but I do have some questions/suggestions as someone who has worked in these files a lot.

@@ -274,6 +320,11 @@ fn ios_deployment_target() -> (u32, u32) {
from_set_deployment_target("IPHONEOS_DEPLOYMENT_TARGET").unwrap_or((7, 0))
}

fn mac_catalyst_deployment_target() -> (u32, u32) {
// If you are looking for the default deployment target, prefer `rustc --print deployment-target`.
from_set_deployment_target("IPHONEOS_DEPLOYMENT_TARGET").unwrap_or((14, 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did 14.0 come from here? Was it just because that's the current assumed target in the other spec files?

IMO (not the reviewer here) its fine to do this but in some followup the hardcoded versions in the current catalyst target specs should get nuked. This would match what users would expect now that were reading this variable. Definitely doesn't need to be done here though I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled it from the original implementation of this behavior that only applied to catalyst https://github.com/rust-lang/rust/pull/114114/files#diff-6bbf6edd930f26d3b180eb5c250b0e48d8f3c5eb474e3274909ef6ae6f0d1e61L343

I agree having it configurable makes sense, at least in this case if you set IPHONEOS_DEPLOYMENT_TARGET it would win though

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed the lines removed in the diff, oops. That answers the immediate question then.

I agree having it configurable makes sense, at least in this case if you set IPHONEOS_DEPLOYMENT_TARGET it would win though

Do you want to take that to a followup PR or shall I? Fixing the mess that's Catalyst has been on my list forever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can send that one after!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah it looks like using the triple for this case 78bbe57#diff-f2aa7189f140501f7d0d283ef503e1980a9449ef0292b48b650f6e0c87bb8d6b complicates things slightly

Copy link
Contributor

Choose a reason for hiding this comment

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

iiuc it should just be making another of the <target_here>_lld_platform_version functions to use for this and other LLVM needs and have that source from the deployment target function. Might have missed a complication with the catalyst target though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sounds good I can try it after this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like to make this work I would need to change add_pre_link_args to take a String or something else since ours would be dynamically computed here, would that be a reasonable change?

compiler/rustc_target/src/spec/apple_base.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/apple_base.rs Show resolved Hide resolved
@thomcc
Copy link
Member

thomcc commented Aug 21, 2023

@thomcc I think you might have more context here. Does this seem ok to you?

I missed this ping until now, but my opinion would be to defer to @BlackHoleFox's review, which seems quite thorough and accurate.

As of Xcode 15 Apple's linker has become a bit more strict about the
warnings it produces. One of those new warnings requires all valid
Mach-O object files in an archive to have a LC_BUILD_VERSION load
command:

```
ld: warning: no platform load command found in 'ARCHIVE[arm64][2106](lib.rmeta)', assuming: iOS-simulator
```

This was already being done for Mac Catalyst so this change expands this
logic to include it for all Apple platforms. I filed this behavior
change as FB12546320 and was told it was the new intentional behavior.
@keith keith force-pushed the ks/always-add-lc_build_version-for-metadata-object-files branch from 8dceddb to d37fdc9 Compare August 21, 2023 20:32
Copy link
Contributor

@BlackHoleFox BlackHoleFox left a comment

Choose a reason for hiding this comment

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

@wesleywiser this looks good to r+ from me now.

@wesleywiser
Copy link
Member

@bors r+ rollup=never

(in case we need to bisect to this PR later)

@bors
Copy link
Contributor

bors commented Aug 29, 2023

📌 Commit 2939e85 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 Aug 29, 2023
@bors
Copy link
Contributor

bors commented Aug 29, 2023

⌛ Testing commit 2939e85 with merge 84a9f4c...

@bors
Copy link
Contributor

bors commented Aug 29, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 84a9f4c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 29, 2023
@bors bors merged commit 84a9f4c into rust-lang:master Aug 29, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Aug 29, 2023
@keith
Copy link
Contributor Author

keith commented Aug 29, 2023

thanks all!

@keith keith deleted the ks/always-add-lc_build_version-for-metadata-object-files branch August 29, 2023 23:34
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (84a9f4c): 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)
2.2% [2.2%, 2.2%] 1
Improvements ✅
(primary)
-0.6% [-0.7%, -0.5%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.7%, -0.5%] 2

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: 630.72s -> 630.625s (-0.02%)
Artifact size: 316.01 MiB -> 315.99 MiB (-0.00%)

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.

None yet

8 participants