native dependencies fail to link on nightly-2022-04-01

[BusyJay]

Sorry, I don't have a minimal reproduction repo. The error can be found at https://github.com/BusyJay/tirocks/runs/5784258424?check_suite_focus=true.

It doesn't seem to be able to resolve the required symbols somehow. I can verify that it compiles with the same code on nightly-2022-03-31.

Meta

rustc --version --verbose:

rustc --version --verbose
rustc 1.61.0-nightly (0677edc86 2022-03-31)
binary: rustc
commit-hash: 0677edc86e342f333d4828b0ee1ef395a4e70fe5
commit-date: 2022-03-31
host: x86_64-unknown-linux-gnu
release: 1.61.0-nightly
LLVM version: 14.0.0

[BusyJay]

It passes build on MacOS, but the tests show that a global shared static variable between rust side and C side is wrong.

[BusyJay]

It's introduced by #93901

[petrochenkov]

I'll check what happens.
Either crate gets something wrong with native library dependency order, or some library needs the +whole-archive modifier which is no longer added by rustc by default in some cases.

[petrochenkov]

Either crate gets something wrong with native library dependency order, or some library needs the +whole-archive modifier

It's actually both.

Dependency order

The build script in tirocks-sys/build.rs gets the dependency order wrong.
crocksdb depends on both rocksdb and stdc++, and rocksdb also depends on stdc++, so the linking order should look like -lcrocksdb -lrocksdb -lstdc++.
Example of a possible fix:

diff --git a/tirocks-sys/build.rs b/tirocks-sys/build.rs
index 1e1bcf7..b65f479 100644
--- a/tirocks-sys/build.rs
+++ b/tirocks-sys/build.rs
@@ -206,7 +206,7 @@ fn build_titan(build: &mut Build) {
     build.include(cur_dir.join("titan"));
 }

-fn build_rocksdb(build: &mut Build) {
+fn build_rocksdb(build: &mut Build) -> PathBuf {
     let target = env::var("TARGET").expect("TARGET was not set");
     let mut cfg = Config::new("rocksdb");
     cfg.out_dir(format!("{}/rocksdb", env::var("OUT_DIR").unwrap()));
@@ -225,7 +225,6 @@ fn build_rocksdb(build: &mut Build) {
         .build_target("rocksdb")
         .very_verbose(true)
         .build();
-    figure_link_lib(&dst, "rocksdb");

     if cfg!(target_os = "windows") {
         build.define("OS_WIN", None);
@@ -251,6 +250,11 @@ fn build_rocksdb(build: &mut Build) {
         build.define("OPENSSL", None);
     }

+    dst
+}
+
+fn link_rocksdb(dst: PathBuf) {
+    figure_link_lib(&dst, "rocksdb");
     println!("cargo:rustc-link-lib=static=z");
     println!("cargo:rustc-link-lib=static=bz2");
     println!("cargo:rustc-link-lib=static=lz4");
@@ -262,13 +266,15 @@ fn main() {
     patch_libz_env();
     let mut build = Build::new();
     build_titan(&mut build);
-    build_rocksdb(&mut build);
+    let rocksdb_dst = build_rocksdb(&mut build);

     build.cpp(true).file("crocksdb/c.cc");
     if !cfg!(target_os = "windows") {
         build.flag("-std=c++11");
         build.flag("-fno-rtti");
     }
-    link_cpp(&mut build);
     build.warnings(false).compile("libcrocksdb.a");
+
+    link_rocksdb(rocksdb_dst);
+    link_cpp(&mut build);
 }

+whole-archive

rocksdb is a C++ library using global C++ constructors.
Smoke tests in cargo test -p tirocks-sys will fail if those constructors are not run.

rocksdb needs to be linked as whole-archive for the constructors to not be dropped.
Unfortunately it's not possible to just add the +whole-archive modifier due to the "linking modifiers +bundle and +whole-archive are not compatible with each other when generating rlibs" errors because tirocks-sys is a rlib and +bundle is the default for static libraries.

I'm afraid we'll have to revert some changes from #93901 and keep +whole-archive as default for some more cases until +whole-archive is easily usable in rlibs.

[BusyJay]

I guess a topological sorting is necessary to get all dependencies order right.

[BusyJay]

...so the linking order should look like -lcrocksdb -lrocksdb -lstdc++.

I change to the link order you suggested, but then the smoke case fails on both MacOS and Linux with the stable toolchain. Changing it to -lrocksdb -lcrocksdb -lstdc++ fix the problem on stable though.

[petrochenkov]

Fixed in #95606.

[Wilfred]

FWIW difftastic is also affected by this: running cargo build worked in nightly-2022-03-30 but fails with a linking error on newer nightlies:

Build log: https://github.com/Wilfred/difftastic/runs/5791227463?check_suite_focus=true
build.rs: https://github.com/Wilfred/difftastic/blob/master/build.rs

[petrochenkov]

@Wilfred
In case of difftastic the fix is to respect dependencies between C and C++ libraries.

diff --git a/build.rs b/build.rs
index 4788fa6b0..977a0a518 100644
--- a/build.rs
+++ b/build.rs
@@ -22,6 +22,13 @@ impl TreeSitterParser {
             }
         }

+        let mut build = cc::Build::new();
+        build.include(&dir).warnings(false); // ignore unused parameter warnings
+        for file in c_files {
+            build.file(dir.join(file));
+        }
+        build.compile(self.name);
+
         if !cpp_files.is_empty() {
             let mut cpp_build = cc::Build::new();
             cpp_build
@@ -42,13 +49,6 @@ impl TreeSitterParser {
             }
             cpp_build.compile(&format!("{}-cpp", self.name));
         }
-
-        let mut build = cc::Build::new();
-        build.include(&dir).warnings(false); // ignore unused parameter warnings
-        for file in c_files {
-            build.file(dir.join(file));
-        }
-        build.compile(self.name);
     }
 }

It doesn't involve the "+whole-archive in rlib" case unfixable by user, so it won't be fixed by #95606.

[Wilfred]

@petrochenkov that works perfectly, huge thanks for taking a look :)

Do you know why this changed on nightly? I see RELEASES.md doesn't have a changelog for items since the last stable release.