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

Implement RFC 2951: Native link modifiers #83507

Merged
merged 1 commit into from
May 6, 2021

Conversation

luqmana
Copy link
Member

@luqmana luqmana commented Mar 26, 2021

A first attempt at implementing rust-lang/rfcs#2951 / rust-lang/compiler-team#356.

Tracking Issue: #81490

Introduces feature flags for the general syntax (native_link_modifiers) and each modifier (native_link_modifiers_{as_needed,bundle,verbatim,whole_archive}).

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@tschuett
Copy link

Do you want to file a bug for the features that you need on OSX with LLD:
https://bugs.llvm.org/buglist.cgi?component=MachO&list_id=207355&product=lld&resolution=---

compiler/rustc_ast_passes/src/feature_gate.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/config.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/config.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/config.rs Show resolved Hide resolved
compiler/rustc_metadata/src/native_libs.rs Outdated Show resolved Hide resolved
compiler/rustc_metadata/src/native_libs.rs Show resolved Hide resolved
// Even though we pass `whole-archive` (or similar) to the linker here,
// gc-sections (or similar) may end up stripping away any unreferenced
// symbols. So let's try explicitly disabling that.
cmd.no_gc_sections();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that this is necessary and linker doesn't disable --gc-sections automatically when it sees --whole-archive? If it's necessary, then that's unfortunate.

Anyway, if it's necessary and --gc-sections is disabled before link_whole_staticlib, then it should be re-enabled after it.
Or not, because --gc-sections is not always enabled by default. (Pehaps --push-state/--pop-state can help here, although https://sourceware.org/binutils/docs/ld/Options.html doesn't list --gc-sections among the saved/restored flags.)
What is worse, /OPT:NOREF,NOICF on MSVC is global and applies to all libraries, not to this specific one. (But again, maybe it's not even necessary?)

Copy link
Contributor

@petrochenkov petrochenkov Mar 27, 2021

Choose a reason for hiding this comment

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

From reading LLD sources it looks like --gc-sections is a global flag affecting all files regardless of order in ld-like linkers as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, I did some quick testing and it seemed like --whole-archive by itself isn't enough unfortunately. And like you said, it's a global flag. :/

compiler/rustc_codegen_ssa/src/back/linker.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/linker.rs Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

  1. There is some overlap between the bundle and whole-archive modifiers as it is currently. rustc already passed --whole-archive for NativeLibKind::StaticBundle.

There are two cases here.

  • Either the static-bundle attribute/flag is specified when building a rlib (or staticlib?), in that case it's not linking and we don't know which symbols are used and which not, so we have to bundle everything. Later during linking we are linking the rlib (which may contain multiple native libs in addition to its own code) as a single unit and cannot specify any flags for its contents with better granularity. So everything is linked as --whole-archive (including the original native libraries) just because rlibs are always linked as --whole-archive.
  • Alternatively, the static-bundle attribute/flag is specified when building a final artifact, in that case linking happens and we can throw away unnecessary parts of the native lib. I don't know why rustc enables --whole-archive in this case. Maybe for consistency with the case above?

Anyway, I think we should keep the status quo behavior for now, i.e. bundle implies whole-archive unless whole-archive was disabled explicitly with modifiers.

  1. ld64 doesn't have --[no-]as-needed but as of macOS 11 has similar -needed_{library,framework} flags but unsure how we can detect that at the point where we'd want to pass those flags.

Unfortunately, I don't know anything about Apple targets.

@petrochenkov
Copy link
Contributor

Opening as a draft to get some feedback on the approach.

I don't really see any alternative ways to implement this compared to what this PR does.

@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 Mar 27, 2021
@bors

This comment has been minimized.

@petrochenkov
Copy link
Contributor

@luqmana
Any plans to continue this work?
I'm interested in landing the modifier infra sooner than specific modifiers to make some experiments with #73319.
If you don't have time then I can rebase/update it myself during the upcoming week.

@luqmana luqmana force-pushed the native-link-modifiers branch 2 times, most recently from 06c4359 to 1d559ec Compare May 2, 2021 04:54
@luqmana
Copy link
Member Author

luqmana commented May 2, 2021

@petrochenkov I rebased and addressed most of the review comments. Feel free to pick out any parts you needed.

@petrochenkov petrochenkov 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 2, 2021
@petrochenkov
Copy link
Contributor

I thinks this PR as a whole is pretty much ready for landing.

Remaining issues:

  • I'd remove all the no_gc_sections additions for now. The modifier tells whole-archive - we add --whole-archive, end of the story. Other flags passed by the compiler may cause issues, but they are orthogonal issues that may be addressed separately.
  • In absence of explicitly written bundle or whole-archive modifiers the behavior needs to preserve the current status quo for now (--whole-archive for NativeLibKind::StaticBundle etc).
  • For the previous item and in general it would be useful to keep modifiers as Option<bool> instead of bool to preserve all three states - "no modifier", "+modifier" and "-modifier".

@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 3, 2021
@luqmana luqmana marked this pull request as ready for review May 4, 2021 00:21
@rust-log-analyzer

This comment has been minimized.

@luqmana luqmana changed the title [WIP] Implement RFC 2951: Native link modifiers Implement RFC 2951: Native link modifiers May 4, 2021
@petrochenkov petrochenkov 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 4, 2021
@petrochenkov
Copy link
Contributor

r=me, but it would be nice to cleanup the git history a bit to avoid back and forth changes (or just squash everything).

@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 5, 2021
This commit implements both the native linking modifiers infrastructure
as well as an initial attempt at the individual modifiers from the RFC.
It also introduces a feature flag for the general syntax along with
individual feature flags for each modifier.
@luqmana luqmana 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 6, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 6, 2021

📌 Commit db555e1 has been approved by petrochenkov

@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 6, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2021
Rollup of 8 pull requests

Successful merges:

 - rust-lang#83507 (Implement RFC 2951: Native link modifiers)
 - rust-lang#84328 (Stablize {HashMap,BTreeMap}::into_{keys,values})
 - rust-lang#84712 (Simplify chdir implementation and minimize unsafe block)
 - rust-lang#84851 (:arrow_up: rust-analyzer)
 - rust-lang#84923 (Only compute Obligation `cache_key` once  in `register_obligation_at`)
 - rust-lang#84945 (E0583: Include secondary path in error message)
 - rust-lang#84949 (Fix typo in `MaybeUninit::array_assume_init` safety comment)
 - rust-lang#84950 (Revert PR 83866)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5dcdeb8 into rust-lang:master May 6, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 6, 2021
@luqmana luqmana deleted the native-link-modifiers branch May 6, 2021 22:23
bors added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2021
remove native_link_modifiers from the list of incomplete features.

These features are fully implemented and not incomplete.
The tracking issue of them is rust-lang#81490.
The implement PR is rust-lang#83507.
@krasimirgg
Copy link
Contributor

Is the Relative order of -l and -Clink-arg(s) options part of the RFC implemented?
We've seen a curious case where, under #![feature(native_link_modifiers)], a rustc ... -Clink-arg=foo ... -lstatic=bar ... invocation produces a... -lbar ... foo ... linker invocation, where the relative order is not preserved.

@petrochenkov
Copy link
Contributor

@krasimirgg

Is the Relative order of -l and -Clink-arg(s) options part of the RFC implemented?

No, it's not yet implemented.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants