x86_64-musl: allow building dylibs with -crt-static #38075
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| @@ -237,7 +237,16 @@ pub fn default_output_for_target(sess: &Session) -> config::CrateType { | |||
| /// Checks if target supports crate_type as output | |||
| pub fn invalid_output_for_target(sess: &Session, | |||
| crate_type: config::CrateType) -> bool { | |||
| match (sess.target.target.options.dynamic_linking, | |||
| let requested_features = sess.opts.cg.target_feature.split(','); | |||
There was a problem hiding this comment.
This addition duplicates part of the add_configuration function (librustc_driver/target_feature.rs) and should be refactored into its own function.
There was a problem hiding this comment.
Perhaps this logic could get refactored to a method on Session? I had it there originally but ended up not using it, but I feel like we should avoid duplicating that here.
| @@ -62,7 +62,7 @@ pub fn opts() -> TargetOptions { | |||
| // MUSL support doesn't currently include dynamic linking, so there's no | |||
| // need for dylibs or rpath business. Additionally `-pie` is incompatible | |||
| // with `-static`, so we can't pass `-pie`. | |||
| base.dynamic_linking = false; | |||
| base.dynamic_linking = true; | |||
| base.has_rpath = false; | |||
| base.position_independent_executables = false; | |||
There was a problem hiding this comment.
Hm I wonder if this logic is insufficient now? Presumably we should pass -pie for dynamically linked musl executables?
Done both.
Forgot about this one. 😅 Refactored the crt_static logic as well. And tested with: Disclaimer: Still testing the latest commit. |
| @@ -237,7 +237,7 @@ pub fn default_output_for_target(sess: &Session) -> config::CrateType { | |||
| /// Checks if target supports crate_type as output | |||
| pub fn invalid_output_for_target(sess: &Session, | |||
| crate_type: config::CrateType) -> bool { | |||
| match (sess.target.target.options.dynamic_linking, | |||
| match (sess.target.target.options.dynamic_linking && !sess.target_is_crt_static(), | |||
There was a problem hiding this comment.
Hm I think this isn't quite accurate for MSVC where you can statically link msvcrt but still create DLLs (I believe)
There was a problem hiding this comment.
Following that logic, it should also be possible to build MUSL dylibs (.so libraries) that statically link to libc but link dynamically to other libraries I think (maybe not?). Do we want to support that as well?
If not then I can just add a !musl || here I think.
There was a problem hiding this comment.
I suppose if it's possible we should allow it. AFAIK it just gave a weird linker error when I tried with musl awhile back. That's probably b/c we were using wrong startup objects or something like that?
Ideally we'd avoid as many !musl conditions as possible...
| cmd.arg(root.join(obj)); | ||
| // When dynamically linking to MUSL we don't need to pass our startup | ||
| // objects as those will be provided by the MUSL toolchain | ||
| if !musl || crt_static { |
There was a problem hiding this comment.
Hm this is always something I wish we could sink into the target specs rather than have here... Does anything leap to mind for you as an easy way to do that?
There was a problem hiding this comment.
Does anything leap to mind for you as an easy way to do that?
pre_link_objects_crt_static and pre_link_objects_crt_dynamic? 😄
BTW, is pre_link_args "setable" from JSON files? This field feels like a Rust distribution implementation detail because the objects get prepended with the sysroot's absolute path. I doubt custom targets will be able to use this field lest they modify the sysroot ...
There was a problem hiding this comment.
Yeah I think all options are configurable from json files now (we round-trip through that format to read our own options IIRC).
I mean all the options in the custom target specs are already super specific... I guess I wouldn't really have a problem with those.
|
Ping, all! It's been a month since the last activity here. |
|
@japaric do you have further thoughts on my comments? I think I'd still like to remove some special casing here if we can. |
|
Closing due to inactivity, but feel free to reopen/resubmit! |
Tested with:
Note that
x86_64-unknown-linux-musl-gccis a "real" MUSL toolchain unlike themusl-gccwrapper that Ubuntu provides. The former provides a MUSL libc.so; the latter doesn't because it's just a wrapper aroundgccwith some extra flags (-static, etc.).Also this made me realize that when using
-crt-staticwith MUSL targets:We shouldn't be passing
crt*.oobjects; the MUSL toolchain will provide those.With that ^ change, we should be able to enable PIE for these dynamically linked MUSL binaries.
We probably can drop the
-Wl,-(and-Wl,-)as well.Also, with this change, Cargo still refuses to compile dylibs:
r? @alexcrichton