Implement RFC 2951: Native link modifiers by luqmana · Pull Request #83507 · rust-lang/rust

luqmana · 2021-03-26T04:32:10Z

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

tschuett · 2021-03-26T15:01:54Z

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

src/test/ui/feature-gates/feature-gate-static-nobundle.rs

compiler/rustc_session/src/config.rs

compiler/rustc_metadata/src/native_libs.rs

petrochenkov · 2021-03-27T19:06:12Z

compiler/rustc_codegen_ssa/src/back/link.rs

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?)

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.

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

petrochenkov · 2021-03-27T19:31:50Z

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.

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 · 2021-03-27T19:34:23Z

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.

src/test/ui/feature-gates/feature-gate-static-nobundle.rs

petrochenkov · 2021-04-30T22:41:06Z

@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 · 2021-05-02T04:55:23Z

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

petrochenkov · 2021-05-03T16:22:34Z

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 · 2021-05-04T14:17:26Z

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

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.

petrochenkov · 2021-05-06T10:03:14Z

@bors r+

bors · 2021-05-06T10:03:15Z

📌 Commit db555e1 has been approved by petrochenkov

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

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 · 2022-02-09T15:21:56Z

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 · 2022-02-09T16:13:14Z

@krasimirgg

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

No, it's not yet implemented.

rust-highfive assigned petrochenkov Mar 26, 2021

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

This comment has been minimized.

Sign in to view

petrochenkov mentioned this pull request Mar 26, 2021

Using rustc as the linker driver for mixed C++/Rust binaries #83282

Closed

petrochenkov reviewed Mar 27, 2021

View reviewed changes

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

This comment has been minimized.

Sign in to view

sjakobi reviewed Apr 11, 2021

View reviewed changes

src/test/ui/feature-gates/feature-gate-static-nobundle.rs Outdated Show resolved Hide resolved

petrochenkov mentioned this pull request Apr 20, 2021

Partial support for raw-dylib linkage #84171

Merged

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

petrochenkov mentioned this pull request May 2, 2021

Deduplicate native libs before they are passed to the linker #84794

Merged

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 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 marked this pull request as ready for review May 4, 2021 00:21

This comment has been minimized.

Sign in to view

luqmana changed the title ~~[WIP] Implement RFC 2951: Native link modifiers~~ Implement RFC 2951: Native link modifiers May 4, 2021

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

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

luqmana force-pushed the native-link-modifiers branch from 71b5ccb to db555e1 Compare May 5, 2021 23:18

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

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

Dylan-DPC-zz mentioned this pull request May 6, 2021

Rollup of 8 pull requests #84982

Merged

bors merged commit 5dcdeb8 into rust-lang:master May 6, 2021

rustbot added this to the 1.54.0 milestone May 6, 2021

luqmana deleted the native-link-modifiers branch May 6, 2021 22:23

luqmana mentioned this pull request May 6, 2021

Tracking Issue for linking modifiers for native libraries #81490

Closed

3 tasks

This was referenced May 23, 2021

build llvm libunwind.a in rustbuild #85600

Merged

remove native_link_modifiers from the list of incomplete features. #85606

Merged

gcoakes mentioned this pull request Aug 16, 2021

Whole archive modifier for native libraries is not propagated to final output. #88085

Closed

petrochenkov mentioned this pull request Feb 16, 2022

Stabilize native library modifier syntax and the whole-archive modifier specifically #93901

Merged

petrochenkov mentioned this pull request May 8, 2022

Stabilize the bundle native library modifier #95818

Merged

This was referenced Jul 18, 2022

Tracking Issue for native link modifier as-needed #99424

Open

Tracking Issue for native link modifier verbatim #99425

Closed

petrochenkov mentioned this pull request Nov 13, 2022

Stabilize native library modifier verbatim #104360

Merged

Uh oh!

Conversation

luqmana commented Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

tschuett commented Mar 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov Mar 27, 2021

Choose a reason for hiding this comment

Uh oh!

petrochenkov Mar 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luqmana May 2, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Mar 27, 2021

Uh oh!

petrochenkov commented Mar 27, 2021

Uh oh!

This comment has been minimized.

Uh oh!

petrochenkov commented Apr 30, 2021

Uh oh!

luqmana commented May 2, 2021

Uh oh!

petrochenkov commented May 3, 2021

Uh oh!

This comment has been minimized.

petrochenkov commented May 4, 2021

Uh oh!

petrochenkov commented May 6, 2021

Uh oh!

bors commented May 6, 2021

Uh oh!

krasimirgg commented Feb 9, 2022

Uh oh!

petrochenkov commented Feb 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

luqmana commented Mar 26, 2021 •

edited

Loading

petrochenkov Mar 27, 2021 •

edited

Loading