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

"sandwich" rlibs between native deps in linker order #1333

Merged
merged 4 commits into from
May 13, 2022

Conversation

krasimirgg
Copy link
Collaborator

@krasimirgg krasimirgg commented May 11, 2022

This is part of the work on #1268.

Consider a case where we have a rust binary B that depends on a rust library R that depends on a native library N. With a recent nightly rustc that passes the native library via -l to the linker (not using --whole-archive as it used to) and a linker that checks for backward references, we can run into backward reference errors when N appears before R on the linker invocation. Example at rust-lang/rust#81490 (comment).

We came up with two approaches to deal with such cases: a "sandwich" one and one where we'd pass appropriate -l flags to rustc when building rust_libraries. This PR implements the "sandwich" one.

Under the "sandwich" approach, we modify the rustc command link for building B in a way that the native libraries section on the linker invocation is repeated once more after the rlib section:

ld ... n1 n2 ... r1 r2 ... n1 n2 ...
        X          Y         Z

By doing this, we allow the linker to resolve a dependency from an rlib to a native lib from Y to Z.

This should allow us to remove the experimental_use_whole_archive_for_native_deps stopgap feature, while additionally allowing us to take advantage of the new rustc no-whole-archive behaviour.

The approach to pass appropriate -l when building rust_libraries also seems plausible, but we've got some concerns about scaling in some cases, e.g., where we have deeply nested "ladder-like" dependency graphs composed of mixed rust and native libraries. Embedding the transitive set of native library names in the rlib also means we may need to build multiple variants of rlibs to work, e.g., in pic/no-pic mode, whereas now a single rlib is sufficient for both modes. We'll continue to investigate.

@krasimirgg krasimirgg requested a review from scentini May 12, 2022 07:26
@krasimirgg krasimirgg marked this pull request as ready for review May 12, 2022 07:26
Copy link
Collaborator

@scentini scentini left a comment

Choose a reason for hiding this comment

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

LG, thanks!

rust/private/rustc.bzl Outdated Show resolved Hide resolved
@keith
Copy link
Member

keith commented May 12, 2022

This probably fixes #1276 too

@scentini scentini merged commit 51f8e30 into bazelbuild:main May 13, 2022
krasimirgg added a commit to krasimirgg/rules_rust that referenced this pull request May 16, 2022
scentini pushed a commit that referenced this pull request May 16, 2022
This isn't needed anymore after #1333 I believe. This is part of the work on #1268.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants