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

linker: Report linker flavors incompatible with the current target #110807

Merged
merged 4 commits into from
Jun 1, 2023

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Apr 25, 2023

The linker flavor is checked for target compatibility even if linker is never used (e.g. we are producing a rlib).
If it causes trouble, we can move the check to link.rs so it will run if the linker (flavor) is actually used.

And also feature gate explicitly specifying linker flavors for tier 3 targets.

The next step is supporting all the internal linker flavors in user-visible interfaces (command line and json).

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2023

r? @b-naber

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

rustbot commented Apr 25, 2023

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

@b-naber
Copy link
Contributor

b-naber commented Apr 28, 2023

r? rust-lang/compiler

@rustbot rustbot assigned wesleywiser and unassigned b-naber Apr 28, 2023
@bors

This comment was marked as resolved.

@petrochenkov
Copy link
Contributor Author

cc @lqd

@lqd
Copy link
Member

lqd commented May 12, 2023

The linker flavor is checked for target compatibility even if linker is never used (e.g. we are producing a rlib)

That seems fine under "common usage" and targets, right (even in cross- compilation cases) ?

Maybe we can do a perf run just to exercize the PR with a few real world crates with build scripts and proc-macros, in case rustc-perf has more coverage there than our tests in practice. (I wouldn’t really expect issues to be found per se)

Did we ever consider the bpf/ptx linker flavors stable ?

Otherwise this LGTM.

@petrochenkov
Copy link
Contributor Author

@lqd

That seems fine under "common usage" and targets, right (even in cross- compilation cases) ?

Yes, the only cases of breakage I can imagine are 1) wrong -Clinker-flavor is added accidentally and never used because no linking happens and 2) compile-fail tests in our own test suite.

Maybe we can do a perf run just to exercize the PR with a few real world crates with build scripts and proc-macros, in case rustc-perf has more coverage there than our tests in practice. (I wouldn’t really expect issues to be found per se)

I wouldn't expect any effect on performance, this is a cheap check that is performed once during compilation.

Did we ever consider the bpf/ptx linker flavors stable ?

I guess no?
But technically they could be specified with a stable compiler and non tier-3 targets in cases when no linking actually happens (e.g. producing a rlib).
But I wouldn't expect it to ever happen in practice, these are the only working linker flavors for bpf/ptx targets respectively, you never need to specify them explicitly.

@petrochenkov
Copy link
Contributor Author

This needs a couple of tests, one for the target compat check and another for the feature gates.

@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 May 14, 2023
@petrochenkov petrochenkov force-pushed the strictflavor branch 3 times, most recently from 1dd364e to 4851f0b Compare May 22, 2023 21:04
@petrochenkov
Copy link
Contributor Author

Added tests.
@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 May 22, 2023
@bors

This comment was marked as resolved.

// [bpf] compile-flags: --target=bpfel-unknown-none -C linker-flavor=bpf-linker --crate-type=rlib
// [bpf] error-pattern: linker flavor `bpf-linker` is unstable, `-Z unstable-options` flag
// [bpf] needs-llvm-components:
// [ptx] compile-flags: --target=nvptx64-nvidia-cuda -C linker-flavor=ptx-linker --crate-type=rlib
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it matters but nvptx64-nvidia-cuda is Tier 2, not Tier 3.

@wesleywiser
Copy link
Member

@bors r=lqd,wesleywiser

@bors
Copy link
Contributor

bors commented May 26, 2023

📌 Commit 6675f46389bc931c0d73da75ed709846e2f76c53 has been approved by lqd,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 May 26, 2023
@bors
Copy link
Contributor

bors commented May 26, 2023

⌛ Testing commit 6675f46389bc931c0d73da75ed709846e2f76c53 with merge f74e273755395be9266f36badf49b933af48b1ed...

@bors bors 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 26, 2023
@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 May 27, 2023
Go through an intermediate pair of `cc`and `lld` hints instead of mapping CLI options to `LinkerFlavor` directly, and use the target's default linker flavor as a reference.
Previously they would be reported as link time errors about unknown linker options
@petrochenkov
Copy link
Contributor Author

@bors r=lqd,wesleywiser

@bors
Copy link
Contributor

bors commented May 29, 2023

📌 Commit 2f7328d has been approved by lqd,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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 29, 2023
Noratrieb added a commit to Noratrieb/rust that referenced this pull request May 30, 2023
…esleywiser

linker: Report linker flavors incompatible with the current target

The linker flavor is checked for target compatibility even if linker is never used (e.g. we are producing a rlib).
If it causes trouble, we can move the check to `link.rs` so it will run if the linker (flavor) is actually used.

And also feature gate explicitly specifying linker flavors for tier 3 targets.

The next step is supporting all the internal linker flavors in user-visible interfaces (command line and json).
@Noratrieb
Copy link
Member

looks like this broke dist-x86_64-linux? #112092 (comment)
@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 30, 2023
@Noratrieb
Copy link
Member

Nevermind, it was innocent: #112095 (comment)

@bors r=lqd,wesleywiser

@bors
Copy link
Contributor

bors commented May 30, 2023

📌 Commit 2f7328d has been approved by lqd,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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 30, 2023
@bors
Copy link
Contributor

bors commented May 31, 2023

⌛ Testing commit 2f7328d with merge 9af3865...

@bors
Copy link
Contributor

bors commented Jun 1, 2023

☀️ Test successful - checks-actions
Approved by: lqd,wesleywiser
Pushing 9af3865 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 1, 2023
@bors bors merged commit 9af3865 into rust-lang:master Jun 1, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 1, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9af3865): 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.6% [0.6%, 0.6%] 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)
- - 0
Regressions ❌
(secondary)
2.3% [2.3%, 2.3%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

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: 643.718s -> 644.991s (0.20%)

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.

9 participants