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

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

Closed
cormacrelf opened this issue Oct 27, 2021 · 10 comments · Fixed by #426
Closed

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

cormacrelf opened this issue Oct 27, 2021 · 10 comments · Fixed by #426

Comments

@cormacrelf
Copy link

cormacrelf commented Oct 27, 2021

Summary

Only on macOS. May occur on macOS earlier than 12 using Xcode 13, I'm not sure. Underlying issue is rust-lang/rust#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:

curl-rust/src/lib.rs

Lines 93 to 101 in df64ee4

/// An exported constructor function. On supported platforms, this will be
/// invoked automatically before the program's `main` is called.
#[cfg_attr(
any(target_os = "linux", target_os = "freebsd", target_os = "android"),
link_section = ".init_array"
)]
#[cfg_attr(target_os = "macos", link_section = "__DATA,__mod_init_func")]
#[cfg_attr(target_os = "windows", link_section = ".CRT$XCU")]
static INIT_CTOR: extern "C" fn() = init_inner;

I think the actual bug is due to the the workaround for rust-lang/rust#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
Copy link
Owner

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
Copy link
Collaborator

sagebind commented Oct 27, 2021

Also yes your assessments are correct that the weird indirection in this code is a workaround for rust-lang/rust#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 #333.

@cormacrelf
Copy link
Author

cormacrelf commented Oct 27, 2021

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link

shepmaster commented Nov 7, 2021

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 added a commit that referenced this issue Nov 17, 2021
Compilation currently fails for curl on macOS Monterey due to upstream rustc issue rust-lang/rust#90342. To make this problem hurt users less, we can work around this by avoiding the specific issue that this bug causes.

To avoid the rustc issue, we cannot directly reference any symbol that is configured to be in a constructor linker section, which we were previously doing intentionally to work around a different rustc issue rust-lang/rust#47384. We should be able to avoid both bugs by defining our constructor symbol as a public item in the root module, though not directly referencing it in other code. Since the root module is always used (`init` is called on-demand in key places in the code) it should not be removed by optimization.

Also add a quick unit test to make sure the constructor is still working for the platforms we have CI for.

Fixes #417.
alexcrichton pushed a commit that referenced this issue Nov 17, 2021
Compilation currently fails for curl on macOS Monterey due to upstream rustc issue rust-lang/rust#90342. To make this problem hurt users less, we can work around this by avoiding the specific issue that this bug causes.

To avoid the rustc issue, we cannot directly reference any symbol that is configured to be in a constructor linker section, which we were previously doing intentionally to work around a different rustc issue rust-lang/rust#47384. We should be able to avoid both bugs by defining our constructor symbol as a public item in the root module, though not directly referencing it in other code. Since the root module is always used (`init` is called on-demand in key places in the code) it should not be removed by optimization.

Also add a quick unit test to make sure the constructor is still working for the platforms we have CI for.

Fixes #417.
@sagebind
Copy link
Collaborator

sagebind commented Nov 17, 2021

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
Copy link

shepmaster commented Nov 17, 2021

% 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 %

image

@sagebind
Copy link
Collaborator

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

@sagebind
Copy link
Collaborator

The fix for this is now available in curl 0.4.41.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 14, 2021
…rt, r=Mark-Simulacrum

Revert setting a default for the MACOSX_DEPLOYMENT_TARGET env var for linking

This reverts commit b376f56, which is the main part of rust-lang#90499, because it turns out that this causes a good amount of breakage in crates relying on the old behavior. In particular `winit`, `coreaudio` and crates that depend on them are affected. Fixes rust-lang#91372.

Background:
Before rust-lang#90499 the behavior was the following: If MACOSX_DEPLOYMENT_TARGET is not set,  we pass the minimum supported OS version to LLVM but not to the linker. The linker default depends on the Xcode version and the version of the OS it is running on. That caused one known problem in libcurl with the most recent Xcode versions. rust-lang#90499 passed the minumum supported version (10.7 for Macos x86-64) to the linker instead. This has shown to be problematic because some crates such as winit, coreaudio implicitly expect a newer minimum OS version. The libcurl issue has been fixed independently (see alexcrichton/curl-rust#417), so a revert should not really be problematic.

