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

Tracking Issue for linking modifiers for native libraries #81490

Closed
3 tasks done
petrochenkov opened this issue Jan 28, 2021 · 33 comments
Closed
3 tasks done

Tracking Issue for linking modifiers for native libraries #81490

petrochenkov opened this issue Jan 28, 2021 · 33 comments
Assignees
Labels
A-linkage Area: linking into static, shared libraries and binaries C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 28, 2021

This is a tracking issue for the RFC "Linking modifiers for native libraries" (rust-lang/rfcs#2951) accepted through MCP rust-lang/compiler-team#356.
The feature gate for the issue is #![feature(native_link_modifiers)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@petrochenkov petrochenkov added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jan 28, 2021
@jonas-schievink jonas-schievink added the A-linkage Area: linking into static, shared libraries and binaries label Jan 28, 2021
@petrochenkov petrochenkov self-assigned this Feb 7, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 5, 2021
…trochenkov

Implement RFC 2951: Native link modifiers

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

Tracking Issue: rust-lang#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`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 6, 2021
…trochenkov

Implement RFC 2951: Native link modifiers

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

Tracking Issue: rust-lang#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`
@12101111
Copy link
Contributor

warning: the feature `native_link_modifiers` is incomplete and may not be safe to use and/or cause compiler crashes
 --> library/unwind/src/lib.rs:8:12
  |
8 | #![feature(native_link_modifiers)]
  |            ^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(incomplete_features)]` on by default
  = note: see issue #81490 <https://github.com/rust-lang/rust/issues/81490> for more information

There are 4 feature gate: native_link_modifiers_{as_needed,bundle,verbatim,whole_archive}, are they both incomplete or just some of them are incomplete?

@petrochenkov
Copy link
Contributor Author

I don't think the incomplete warning is correct, the modifiers are not yet stable, but they are complete enough.

bors added a commit to rust-lang-ci/rust that referenced this issue 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.
@luqmana
Copy link
Member

luqmana commented Sep 10, 2021

@petrochenkov Is there anything left before at least stabilizing the modifiers syntax (the native_link_modifiers feature)? If not I can help move that forward. It's definitely something we need to do before we stabilize any of the specific formats.

@petrochenkov
Copy link
Contributor Author

No, as far as I know (I wanted to audit the implementation once more though).
It probably makes sense to stabilize the syntax together with one of the specific modifiers, otherwise the stabilization is all the responsibility and none of the profits.

@krasimirgg
Copy link
Contributor

@petrochenkov I'm interested in helping out getting this stabilized if that's OK.
I'm hoping this RFC will help us improve Bazel Rust support.
I'm a novice with respect to rustc development.
What's left and what's the best way to get involved?

Maybe: the Relative order of -l and -Clink-arg(s) options part of the RFC is not yet implemented: #83507 (comment). Is anyone working on it, otherwise does it sound reasonable for me to pick this up?

@petrochenkov
Copy link
Contributor Author

@krasimirgg
I actually just started auditing the implementation for stabilization today (the general modifier syntax, and the whole-archive modifier specifically). If no other task interrupts me, then I'll likely make a PR tomorrow.

The "relative order" part is entirely orthogonal though, so you can certainly work on that.
I remember starting implementing that and discovering that it is a more invasive change than I expected, so I abandoned it due to a lack of time, IIRC.

The code parsing these options (-l, -C link-arg and -C link-args) lives in compiler\rustc_session\src\options.rs and in compiler\rustc_session\src\config.rs.
It currently builds two separate option vectors - Options::libs and CodegenOptions::link_args, instead it needs to build a single vector for the both kinds of options that will then be used throughout the compiler.

@hlopko
Copy link
Contributor

hlopko commented Feb 11, 2022

I have a few clarifying questions if I may :)

  1. So the stabilization of modifiers is orthogonal to the relative order fix?

  2. Should we still reference the same RFC for the work?

  3. Should we guard the new behavior of the relative order fix behind an experimental flag? Or see it as a bugfix and change the default behavior of the compiler?

Thanks!

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Feb 11, 2022

@hlopko

  1. Yes.
  2. Yes.
  3. No, probably not, I see it as a bugfix. See below.

@petrochenkov
Copy link
Contributor Author

I remember starting implementing that and discovering that it is a more invasive change than I expected, so I abandoned it due to a lack of time, IIRC.

I now remember why I abandoned it.

The linker command line is constructed in fn linker_with_args.
Currently the options are passed roughly like this:

  • libraries (add_local_native_libraries in particular)
  • options added by the compiler (add_order_independent_options)
  • options added by the user (add_user_defined_link_args)

So user options passed with -Clink-arg(s) can be used to override options added by the compiler by default.
This behavior must be kept.

So I guess the user options will have to be split into two groups now - "before the last -l" and "after the last -l", with the "Relative order of -l and -Clink-arg(s)" change affecting only the first group.
This is all pretty annoying, I need to think about it more, maybe a new option will be required instead.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Feb 11, 2022

maybe a new option will be required instead

For example, we can reuse the -l itself, by adding a new kind :)

-l link-arg=something

Then such args will always be ordered relatively to other -l options and used (relatively) early on the command line (add_local_native_libraries), and the old -C link-args will be left alone.

@petrochenkov
Copy link
Contributor Author

@krasimirgg
Could you say which specific options did you plan to order relatively to -ls in #83507 (comment)?

@petrochenkov
Copy link
Contributor Author

Stabilization PR for the bundle modifier - #95818.
It's currently blocked, but it documents the step-by-step path to merging it.

@krasimirgg
Copy link
Contributor

We're seeing some issues with linker argument order for -lstatic native dependencies after rust-lang commit 1004783. As far as I understand,

  • before rustc lowered -lstatic=foo into something like -Wl,--whole-archive -llib -Wl,--no-whole-archive
  • after rustc lowers it to -llib directly.

I've prepared an example to illustrate the issue: https://github.com/krasimirgg/native-libs-order-example.
In this example we've got a rust binary main that depends on a rust library bar that depends on a native library foo.
You can run in by running ./run-in-docker.sh. That script sets up a Docker container, then first runs the build with the stable rustc, then tries to run the same build using a recent nightly. The output looks like:

+ docker run -it rustc-linkargs-example:latest ./run.sh
info: using existing install for 'stable-x86_64-unknown-linux-gnu'
info: default toolchain set to 'stable-x86_64-unknown-linux-gnu'

  stable-x86_64-unknown-linux-gnu unchanged - rustc 1.60.0 (7737e0b5c 2022-04-04)

gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

rustc 1.60.0 (7737e0b5c 2022-04-04)
LLD 10.0.0 (compatible with GNU linkers)
Creating native library foo
Building rust rlib bar
Building rust bin main
Cleaning up
+ docker run -it rustc-linkargs-example:latest ./run.sh nightly
info: using existing install for 'nightly-2022-05-02-x86_64-unknown-linux-gnu'
info: default toolchain set to 'nightly-2022-05-02-x86_64-unknown-linux-gnu'

  nightly-2022-05-02-x86_64-unknown-linux-gnu unchanged - rustc 1.62.0-nightly (4dd8b420c 2022-05-01)

gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
Copyright (C) 2019 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

rustc 1.62.0-nightly (4dd8b420c 2022-05-01)
LLD 10.0.0 (compatible with GNU linkers)
Creating native library foo
Building rust rlib bar
Building rust bin main
error: linking with `cc` failed: exit status: 1
  |
  = note: "cc" "-m64" "/tmp/rustcSwvF2s/symbols.o" "main.main.729108ea-cgu.0.rcgu.o" "main.1cg9fo4wc2frqp4k.rcgu.o" "-Wl,--as-needed" "-L" "/example" "-L" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,-Bstatic" "-lfoo" "/example/libbar.rlib" "-Wl,--start-group" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-202fc93d8ccaebf2.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-1333333cbe389678.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libobject-93491bde8b3642ba.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libmemchr-d338f5690e3fda2f.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libaddr2line-2cd7f06709609788.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libgimli-05bd833c6cc845b5.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_demangle-5543e955d2b2e107.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd_detect-a963b8f78c0365f5.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libhashbrown-626bd4749ba5679b.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libminiz_oxide-1c5c08d77aa4ee1f.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libadler-43c97e136c6f66b3.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_std_workspace_alloc-3ad551729ddf5bdf.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libunwind-46de9b9399df1cae.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcfg_if-aa03de290f9594ce.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-020914c5936c5f85.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc-89782a6344bc3ddf.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_std_workspace_core-2a6a2797f7a73818.rlib" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-0e3656b1fda5fd7b.rlib" "-Wl,--end-group" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-16d69221f10b0282.rlib" "-Wl,-Bdynamic" "-lgcc_s" "-lutil" "-lrt" "-lpthread" "-lm" "-ldl" "-lc" "-Wl,--eh-frame-hdr" "-Wl,-znoexecstack" "-L" "/root/.rustup/toolchains/nightly-2022-05-02-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-o" "main" "-Wl,--gc-sections" "-pie" "-Wl,-zrelro,-znow" "-nodefaultlibs" "-fuse-ld=lld" "-Wl,--warn-backrefs" "-Wl,--fatal-warnings"
  = note: ld.lld: error: backward reference detected: f in /example/libbar.rlib(bar.bar.412f8222-cgu.0.rcgu.o) refers to /example/libfoo.a(foo.o)
          collect2: error: ld returned 1 exit status
          

error: aborting due to previous error

With nightly rustc the linker reports a backward reference error of the bar rlib referring back to foo. The issue is that the -lfoo appears before bar.rlib in the linker command line:

ld ... "-Wl,-Bstatic" "-lfoo" "/example/libbar.rlib" [rust stdlib rlibs] ...

With the old rustc behavior, these native libs were put in a --whole-archive range, and the linker is more relaxed about the order in such case.

To address this, would it make sense to update rustc to mention -lstatic=foo libs one more time on the linker command line, after all the non-standard rlibs (or after all rlibs)? For example, the linker invocation pattern would change to:

ld ... "-Wl,-Bstatic" "-lfoo" "/example/libbar.rlib" "-lfoo" [rust stdlib rlibs] ...

@petrochenkov
Copy link
Contributor Author

@krasimirgg
The change is intentional, before #93901 rustc added --whole-archive to static libraries in some (semi-random) cases.
After #93901 we are no longer doing that, and the documentation was updated to mention that, and the release notes will mention it too.

The fix is (usually) to reorder the native libraries to respect dependencies between them, or (more rarely) to add a +whole-archive modifier. E.g. see #96328 (comment) in another reported issue about this.

@krasimirgg
Copy link
Contributor

@petrochenkov I agree. This example is a bit different: there is just 1 native library and a reference from an rlib to it. The problem is with the relative order between an rlib and its native lib dependency. The general instance of this problem is something like: anytime something uses a picky linker with an rlib -> native dependency, they risk backward reference errors and will have to fall back to using +whole-archive.

In Bazel rules_rust, we currently have a knob to use +whole-archive as a workaround for this issue, but it's not ideal, as we ideally want to only use whole-archive for libraries that really need it (alwayslink) and use standard linker options for standard libraries, enabling more linker optimisations.

We could update rules_rust to use an additional -Clink-arg=-lfoo for each native dep of a transitive rlib dependency, but I thought that this problem is general enough to try and address within rustc itself, if possible.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented May 3, 2022

This example is a bit different: there is just 1 native library and a reference from an rlib to it.

Nothing in https://github.com/krasimirgg/native-libs-order-example indicates that libbar.rlib depends on libfoo.a though.
Rustc does preserve dependencies when they are specified, typically bar.rs would have a #[link(name = foo)] in its source code or println!("cargo:rustc-link-lib=foo"); (*) in its build script.
But if dependencies are not specified, then rustc is free to put libraries in any random order.

(*) This will result in rustc ... -l bar ... when building libbar.rlib, which won't link anything because no linker is involved when producing rlibs, but will register the dependency.

UPD:

typically bar.rs would have a #[link(name = foo)] in its source code or println!("cargo:rustc-link-lib=foo"); (*) in its build script

The -bundle modifier also needs to be used on foo, otherwise libfoo.a will be "copypasted" into libbar.rlib, which is not desirable in this case, as I understand.

@petrochenkov
Copy link
Contributor Author

To address this, would it make sense to update rustc to mention -lstatic=foo libs one more time on the linker command line, after all the non-standard rlibs (or after all rlibs)?

Rustc is not the right abstraction level for that, IMO, since it's not aware of dependencies between libraries unless they are specified externally.
As I understand Bazel/rules_rust is a replacement of cargo, so it should be aware of all the dependencies, and can either pass -l options when building rlibs to register dependencies, or to order the final command line while preserving dependencies.

@krasimirgg
Copy link
Contributor

Thank you very much! What I was missing was this way to instruct rustc to say an rlib depends on a native library at the time of building the rlib (with the -bundle modifier):

This will result in rustc ... -l bar ... when building libbar.rlib, which won't link anything because no linker is involved when producing rlibs, but will register the dependency.

I think Bazel/rules_rust wasn't doing this properly in all cases, and because previously rustc was always using --whole-archive for native deps we didn't notice these problems until now. I'll look into adapting rules_rust.

@krasimirgg
Copy link
Contributor

@petrochenkov I'm curious what's the mechanism via which rustc registers the dependency in this case:

(*) This will result in rustc ... -l bar ... when building libbar.rlib, which won't link anything because no linker is involved when producing rlibs, but will register the dependency.

Does it do it by recording some metadata in the rmeta rlib archive member?

@petrochenkov
Copy link
Contributor Author

@krasimirgg

Does it do it by recording some metadata in the rmeta rlib archive member?

Yes.

@krasimirgg
Copy link
Contributor

@petrochenkov : Consider a case where an rlib depends on multiple native libraries, which themselves depend on each other, say rlib r depends on libnative1.a which itself depends on libnative2.a. My understanding is that while building r, we'll supply its native dependencies in the expected relative linker order, e.g., rustc -o r.rlib ... -lstatic:-bundle=native1 -lstatic:-bundle=native2 ....

Later, when r is used to build a rust binary b, I'd expect that in the linker invocation constructed to build b, the relative order of native1 and native2 will be preserved. Does rustc guarantee that?

@petrochenkov
Copy link
Contributor Author

@krasimirgg

I'd expect that in the linker invocation constructed to build b, the relative order of native1 and native2 will be preserved. Does rustc guarantee that?

Yes, in practice.
See #96328 for some discussion about documenting it.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue May 31, 2022
Stabilize the `bundle` native library modifier

And remove the legacy `static-nobundle` linking kind.

Stabilization report - rust-lang#95818 (comment).

cc rust-lang#81490
Closes rust-lang#37403
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 10, 2022
Stabilize the `bundle` native library modifier

And remove the legacy `static-nobundle` linking kind.

Stabilization report - rust-lang#95818 (comment).

cc rust-lang#81490
Closes rust-lang#37403
bors added a commit to rust-lang-ci/rust that referenced this issue Jun 10, 2022
Stabilize the `bundle` native library modifier

And remove the legacy `static-nobundle` linking kind.

Stabilization report - rust-lang#95818 (comment).

cc rust-lang#81490
Closes rust-lang#37403
@petrochenkov petrochenkov added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 18, 2022
@petrochenkov
Copy link
Contributor Author

@dcsommer
Copy link
Contributor

dcsommer commented Aug 2, 2022

For "Unresolved Questions" I think there is still the matter of the default of the bundle link modifier and it's purpose and interaction with respect to whole-archive. The full cross-product of intended behaviors for (+/-whole-archive, +/-bundle, crate-type) is a little unclear to me and would ideally get documentation.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Aug 2, 2022

@dcsommer
This is already documented in beta/nightly docs - https://doc.rust-lang.org/beta/rustc/command-line-arguments.html
Except that the compatibility case for whole-archive is intentionally vague, and the restriction on +bundle,+whole-archive in rlibs in not documented because it's sort of an implementation deficiency that we are planning to fix (#99429).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants