Wrap libraries in linker groups, allowing backwards/circular references

[joshtriplett]

Some native library sets, such as components of libc, libgcc, or
compiler-builtins, require backwards or circular references.
Additionally, because link order doesn't matter for native libraries
when using dynamic linking, many crates don't take it into account for
static linking either; this can introduce platform-specific issues.
Rather than make the order of #[link] lines or cargo:rustc-link-lib
lines semantically significant (and potentially even require specifying
some libraries twice), wrap all libraries in linker groups. This ensures
that symbols in all libraries resolve correctly regardless of library
order.

Linker groups of reasonable sizes don't have a substantial cost, but
putting the entirey of a large link line inside a linker group can
add enough cost to be noticeable. To minimize the cost of linker groups,
use separate groups for local native libraries, upstream crates, and
upstream native libraries. This doesn't allow backreferences between
those groups, but in most cases we won't want such backreferences.

[rust-highfive]

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

[joshtriplett]

@bors try

[bors]

⌛ Trying commit 95087264ce73f6ba1e177dbfde29b3099cc986f2 with merge e1d60f651d7c6ca5c639dc1482eab9ca708d4162...

[bors]

☀️ Try build successful - checks-actions
Build commit: e1d60f651d7c6ca5c639dc1482eab9ca708d4162 (e1d60f651d7c6ca5c639dc1482eab9ca708d4162)

[jackh726]

I definitely don't know enough here to review. @joshtriplett do you know anyone better?

[alexcrichton]

I would expect the motivation of a PR like this to just generally make things easier than they are today. Are there other motivations though?

Originally I tried to avoid the --start-group and such flags as much as possible, mainly because the man page has the scary warning:

Using this option has a significant performance cost. It is best to use it only when there are unavoidable circular references between two or more archives.

I never quantified that performance cost myself nor did I ever dig around to figure out what that clause was added in the first place. My best guess is that it requires that more symbols are kept in memory, but that didn't seem like a reason to put such a scary warning in the man page...

That all being said if there isn't actually really much of a performance issue from this, I think that this would improve consistency across platforms a little. MSVC I believe behaves as-if --start-group and --end-group are used across the entire command line. It doesn't actually have any knobs for this, I think it's just how it works. Although this is a double-edged sword in that it could make Rust-based development easier but make integrating a Rust staticlib into other projects more difficult if they aren't already using --start-group and --end-group.

A lot of the linker args and stuff in rustc is generally set up to specify things in a topologically-sorted fashion. There's a ton of stuff in various places that tries to be extremely careful about the order of linked libraries. I think all of this though is in the category of "this would generate an error otherwise", not something like "if this is in the other order bad things happen". Mostly this is just me realizing that there's probably a lot of tangential stuff that could be simplified after landing a change like this (probably no need to do it inline here of course).

Anyway, overall I think this would be a nice thing to land. It's just one less thing to worry about with the linker which AFAIK has little cost. I do think it would be worthwhile to figure out that performance claim though in the man page and see if it's something that big projects will eventually run into or whether it's something Rust doesn't run into. Once we land this I doubt there's really any turning back. The best we could do in the future would be to add an option to go back to the old behavior, but that'd be a bummer to maintain that...

[joshtriplett]

@alexcrichton wrote:

I would expect the motivation of a PR like this to just generally make things easier than they are today. Are there other motivations though?

Yes, I did have a specific motivation beyond just making it much easier to handle static libraries. In the course of handling aarch64 outline-atomics and adding the symbols to compiler-builtins for that, it turns out that that creates a circular dependency between compiler-builtins and libc. So, this is actually needed on aarch64 now, and rather than do a special-case fix I wanted to address the general problem.

Apart from that, I also think this will help us along the path towards standardizing the handling of dynamic vs static linking of native libraries. Right now, numerous Rust crates handle that via feature flags; I'd love to standardize a general "please link your native libraries statically" flag for native crates to give the top-level crate more control about the linking of native libraries in dependencies. I think that will be much easier if those crates don't suddenly have to worry about library ordering when they didn't for dynamic linking.

MSVC I believe behaves as-if --start-group and --end-group are used across the entire command line. It doesn't actually have any knobs for this, I think it's just how it works.

LLVM's lld behaves this way as well; it doesn't need these options either.

I do think it would be worthwhile to figure out that performance claim though in the man page and see if it's something that big projects will eventually run into or whether it's something Rust doesn't run into.

Discussion in #76992 included some performance analysis that could not detect any appreciable performance hit at all.

[petrochenkov]

I would prefer not to give this guarantee now, but stabilize linking modifiers first (#81490), then migrate target specs / libc / libunwind to #[link] attributes, implement library deduplication, and then look what other cases for --(start,end)-group remain and how they can be addressed without compiler adding the groups implicitly.

Even with groups, linker argument order still remains significant in presence of duplicate / conflicting symbols (see e.g. #84254).
Besides that, not all linkers support the grouping options or behave like MSVC (?), so the order will remain significant for them as well.

[petrochenkov]

As a precedent, CMake does a pretty similar job of building a dependency tree of native libraries and turning it into a linker invocation, and it doesn't add grouping options implicitly, but rather gives users ability to add them manually.

[joshtriplett]

@petrochenkov So, I do want to see link modifiers, but that seems orthogonal to this.

And I agree that argument order still matters if you have duplicate symbols, and I added comments to that effect here. But I think it's still reasonable to make order matter less, so that people who have the more unusual case of duplicate symbols have to deal with it, but people who just have ordinary dependencies don't. That would affect the degree to which we need to add facilities to enable such ordering.

This is a problem we'd have to solve in order to deduplicate libraries, but I also think this will help us avoid cases where people would otherwise need to duplicate libraries (since circular dependencies wouldn't require that anymore).

To be explicit: I don't want to rip out the code that carefully maintains topological ordering, and I think any PR proposing to do so would need to address all the potential compatibility and future-compatibility concerns. In this PR, I've carefully preserved the topological ordering logic, and added a comment that should make it clear that it shouldn't be removed lightly.

Besides that, not all linkers support the grouping options or behave like MSVC (?),

To the best of my knowledge, all the linkers we currently support either have these options (ld) or don't need them because they behave that way by default (lld, MSVC). I'm not aware of any current linker we support that errors out on dependencies from a later library to an earlier one and doesn't support this option.

[petrochenkov]

I'm not aware of any current linker we support that errors out on dependencies from a later library to an earlier one and doesn't support this option.

Non-linker_is_gnu linkers that are not lld or link.exe need checking (that's Apple, Solaris, L4Re?, maybe wasm and nvptx linkers?).

[petrochenkov]

One additional thing that I don't like is that --(start,end)-group make accidental dependencies easier.

For example, in crate a you write an extern block without linking to anything, but it still works because some of your parent or neighbor crates (or native libraries) specifies the necessary library.

[joshtriplett]

I'm not aware of any current linker we support that errors out on dependencies from a later library to an earlier one and doesn't support this option.

Non-linker_is_gnu linkers that are not lld or link.exe need checking (that's Apple, Solaris, L4Re?, maybe wasm and nvptx linkers?).

The linkers typically used with wasm don't need this. My understanding is that Apple's ld (and zld) don't care either. I also found explicit documentation on Solaris's linker saying that it resolves symbol references that go backwards in order, as well.

[joshtriplett]

One additional note: there's one more case where link order matters, namely the accumulation of symbols into sections. For instance, the order of .init_array and .fini_array sections can depend on link order. This is another reason why we should continue to preserve order and eventually provide easier ways to manage order. I still think it's reasonable to make this easier for people to deal with in the common case, though.

[alexcrichton]

Oh apparently I wrote the same comment as above some time ago as well... Well in any case those are my thoughts on this feature and I'd personally be ok with it so long as someone else is willing to back up the claims of peformance. (the testing in the thread doesn't seem to be super rigorous about big projects and performance there, I'd expect a performance difference for something like librustc.so or libllvm.so if it actually mattered)

[joshtriplett]

@alexcrichton I'll do some additional performance testing and follow up.

[joshtriplett]

I built cargo using a nightly compiler, and then re-ran the final rustc invocation with -C save-temps and -Z print-link-args. The resulting linker invocation had 214 .rcgu.o files and 120 .rlib files, in addition to various native libraries.

I used hyperfine to benchmark the resulting invocation:

Benchmark #1: sh test.sh
  Time (mean ± σ):      7.207 s ±  0.020 s    [User: 6.097 s, System: 1.105 s]
  Range (min … max):    7.183 s …  7.238 s    10 runs

I then deleted the -Wl,--start-group and -Wl,--end-group from the command line, confirmed that it still ran successfully, and used hyperfine to benchmark the resulting invocation:

Benchmark #1: sh test-nogroups.sh
  Time (mean ± σ):      7.216 s ±  0.012 s    [User: 6.113 s, System: 1.096 s]
  Range (min … max):    7.192 s …  7.233 s    10 runs

Then I wrapped the entire set of objects and libraries in -Wl,--start-group and -Wl,--end-group, and used hyperfine to benchmark the resulting invocation:

Benchmark #1: sh test-group.sh
  Time (mean ± σ):      7.521 s ±  0.012 s    [User: 6.396 s, System: 1.119 s]
  Range (min … max):    7.505 s …  7.543 s    10 runs

I also tried wrapping just std and below, plus the native libraries, in a group:

Benchmark #1: sh test-group.sh
  Time (mean ± σ):      7.372 s ±  0.023 s    [User: 6.217 s, System: 1.147 s]
  Range (min … max):    7.328 s …  7.410 s    10 runs

So, this does have a measurable impact with ld. Not an earth-shattering one, but a measurable one.

I still think this is worthwhile, though. But I can also try narrowing the scope of the group to see if I can reduce the impact.

[alexcrichton]

Can you take a look at the impact of a larger project like rustc as well? (rustc's large dynamic library that is, not the executable which should be very small). Ideally you'd also statically link LLVM so there's a lot of C++ code in the mix as well.

[joshtriplett]

The easiest way to do that might be a rust-timer invocation:

@rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[joshtriplett]

If this turns up an excessive performance impact, I'm going to try narrowing the scope of the group. It's not necessarily required to allow backreferences from native libraries to Rust libraries.

[joshtriplett]

Also, I confirmed that wrapping only std and below and the native libraries into a group has less impact:

Benchmark #1: sh test-group.sh
  Time (mean ± σ):      7.372 s ±  0.023 s    [User: 6.217 s, System: 1.147 s]
  Range (min … max):    7.328 s …  7.410 s    10 runs

[apiraino]

We'll revisit this PR this Fall, the team agrees with this comment, see Zulip discussion

[joshtriplett]

@apiraino Sounds good to me; no rush. I'm happy to update this to apply again at that time.

[tmandry]

When we do revisit, perhaps it should be as an MCP?

[petrochenkov]

Ok, I guess my main take on this is that backward references are exceptional, therefore they should be explicit.

If some symbol is satisfied through a backward referece, the it's more likely that it's some kind of missing unspecified dependency, rather than the intent.
So we should try preventing it by default, but should also provide some way to explicitly say "yep, this symbol is supposed to be satisfied through a backreference".

If we look at Rust dependecies as an example, cargo and rustc try very carefully to have all crate dependencies specified.
You cannot write extern crate foo; and accidentally access an indirect dependency crate or a crate that is a sibling in the build tree this way.
I think we should use the same approach to native dependencies as well, and we do generally use it now, but grouping suggested here will make a hole in it allowing to refer to native libraries linked by parents or siblings.

What specific way can be used to specify backreferences is probably not too important right now, if the main users are libc and libgcc like mentioned in the PR description, so it can be unstable and anything will go (an attribute or maybe a command line option, mentioning specific symbols or maybe whole libraries).

[petrochenkov]

Additionally, because link order doesn't matter for native libraries when using dynamic linking, many crates don't take it into account for static linking either

The dynamic library order does matter because we are using --as-needed by default on all platforms that support it, so dynamic libraries not satisfying any symbols at a given point on the command line will be "optimized away" by the linker.

[joshtriplett]

Based on discussions with @pnkfelix, I'm going to rework this patch to provide a separate option that enables linker groups, so that people can turn it on and experiment with it, but leave them disabled by default for now.

[Dylan-DPC]

@joshtriplett any updates on this?

[joshtriplett]

Discussed this with @pnkfelix today, and he's going to update the PR to add a -Z linker-groups option.

[camelid]

triage: @joshtriplett @pnkfelix what's the status of this PR?

[joshtriplett]

I'm going to close this for now; I don't have the bandwidth to pursue broader static linking reform at this time.