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

build script fails to link #41416

Closed
lilydjwg opened this issue Apr 20, 2017 · 10 comments
Closed

build script fails to link #41416

lilydjwg opened this issue Apr 20, 2017 · 10 comments
Assignees
Labels
A-linkage Area: linking into static, shared libraries and binaries P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.

Comments

@lilydjwg
Copy link

lilydjwg commented Apr 20, 2017

In the demo, I have Rust calls my C function which links to an external library.

With rustc 1.17.0-nightly (8c4f2c64c 2017-03-22) and before, it builds and runs successfully. With rustc 1.18.0-nightly (666e7148d 2017-04-08) and later, I get undefined references. I get these Rust's from the official distribution (though without rustup). My system is Arch Linux up-to-date.

     Running `rustc --crate-name linkissue src/main.rs --crate-type bin --emit=dep-info,link -C debuginfo=2 -C metadata=9db7c238d92ce6c3 -C extra-filename=-9db7c238d92ce6c3 --out-dir /home/lilydjwg/temp/linkissue/target/debug/deps -L dependency=/home/lilydjwg/temp/linkissue/target/debug/deps -L native=/usr/lib -L native=/home/lilydjwg/temp/linkissue/target/debug/build/linkissue-ab7e3c957d2eb908/out -l lua -l m -l static=c-sys`
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/usr/lib64/rustlib/x86_64-unknown-linux-gnu/lib" "/home/lilydjwg/temp/linkissue/target/debug/deps/linkissue-9db7c238d92ce6c3.0.o" "-o" "/home/lilydjwg/temp/linkissue/target/debug/deps/linkissue-9db7c238d92ce6c3" "-Wl,--gc-sections" "-pie" "-nodefaultlibs" "-L" "/home/lilydjwg/temp/linkissue/target/debug/deps" "-L" "/usr/lib" "-L" "/home/lilydjwg/temp/linkissue/target/debug/build/linkissue-ab7e3c957d2eb908/out" "-L" "/usr/lib64/rustlib/x86_64-unknown-linux-gnu/lib" "-l" "lua" "-l" "m" "-Wl,-Bstatic" "-Wl,--whole-archive" "-l" "c-sys" "-Wl,--no-whole-archive" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-f4594d3e53dcb114.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/librand-1efbcfd8938372b6.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcollections-532a3dbf317eff86.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd_unicode-cfbd6648f7db2ee5.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-a0157c0ca919c364.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/libunwind-488b4ab4bd53a138.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc-ca07b617414dd0fa.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc_jemalloc-492d8ea7fa3384ff.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-88c194c15fdb6521.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-687e6a964d22cbb4.rlib" "/usr/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-987729be881d4d32.rlib" "-Wl,-Bdynamic" "-l" "dl" "-l" "rt" "-l" "pthread" "-l" "gcc_s" "-l" "pthread" "-l" "c" "-l" "m" "-l" "rt" "-l" "util"
  = note: /home/lilydjwg/temp/linkissue/target/debug/build/linkissue-ab7e3c957d2eb908/out/libc-sys.a(c.o): In function `call_c':
          /home/lilydjwg/temp/linkissue/src/c.c:8: undefined reference to `luaL_newstate'
          /home/lilydjwg/temp/linkissue/src/c.c:9: undefined reference to `luaL_openlibs'
          /home/lilydjwg/temp/linkissue/src/c.c:10: undefined reference to `luaL_loadstring'
          /home/lilydjwg/temp/linkissue/src/c.c:10: undefined reference to `lua_pcallk'
          collect2: error: ld returned 1 exit status


error: aborting due to previous error

I've diff'ed cargo run -vv, all other parts are the same, only the above error report appears with newer versions.

PS: you can replace lua53 in build.rs with whatever version you have in your system. It shouldn't matter.
PPS: this error occurs in Ubuntu 14.04 too.

@sga001
Copy link

sga001 commented Apr 24, 2017

I am also experiencing this issue.

In particular, I'm having issues linking to dynamic libraries.

In rust, when I declare the extern block I add #[link(name="libname")] but I get linking issues. If I instead add #[link(name="libname", kind="static")] everything works fine.

I created a super simple toy example to show this. The code is available at: https://github.com/sga001/rust-linking-issue

I tested the above with the following nightly versions:

rustc 1.18.0-nightly (2564711 2017-04-04) works fine.
rustc 1.18.0-nightly (91ae22a 2017-04-05) works fine.
rustc 1.18.0-nightly (50c1864 2017-04-06) does not work.
rustc 1.18.0-nightly (53f4bc3 2017-04-07) does not work.
rustc 1.18.0-nightly (ad36c2f 2017-04-09) does not work.
rustc 1.18.0-nightly (2bd4b5c 2017-04-23) does not work.

Here's a gist of the output from the latest nightly version.
https://gist.github.com/sga001/c6031a9f4c4756f97cdf025f433fcbbe

Note that this is not just about Boost. I used boost in the toy example because that's a common library. In my actual application this happens with boost, mpfr, gmp, and gomp (basically all libraries that I'm dynamically linking).

Thanks

P.S. I also posted this in (rust-lang/cc-rs#155)

@mbrubeck mbrubeck added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label May 2, 2017
@mbrubeck
Copy link
Contributor

mbrubeck commented May 2, 2017

Here's the commit range for the nightly regression range above, for the rust repo. (I'm not sure whether the bug is in Rust or Cargo, so I might be looking in the wrong place.)

78c3a49 from #40805 looks like it might be relevant.

@arielb1 arielb1 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 2, 2017
@lilydjwg
Copy link
Author

lilydjwg commented May 3, 2017

It shouldn't be in Cargo or gcc-rs etc, because they are the same in my case. Only rustc and rust-std are different.

@brson brson added A-linkage Area: linking into static, shared libraries and binaries I-wrong P-high High priority labels May 4, 2017
@alexcrichton
Copy link
Member

Thanks for the report @lilydjwg! Looks like @mbrubeck is spot on in a diagnosis.

The critical change is that the order of arguments passed to the linker is different than before:

"-l" "lua" "-l" "m" "-Wl,-Bstatic" "-Wl,--whole-archive" "-l" "c-sys" "-Wl,--no-whole-archive"

Previously code partitioned linked libraries into static first then dynamic. That was mostly just an accident and not intentional. In 78c3a49 it was changed, however, to not partition.

The real bug here is that the order libraries to the linker matters. The c-sys library requires lua, but is mentioned after lua, so the linker will discard the lua library before it reaches c-sys.

You can fix this locally by simply moving the call to pkg_config lower in your build script, but we may wish to consider restoring the old behavior though. As a test, though, is such a change difficult to make for you, or could you confirm that it does indeed work?

cc @vadimcn

@sga001
Copy link

sga001 commented May 4, 2017

@alexcrichton I think that I follow the issue that you explain but not the solution. In the example here: https://github.com/sga001/rust-linking-issue, which exhibits the same issue, what would be the solution?

I tried adding:

println!("cargo:rustc-link-search=/usr/lib/"); 
println!("cargo:rustc-link-lib=boost_system");

at the end of the build script and it did not fix the issue (perhaps the println statements appear first? in which case, how should I go about fixing this?).
More importantly, I already specify the linking in the relevant rust files via the attribute #[link(name="foo")]. Wouldn't your fix effectively mean that the link attribute is useless then? I guess maybe not since this case is a bit special in that it creates a static library that links to a dynamic library, and then rust code links to the newly created static library (so there is some chain of mix static and dynamic libraries). As you mentioned, by having all the dynamic link arguments at the end, this chaining is not an issue.

EDIT: It does work as soon as I remove all link attributes and leave the println in the build file. It seemed cleaner before though, where I could specify all libraries to link in link attributes rather than in the build script.

@vadimcn
Copy link
Contributor

vadimcn commented May 5, 2017

Although not officially specified anywhere, we pass libraries to the linker in this order:

  1. The local object that LLVM just generated.
  2. Local native libraries, in the order they were processed by the compiler. This means that the relative order of #[link] attributes within the same module is preserved. There's also some deterministic ordering between the modules, but I am not quite sure what it is.
  3. Upstream rust libraries.
  4. Native libraries from upstream crates, again in the order compiler saw them in.
  5. Native libraries passed on command line, in the order they appear (this is where cargo:rustc-link-lib='s end up).

As Alex mentioned, previously we reordered libraries in step (2) into all static ones followed by all dynamic ones, but that was done purely for convenience in that particular piece of code.
I thought that preserving the source- and command-line order will make it easier to reason about the overall library ordering, so I made that change.

In your case, it is important to keep in mind that build script helpers like pkg_config and gcc implicitly emit names of their output libraries as cargo:rustc-link-lib= when they run, so that's the order in which they end up on rustc's command line.
Since it looks like you cannot move pkg_config probe later in the script, IMO, the next best thing to do would be to add this:

  for l in lib.libs {
    println!('cargo:rustc-link-lib={}', l);
  }

but we may wish to consider restoring the old behavior though.

I don't think this is a good idea: the behavior depended on whether one or both libraries were static or dynamic. I think this is exactly what @sga001 is running into.

@sga001: Since libtest_time depends on boost_system and libtest_time dependency is declared via build script, I think it is actually more consistent to declare boost_system via the build script as well. Alternatively, you can try doing this instead:

#[link(name = "libtest_time")]
#[link(name = "boost_system")]
extern "C" {
    fn print_time();
}

@lilydjwg
Copy link
Author

lilydjwg commented May 5, 2017

@alexcrichton yes reordering works for me, but it's not as simple as moving lines, since I need to know the include files before compiling my C code. I have to do like this like @vadimcn said:

  let lib = pkg_config::Config::new().cargo_metadata(false).probe("lua53").unwrap();

  let mut gcc = gcc::Config::new();
  gcc.file("src/c.c");
  for p in lib.include_paths {
    gcc.include(p);
  }
  gcc.compile("libc-sys.a");

  for l in lib.libs {
    println!("cargo:rustc-link-lib={}", l);
  }

IMO this thing should be documented somewhere easy to find. I knew it should be ordering issues but I didn't see there was ordering that I could change in the beginning.

@alexcrichton
Copy link
Member

Ok glad to hear it's now working @lilydjwg! Also to confirm @sga001, you've gotten your example to work now as well, right?

At the absolute bare minimum I'll tag the relevant PR with relnotes. I'm personally leaning towards not reverting though if this is easy enough to patch up locally. In some sense the now-present behavior is "more correct" than the previous behavior, which makes me think that we should strive to make this transition.

The question then is how to ease the transition. If we have a lot of instances of this then we should definitely revert and think of a better strategy, but if the problems are isolated and easy to fix I think we can keep forging ahead.

That being said, though, do others on @rust-lang/tools have different opinions?

@sga001
Copy link

sga001 commented May 5, 2017

@alexcrichton: yeah it was easy to fix. Thanks for the help.

@nikomatsakis nikomatsakis added T-tools and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 25, 2017
@Mark-Simulacrum Mark-Simulacrum added T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. and removed T-tools labels May 28, 2017
@brson brson added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jun 15, 2017
@alexcrichton
Copy link
Member

Ok I'm going to close this as "expected breakage" at this point as the change has hit stable and there's explanations above for what this is and why it's changed from before.

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 P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants