fails to link `curl::init()`'s call to `INIT_CTOR` on macOS 12.0 (Monterey)

cormacrelf commented 2 years ago

Summary

Only on macOS. May occur on macOS earlier than 12 using Xcode 13, I'm not sure. Underlying issue is https://github.com/rust-lang/rust/issues/90342

You get a linker error, there is a workaround for end users of the crate (see the rust issue; set the env var). I think curl-rust specifically can avoid this problem.

Repro:

rust 1.56 stable
macOS 12.0.1 Monterey
cargo test in the curl-rust repo (currently at df64ee4).

This is using either system libcurl or with --features static-curl. Makes no difference.

$ cargo test

... very long cc -arch arm64 invocation
...
  = note: ld: reference to symbol (which has not been assigned an address) __ZN4curl4init9INIT_CTOR17h97cc33cf050cb462E in '__ZN4curl4init17ha644d831c2a57f65E' from /Users/cormac/git/tryout/libcurl-monterey/target/debug/deps/libcurl-0f9cbb7dde66dd88.rlib(curl-0f9cbb7dde66dd88.curl.0b6dcf6e-cgu.2.rcgu.o) for architecture arm64
          clang: error: linker command failed with exit code 1 (use -v to see invocation)

Possible solutions, other than waiting for the rust issue

The relevant symbol is defined at:

https://github.com/alexcrichton/curl-rust/blob/df64ee4fcb1e3e2927d04e6bb6d8a1c0feedc844/src/lib.rs#L93-L101

I think the actual bug is due to the the workaround for https://github.com/rust-lang/rust/issues/47384 referenced further down, in which the init function calls INIT_CTOR();.

I think you can solve it by:

Adding #[used] to INIT_CTOR (any MSRV issue? not sure)
Moving INIT_CTOR outside the init function
Change pub fn init() { INIT_CTOR(); } to pub fn init() { init_inner(); }

It works on macOS 12, at least. But I think it would be good enough, because:

We're just trying to get rust to preserve INIT_CTOR, despite that rust-lang/rust issue 47384 where not using any code from the module means it won't get linked at all
Does curl-rust have a problem with people using the library but managing not to use any code from it? If no code is used, do we need the pre-main initializer to run at all?
I don't think so. It's not obvious to me that curl-rust is actually affected by the thing it has a workaround for.

alexcrichton commented 2 years ago

I think any of the solutions you proposed would likely work fine, the goal of this is to basically automatically initialize and the other various details are just implementation oddities that are fine to change.

sagebind commented 2 years ago

Also yes your assessments are correct that the weird indirection in this code is a workaround for https://github.com/rust-lang/rust/issues/47384, where #[used] seems to not actually always have an effect, but actually invoking the static ctor symbol from a function that is likely referenced in the final binary (Easy::new() calls init() calls INIT_CTOR) ensures that it isn't optimized out. (I think that issue title is misleading, as sometimes #[used] items are optimized out even when located in modules that are referenced.)

Does curl-rust have a problem with people using the library but managing not to use any code from it? If no code is used, do we need the pre-main initializer to run at all?

Nope, the initializer only needs to run if the curl crate is actually used. Even then, it isn't strictly required to be run in order to use curl since we lazily invoke init as well in other various function calls, but it is preferred to be run to avoid multithreading issues as described in https://github.com/alexcrichton/curl-rust/issues/333.

cormacrelf commented 2 years ago

I think any of the solutions you proposed would likely work fine

I meant do all three of the things, in order for not calling the static to be okay. But if there are indeed other circumstances where its module is referenced but the static is still optimised out, then even doing all three won't guarantee it.

I had one other silly idea: set MACOSX_DEPLOYMENT_TARGET=10.7 if it's missing in curl-rust's own build.rs.

// Workaround for https://github.com/alexcrichton/curl-rust/issues/417
if cfg!(target_os = "macos") && env::var("MACOSX_DEPLOYMENT_TARGET").is_err() {
    println!("cargo:rustc-env=MACOSX_DEPLOYMENT_TARGET=10.7");
}

I tried it. It compiles with no error, with cargo test and cargo run --example ... (which are, if you think about it, 'other crates' depending on curl). A println I added to init_inner runs before one put in examples/aws_sigv4.rs' main function. I think that might do it.

sagebind commented 2 years ago

Would setting MACOSX_DEPLOYMENT_TARGET affect just compilation of the curl crate, or would it affect the entire program being compiled? Seems like an interesting alternative but would be concerned about other weird side-effects this could have.

cormacrelf commented 2 years ago

The curl crate. Not that the reference actually says this, but yes.

I'd be more worried about linking 10.7 intermediates with dependent crates and others. I can see that the tests/examples in this repo aren't actually a good enough test of this behaviour. In a way I think it would be more obviously fine if it could set an env var for the entire program being compiled.

shepmaster commented 2 years ago

Some playground utilities use cargo which uses curl. I ran into this problem.

As a workaround that doesn't involve updating any of my dependencies, I can make use of the MACOSX_DEPLOYMENT_TARGET idea:

% cargo clean
% MACOSX_DEPLOYMENT_TARGET=11 cargo run

This allows me to hobble on until it's properly fixed by y'all wonderful people.

_{Which I now see was mentioned in the upstream issue 🤦 Hopefully this will save someone else from making the same mistake}

sagebind commented 2 years ago

Can someone running macOS Monterey verify that the latest main now compiles without any extra environment variables? #426 should have fixed the issue, but I don't have such a device to verify myself.

shepmaster commented 2 years ago

% git show --pretty=oneline --stat
0aea09c428b9bc2bcf46da0fc33959fe3f03c74a (HEAD -> main, origin/main, origin/HEAD) Refactor init to avoid link bugs on macOS Monterey (#426)
 src/lib.rs | 102 ++++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 68 insertions(+), 34 deletions(-)

% cargo build
    Updating crates.io index
   Compiling cc v1.0.72
   Compiling pkg-config v0.3.22
   Compiling libc v0.2.107
   Compiling curl v0.4.40 (/Users/shep/Projects/curl-rust)
   Compiling libz-sys v1.1.3
   Compiling curl-sys v0.4.50+curl-7.80.0 (/Users/shep/Projects/curl-rust/curl-sys)
   Compiling socket2 v0.4.2
    Finished dev [unoptimized + debuginfo] target(s) in 4.12s
curl-rust %

sagebind commented 2 years ago

Nice! We'll get this fix out to Crates.io soon.

sagebind commented 2 years ago

The fix for this is now available in curl 0.4.41.

alexcrichton / curl-rust