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

Symbol names may not correctly account for optimization level differences between crates #64319

Closed
alexcrichton opened this issue Sep 9, 2019 · 6 comments · Fixed by #64324
Closed
Labels
A-codegen Area: Code generation 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.

Comments

@alexcrichton
Copy link
Member

On investigating rust-lang/wg-cargo-std-aware#32 I've managed to reduce this down to:

$ cat foo.rs
pub fn foo() {
    bar::<usize>();
}

pub fn bar<T>() {
    baz();
}

fn baz() {}
$ cat bar.rs
extern crate foo;

pub fn bar() {
    foo::foo();
}
$ rustc --crate-type rlib foo.rs
$ rustc --crate-type dylib bar.rs -L .
$ rustc --crate-type dylib bar.rs -L .  -C opt-level=2
error: linking with `link.exe` failed: exit code: 1120
  |
  = note: "C:\\Program Files (x86)\\Microsoft Visual Studio\\2019\\Community\\VC\\Tools\\MSVC\\14.22.27905\\bin\\HostX64\\x64\\link.exe" "/NOLOGO" "/NXCOMPAT" "/LIBPATH:C:\\Users\\alex\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "bar.bar.3a1fbbbh-cgu.0.rcgu.o" "/OUT:bar.dll" "/DEF:C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\lib.def" "bar.13x131manznmlae2.rcgu.o" "bar.4hnz5vh12lvh2qha.rcgu.o" "/OPT:REF,ICF" "/DEBUG" "/NATVIS:C:\\Users\\alex\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\intrinsic.natvis" "/NATVIS:C:\\Users\\alex\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\liballoc.natvis" "/NATVIS:C:\\Users\\alex\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\etc\\libcore.natvis" "/LIBPATH:." "/LIBPATH:C:\\Users\\alex\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-msvc\\lib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libfoo.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libstd-b2f27b8d08c4688f.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libpanic_unwind-9c73c9c2e052b2f1.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libbacktrace-7a588e8fa018f6bc.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\librustc_demangle-74b71f441b8acffe.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libhashbrown-42efce06651eab9c.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\librustc_std_workspace_alloc-7518db6030684168.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libunwind-f7edde5930d50b47.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libcfg_if-30189c8e78e151e8.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\liblibc-5f5719f1cab83a12.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\liballoc-f297c401e81b90c6.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\librustc_std_workspace_core-f8c80c1aefab6a32.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libcore-6d66b6e58725d3ed.rlib" "C:\\Users\\alex\\AppData\\Local\\Temp\\rustcAYUABK\\libcompiler_builtins-1f6a73e107798f53.rlib" "advapi32.lib" "ws2_32.lib" "userenv.lib" "msvcrt.lib" "/DLL" "/IMPLIB:bar.dll.lib"
  = note: lib.def : error LNK2001: unresolved external symbol _ZN3foo3bar17h95b7da637b923d2cE
          bar.dll.lib : fatal error LNK1120: 1 unresolved externals


error: aborting due to previous error

The platform used here to reproduce the error is x86_64-pc-windows-msvc, but it looks like macOS also generates the same error. The error doesn't appear to manifest on Linux, presumably because the linker options just ignore non-defined symbols.

Here you can see that when both crates are compiled with the same optimization level the link succeeds, but with different optimization levels the wrong symbol name is generated, so the wrong symbol is asked to be exported which causes an error.

@alexcrichton alexcrichton added A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 9, 2019
@alexcrichton
Copy link
Member Author

cc @michaelwoerister and @eddyb, y'all may know where to start looking for this?

@jonas-schievink jonas-schievink added A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. labels Sep 9, 2019
@alexcrichton
Copy link
Member Author

I did a bit of instrumentation in the compiler, and it looks like this line is changing depending on which crate is compiling. When compiling the foo crate the value being hashed there is "foo". When compiling bar, however, if you turn off optimizations it's also hashing "foo" (and instantiating_crate is crate15). If you turn on optimizations in the bar crate, however, then it hashes "bar" because instantiating_crate is crate0. That to me implies some funkiness around here where instantiating_crate is calculated.

Sure enough it looks like tcx.sess.opts.share_generics() is changing between compilations which causes issues. I'll look into fixing this locally.

@alexcrichton
Copy link
Member Author

Ok, posted #64324!

bors added a commit that referenced this issue Sep 24, 2019
rustc: Fix mixing crates with different `share_generics`

This commit addresses #64319 by removing the `dylib` crate type from the
list of crate type that exports generic symbols. The bug in #64319
arises because a `dylib` crate type was trying to export a symbol in an
uptream crate but it miscalculated the symbol name of the uptream
symbol. This isn't really necessary, though, since `dylib` crates aren't
that heavily used, so we can just conservatively say that the `dylib`
crate type never exports generic symbols, forcibly removing them from
the exported symbol lists if were to otherwise find them.

The fix here happens in two places:

* First is in the `local_crate_exports_generics` method, indicating that
  it's now `false` for the `Dylib` crate type. Only rlibs actually
  export generics at this point.

* Next is when we load exported symbols from upstream crate. If, for our
  compilation session, the crate may be included from a dynamic library,
  then its generic symbols are removed. When the crate was linked into a
  dynamic library its symbols weren't exported, so we can't consider
  them a candidate to link against.

Overally this should avoid situations where we incorrectly calculate the
upstream symbol names in the face of differnet `share_generics` options,
ultimately...

Closes #64319
Centril added a commit to Centril/rust that referenced this issue Sep 25, 2019
…r=michaelwoerister

rustc: Fix mixing crates with different `share_generics`

This commit addresses rust-lang#64319 by removing the `dylib` crate type from the
list of crate type that exports generic symbols. The bug in rust-lang#64319
arises because a `dylib` crate type was trying to export a symbol in an
uptream crate but it miscalculated the symbol name of the uptream
symbol. This isn't really necessary, though, since `dylib` crates aren't
that heavily used, so we can just conservatively say that the `dylib`
crate type never exports generic symbols, forcibly removing them from
the exported symbol lists if were to otherwise find them.

The fix here happens in two places:

* First is in the `local_crate_exports_generics` method, indicating that
  it's now `false` for the `Dylib` crate type. Only rlibs actually
  export generics at this point.

* Next is when we load exported symbols from upstream crate. If, for our
  compilation session, the crate may be included from a dynamic library,
  then its generic symbols are removed. When the crate was linked into a
  dynamic library its symbols weren't exported, so we can't consider
  them a candidate to link against.

Overally this should avoid situations where we incorrectly calculate the
upstream symbol names in the face of differnet `share_generics` options,
ultimately...

Closes rust-lang#64319
@bors bors closed this as completed in 50c57d8 Sep 25, 2019
@alexcrichton
Copy link
Member Author

Reopening due to #66018

@alexcrichton alexcrichton reopened this Nov 1, 2019
tmandry added a commit to tmandry/rust that referenced this issue Nov 1, 2019
…r=alexcrichton

Revert PR 64324: dylibs export generics again (for now)

As discussed on PR rust-lang#65781, this is a targeted attempt to undo the main semantic change from PR rust-lang#64324, by putting `dylib` back in the set of crate types that export generic symbols.

The main reason to do this is that PR rust-lang#64324 had unanticipated side-effects that caused bugs like rust-lang#64872, and in the opinion of @alexcrichton and myself, the impact of rust-lang#64872 is worse than rust-lang#64319.

In other words, it is better for us, in the short term, to reopen rust-lang#64319 as currently unfixed for now than to introduce new bugs like rust-lang#64872.

Fix rust-lang#64872

Reopen rust-lang#64319
bors added a commit to rust-lang/cargo that referenced this issue Dec 2, 2019
Stabilize profile-overrides.

This stabilizes the profile-overrides feature. This was proposed in [RFC 2282](rust-lang/rfcs#2282) and implemented in #5384. Tracking issue is rust-lang/rust#48683.

This is intended to land in 1.41 which will reach the stable channel on Jan 30th.

This includes a new documentation chapter on profiles. See the ["Overrides" section](https://github.com/rust-lang/cargo/blob/9c993a92ce33f219aaaed963bef51fc0f6a7677a/src/doc/src/reference/profiles.md#overrides) in `profiles.md` file for details on what is being stabilized.

Note: The `config-profile` and `named-profiles` features are still unstable.

Closes #6214

**Concerns**
- There is some risk that `build-override` may be confusing with the [proposed future dedicated `build` profile](#6577). There is some more discussion about the differences at rust-lang/rust#48683 (comment). I don't expect it to be a significant drawback. If we proceed later with a dedicated `build` profile, I think we can handle explaining the differences in the documentation. (The linked PR is designed to work with profile-overrides.)
- I don't anticipate any unexpected interactions with `config-profiles` or `named-profiles`.
- Some of the syntax like `"*"` or `build-override` may not be immediately obvious what it means without reading the documentation. Nobody suggested any alternatives, though.
- Configuring overrides for multiple packages is currently a pain, as you have to repeat the settings separately for each package. I propose that we can extend the syntax in the future to allow a comma-separated list of package names to alleviate this concern if it is deemed worthwhile.
- The user may not know which packages to override, particularly for some dependencies that are split across multiple crates. I think, since this is an advanced feature, the user will likely be comfortable with using things like `cargo tree` to understand what needs to be overridden. There is [some discussion](rust-lang/rust#48683 (comment)) in the tracking issue about automatically including a package's dependencies, but this is somewhat complex.
- There is some possibly confusing interaction with the test/bench profile. Because dependencies are always built with the dev/release profiles, overridding test/bench usually does not have an effect (unless specifying a workspace member that is being tested/benched). Overriding test/bench was previously prohibited, but was relaxed when named profiles were added.
- We may want to allow overriding the `panic`, `lto`, and `rpath` settings in the future. I can imagine a case where someone has a large workspace, and wants to override those settings for just one package in the workspace. They are currently not allowed because it doesn't make sense to change those settings for rlibs, and `panic` usually needs to be in sync for the whole profile.
- There are some strange interactions with `dylib` crates detailed in rust-lang/rust#64319. A fix was attempted, but later reverted. Since `dylib` crates are rare (this mostly applied to `libstd`), and a workaround was implemented for `build-std` (it no longer builds a dylib), I'm not too worried about this.
- The interaction with `share-generics` can be quite confusing (see rust-lang/rust#63484). I added a section in the docs that tries to address this concern. It's also possible future versions of `rustc` may handle this better.
- The new documentation duplicates some of the information in the rustc book.  I think it's fine, as there are subtle differences, and it avoids needing to flip back and forth between the two books to learn what the settings do.
michaelwoerister added a commit to michaelwoerister/rust that referenced this issue Jan 20, 2020
The regression test is originally from rust-lang#64324 but was removed again
after the fix in there turned out to break other things.
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
@jdm
Copy link
Contributor

jdm commented Jan 22, 2020

Is this now fixed by #68277?

@michaelwoerister
Copy link
Member

Yes, that should be the case. A regression test for this issue was added in 0a9bcb0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation 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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants