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

Add --no-undefined-version link flag and fix associated breakage #108017

Merged
merged 2 commits into from
Mar 11, 2023

Conversation

chbaker0
Copy link
Contributor

LLVM upstream sets --no-undefined-version by default in lld: https://reviews.llvm.org/D135402.

Due to a bug in how version scripts are generated, this breaks the dylib output type for most crates. See #105967 (comment) for details.

This PR adds the flag to gcc flavor linkers in anticipation of this LLVM change rolling in, and patches rustc to not attempt to export __rust_* allocator symbols when they weren't generated.

Fixes #105967

@rustbot
Copy link
Collaborator

rustbot commented Feb 13, 2023

r? @oli-obk

(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 Feb 13, 2023
@chbaker0
Copy link
Contributor Author

r? @bjorn3 since we discussed this issue already

@rustbot rustbot assigned bjorn3 and unassigned oli-obk Feb 13, 2023
@chbaker0 chbaker0 marked this pull request as ready for review February 13, 2023 23:55
/// `allocator_kind`, but it may return `None` in more cases (e.g. if using
/// allocator definitions from a dylib dependency).
query codegen_allocator_kind(_: ()) -> Option<AllocatorKind> {
eval_always
Copy link
Member

Choose a reason for hiding this comment

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

This eval_always can be removed as the query doesn't depend on untracked state but only on other queries (dependency_formats and allocator_kind). I'm not sure whether this makes sense to have as a query in the first place or whether it should just be a normal helper function. I think a normal function would make more sense as the output of this will usually change when the inputs (dependency_formats and allocator_kind) change and it's also pretty cheap to compute and only called twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: eval_always: got it, I'm still getting to understand the query system.

A normal function would be fine, but there's some benefit of it being next to allocator_kind too to potentially avoid similar mistakes in the future. Up to you though.

Any suggestion for where's a good place to put the helper function?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe compiler/rustc_codegen_ssa/src/base.rs? It doesn't quite fit, but I don't know a better place.

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, I moved the logic to a function there

Copy link
Contributor Author

@chbaker0 chbaker0 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, I replied but didn't submit it

/// `allocator_kind`, but it may return `None` in more cases (e.g. if using
/// allocator definitions from a dylib dependency).
query codegen_allocator_kind(_: ()) -> Option<AllocatorKind> {
eval_always
Copy link
Contributor Author

Choose a reason for hiding this comment

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

re: eval_always: got it, I'm still getting to understand the query system.

A normal function would be fine, but there's some benefit of it being next to allocator_kind too to potentially avoid similar mistakes in the future. Up to you though.

Any suggestion for where's a good place to put the helper function?

@nikic nikic mentioned this pull request Feb 17, 2023
@chbaker0
Copy link
Contributor Author

Hi, just checking in again

@chbaker0 chbaker0 force-pushed the fix-105967 branch 2 times, most recently from b732615 to 332afdb Compare February 24, 2023 21:18
@chbaker0
Copy link
Contributor Author

Ready for another review

Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I got a minor request to avoid some code duplication, but r=me either way.

compiler/rustc_codegen_ssa/src/base.rs Show resolved Hide resolved
@tmandry
Copy link
Member

tmandry commented Feb 27, 2023

@rustbot author

@rustbot rustbot 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 Feb 27, 2023
@tmandry
Copy link
Member

tmandry commented Feb 27, 2023

@bors delegate+

@bors
Copy link
Contributor

bors commented Feb 27, 2023

✌️ @chbaker0 can now approve this pull request

@chbaker0 chbaker0 force-pushed the fix-105967 branch 2 times, most recently from ac58c9c to 33e8820 Compare March 6, 2023 22:21
@rustbot
Copy link
Collaborator

rustbot commented Mar 6, 2023

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@chbaker0
Copy link
Contributor Author

chbaker0 commented Mar 6, 2023

Thanks!

@bors r+

extra changes are exactly as requested

@bors
Copy link
Contributor

bors commented Mar 6, 2023

📌 Commit 33e8820 has been approved by chbaker0

It is now in the queue for this repository.

@chbaker0
Copy link
Contributor Author

Makes sense; I guess --no-undefined-version only makes sense for a linker that calls this a "version script"

I moved it under the appropriate condition.

Not sure if the delegate earlier is sticky, but I'll try:

@bors r+

@bors
Copy link
Contributor

bors commented Mar 10, 2023

📌 Commit cb41803 has been approved by chbaker0

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 10, 2023

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@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 Mar 10, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2023
Add `--no-undefined-version` link flag and fix associated breakage

LLVM upstream sets `--no-undefined-version` by default in lld: https://reviews.llvm.org/D135402.

Due to a bug in how version scripts are generated, this breaks the `dylib` output type for most crates. See rust-lang#105967 (comment) for details.

This PR adds the flag to gcc flavor linkers in anticipation of this LLVM change rolling in, and patches `rustc` to not attempt to export `__rust_*` allocator symbols when they weren't generated.

Fixes rust-lang#105967
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2023
Add `--no-undefined-version` link flag and fix associated breakage

LLVM upstream sets `--no-undefined-version` by default in lld: https://reviews.llvm.org/D135402.

Due to a bug in how version scripts are generated, this breaks the `dylib` output type for most crates. See rust-lang#105967 (comment) for details.

This PR adds the flag to gcc flavor linkers in anticipation of this LLVM change rolling in, and patches `rustc` to not attempt to export `__rust_*` allocator symbols when they weren't generated.

Fixes rust-lang#105967
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#105798 (Relax ordering rules for `asm!` operands)
 - rust-lang#105962 (Stabilize path_as_mut_os_str)
 - rust-lang#106085 (use problem matchers for tidy CI)
 - rust-lang#107711 (Stabilize movbe target feature)
 - rust-lang#108017 (Add `--no-undefined-version` link flag and fix associated breakage)
 - rust-lang#108891 (Remove an extraneous include)
 - rust-lang#108902 (no more do while :<)
 - rust-lang#108912 (Document tool lints)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 342ca46 into rust-lang:master Mar 11, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 11, 2023
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Mar 15, 2023
Add `--no-undefined-version` link flag and fix associated breakage

LLVM upstream sets `--no-undefined-version` by default in lld: https://reviews.llvm.org/D135402.

Due to a bug in how version scripts are generated, this breaks the `dylib` output type for most crates. See rust-lang#105967 (comment) for details.

This PR adds the flag to gcc flavor linkers in anticipation of this LLVM change rolling in, and patches `rustc` to not attempt to export `__rust_*` allocator symbols when they weren't generated.

Fixes rust-lang#105967
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Mar 15, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#105798 (Relax ordering rules for `asm!` operands)
 - rust-lang#105962 (Stabilize path_as_mut_os_str)
 - rust-lang#106085 (use problem matchers for tidy CI)
 - rust-lang#107711 (Stabilize movbe target feature)
 - rust-lang#108017 (Add `--no-undefined-version` link flag and fix associated breakage)
 - rust-lang#108891 (Remove an extraneous include)
 - rust-lang#108902 (no more do while :<)
 - rust-lang#108912 (Document tool lints)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2023
…proc-macros-issue-99978, r=bjorn3

Restrict linker version script of proc-macro crates to just its two symbols

Restrict linker version script of proc-macro crates to just the two symbols of each proc-macro crate.

The main known effect of doing this is to stop including `#[no_mangle]` symbols in the linker version script.

Background:

The combination of a proc-macro crate with an import of another crate that itself exports a no_mangle function was broken for a period of time, because:

* In PR rust-lang#99944 we stopped exporting no_mangle symbols from proc-macro crates; proc-macro crates have a very limited interface and are meant to be treated as a blackbox to everything except rustc itself. However: he constructed linker version script still referred to them, but resolving that discrepancy was left as a FIXME in the code, tagged with issue rust-lang#99978.
* In PR rust-lang#108017 we started telling the linker to check (via the`--no-undefined-version` linker invocation flag) that every symbol referenced in the "linker version script" is provided as linker input. So the unresolved discrepancy from rust-lang#99978 started surfacing as a compile-time error (e.g. rust-lang#111888).

Fix rust-lang#111888
Fix rust-lang#99978.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

libtest fails to link with lld at main
8 participants