Eventually we should probably mimic clang's behavior and fall back to the default of the currently configured Macos SDK for both the LLVM min os target version and MACOSX_DEPLOYMENT_TARGET for linking. That would entail looking at the `Version` property of the `SDKSettings.json` in the currently configured SDK.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 14, 2021
…rt, r=Mark-Simulacrum

Revert setting a default for the MACOSX_DEPLOYMENT_TARGET env var for linking

This reverts commit b376f56, which is the main part of rust-lang#90499, because it turns out that this causes a good amount of breakage in crates relying on the old behavior. In particular `winit`, `coreaudio` and crates that depend on them are affected. Fixes rust-lang#91372.

Background:
Before rust-lang#90499 the behavior was the following: If MACOSX_DEPLOYMENT_TARGET is not set,  we pass the minimum supported OS version to LLVM but not to the linker. The linker default depends on the Xcode version and the version of the OS it is running on. That caused one known problem in libcurl with the most recent Xcode versions. rust-lang#90499 passed the minumum supported version (10.7 for Macos x86-64) to the linker instead. This has shown to be problematic because some crates such as winit, coreaudio implicitly expect a newer minimum OS version. The libcurl issue has been fixed independently (see alexcrichton/curl-rust#417), so a revert should not really be problematic.

Eventually we should probably mimic clang's behavior and fall back to the default of the currently configured Macos SDK for both the LLVM min os target version and MACOSX_DEPLOYMENT_TARGET for linking. That would entail looking at the `Version` property of the `SDKSettings.json` in the currently configured SDK.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 14, 2021
…rt, r=Mark-Simulacrum

Revert setting a default for the MACOSX_DEPLOYMENT_TARGET env var for linking

This reverts commit b376f56, which is the main part of rust-lang#90499, because it turns out that this causes a good amount of breakage in crates relying on the old behavior. In particular `winit`, `coreaudio` and crates that depend on them are affected. Fixes rust-lang#91372.

Background:
Before rust-lang#90499 the behavior was the following: If MACOSX_DEPLOYMENT_TARGET is not set,  we pass the minimum supported OS version to LLVM but not to the linker. The linker default depends on the Xcode version and the version of the OS it is running on. That caused one known problem in libcurl with the most recent Xcode versions. rust-lang#90499 passed the minumum supported version (10.7 for Macos x86-64) to the linker instead. This has shown to be problematic because some crates such as winit, coreaudio implicitly expect a newer minimum OS version. The libcurl issue has been fixed independently (see alexcrichton/curl-rust#417), so a revert should not really be problematic.

Eventually we should probably mimic clang's behavior and fall back to the default of the currently configured Macos SDK for both the LLVM min os target version and MACOSX_DEPLOYMENT_TARGET for linking. That would entail looking at the `Version` property of the `SDKSettings.json` in the currently configured SDK.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 15, 2021
…rt, r=Mark-Simulacrum

Revert setting a default for the MACOSX_DEPLOYMENT_TARGET env var for linking

This reverts commit b376f56, which is the main part of rust-lang#90499, because it turns out that this causes a good amount of breakage in crates relying on the old behavior. In particular `winit`, `coreaudio` and crates that depend on them are affected. Fixes rust-lang#91372.

Background:
Before rust-lang#90499 the behavior was the following: If MACOSX_DEPLOYMENT_TARGET is not set,  we pass the minimum supported OS version to LLVM but not to the linker. The linker default depends on the Xcode version and the version of the OS it is running on. That caused one known problem in libcurl with the most recent Xcode versions. rust-lang#90499 passed the minumum supported version (10.7 for Macos x86-64) to the linker instead. This has shown to be problematic because some crates such as winit, coreaudio implicitly expect a newer minimum OS version. The libcurl issue has been fixed independently (see alexcrichton/curl-rust#417), so a revert should not really be problematic.

Eventually we should probably mimic clang's behavior and fall back to the default of the currently configured Macos SDK for both the LLVM min os target version and MACOSX_DEPLOYMENT_TARGET for linking. That would entail looking at the `Version` property of the `SDKSettings.json` in the currently configured SDK.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants