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

Project fails to link when using dylibs without the -Zshare-generics flag #67276

Closed
alexkornitzer opened this issue Dec 13, 2019 · 11 comments
Closed
Assignees
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexkornitzer
Copy link

I have opened this issue under the recommendation of @michaelwoerister who has been helping me track down the linking issues I have been having with dylibs.

As reported in previous issues which I piggybacked, the example provided will build on 1.36.0 but nothing newer: https://github.com/AlexKornitzer/dylib-errors/tree/error/consul

To quote @michaelwoerister, the -Zshare-generics flag has potentially been identified as the root cause:

OK, with the latest version of the error/consul branch I can reproduce. Interestingly the error goes away when compiling with RUSTFLAGS=-Zshare-generics=no.
The error also isn't present for me when compiling with cargo build --release (which is expected because --release implies -Zshare-generics=no)

Hence when building with --release the linking issues go away.

This is potentially related to #64319 and was originally being tracked in #64340.

@jonas-schievink jonas-schievink added A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 13, 2019
@michaelwoerister
Copy link
Member

Nominating for prioritization.

@michaelwoerister michaelwoerister added I-nominated regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Jan 6, 2020
@pnkfelix
Copy link
Member

pnkfelix commented Jan 9, 2020

pre-triage: Explicitly leaving nominated (and not prioritized); lets figure out the priority of this as a group tomorrow.

@michaelwoerister michaelwoerister self-assigned this Jan 15, 2020
@michaelwoerister
Copy link
Member

Alright, I have identified the cause of this problem. Consider the following subset of the repro's crate graph:

    tokio_timer.rlib         regex_syntax.rlib
         ^                     ^      ^
         |                     |      |
         +-----------+---------+      |
                     |             env_logger.rlib
                 shared.so            ^
                     ^                |
                     |                |
                     +-------+--------+
                             | 
                         serverctl.exe

Both tokio_timer and regex_syntax contain a reusable instance of Cell<isize>::set and we get a linker error that the symbol for Cell<isize>::set is undefined when trying to link serverctl. The problem is that:

  • when compiling env_logger the compiler only knows about the instance in regex_syntax, but
  • only the version from tokio_timer is reexported from shared.so, so that
  • the regex_syntax version cannot be found when the env_logger objects are being linked as part of serverctl.

-Zshare-generics deterministically choose an instance if multiple instances are available but the set to choose from can be different for different crates in the graph. That is fine if we make sure that these sets can only grow but in the case of dylibs the set is shrunk again because we don't re-export all available instances.

I'm pretty sure that's also the cleanest fix: Re-export all instances from dylibs.

@pnkfelix
Copy link
Member

@michaelwoerister Question: how can this be a stable-to-stable regression when it depends on -Zshare-generics, an unstable command-line option?

@pnkfelix
Copy link
Member

I'll put this on the explicit agenda for tomorrow.

@alexkornitzer
Copy link
Author

alexkornitzer commented Jan 16, 2020

@pnkfelix, thats my fault for putting in a bad issue title basically in 1.36.0 dylibs will link fine without the generics flag. In 1.37.0 and higher they will only compile with --release or the -Zshare-generics flags. Hence, regression from stable to stable.

@alexkornitzer alexkornitzer changed the title Project fails to link when using dylibs and the -Zshare-generics flag Project fails to link when using dylibs without the -Zshare-generics flag Jan 16, 2020
@michaelwoerister
Copy link
Member

@pnkfelix Yes, -Zshare-generics defaults to true for debug builds and on stable there is no way to opt out (because the -Z flag is not available).

@pnkfelix pnkfelix added P-high High priority and removed I-nominated labels Jan 16, 2020
bors added a commit that referenced this issue Jan 20, 2020
…alexcrichton

Make sure that all upstream generics get re-exported from Rust dylibs.

This PR contains a fix for #67276. Rust dylibs would not re-export all generic instances when compiling with `-Zshare-generics=on` (=default for debug builds) which could lead to situations where the compiler expected certain generic instances to be available but then the linker would not find them.

### TODO
- [x] Write a regression test based on the description [here](#67276 (comment)).
- [x] Find out if this also fixes other issues related to #64319.

r? @alexcrichton ~~(once the TODOs are done)~~
cc @pnkfelix @alexkornitzer
@alexkornitzer
Copy link
Author

Now that the PR is merged, will report back once the nightly is live so that I can test on a non toy example.

@michaelwoerister
Copy link
Member

Excellent. Thank you.

@alexkornitzer
Copy link
Author

Just tried the latest nightly and problem solved, thanks again for sorting it out :D

@michaelwoerister
Copy link
Member

That's great to hear. Thanks for the reproduction case, it really helped!

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-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. 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

4 participants