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

Enable windows-gnu target to link to libgcc dynamically. #90020

Closed
wants to merge 6 commits into from

Conversation

Berrysoft
Copy link
Contributor

@Berrysoft Berrysoft commented Oct 18, 2021

Closes #89919.

It seems that the change is trival, and I've tested on my own computer with success, with both crt-static is enabled or disabled.

The libunwind part is not tested, but it should work in theory, according to the patch of MSYS2.

There's one more problem needs to talk about: should we enable crt-static as default when using GNU toolchain on Windows?

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2021
@Mark-Simulacrum
Copy link
Member

r? @petrochenkov maybe?

@petrochenkov
Copy link
Contributor

cc @Amanieu @mati865
What do you think? Does it make sense to add support for +crt-static mode for windows-gnu targets?

If it does make sense then the changes here look entirely reasonable to me.
(Although not sufficient, other libraries linked from windows_gnu_base.rs also need to be moved to #[link] attributes in libunwind/libstd and linked statically if possible.)

@petrochenkov petrochenkov added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 19, 2021
@Amanieu
Copy link
Member

Amanieu commented Oct 19, 2021

I think this breaks the case where -C prefer-dynamic is used since we must always use the shared libgcc_s.so in this case.

@Berrysoft
Copy link
Contributor Author

Berrysoft commented Oct 20, 2021

@Amanieu I don't think so. -C prefer-dynamic is incompatible with -C target-feature=+crt-static, because the libstd-***.dll is always dynamically linked with C runtime. In addition, if there's other dylibs in the dependencies, should we statically link the C runtime? Of course not, for unwind safety. Therefore, you cannot specify +crt-static together with prefer-dynamic.

@petrochenkov
Copy link
Contributor

you cannot specify +crt-static together with prefer-dynamic

Technically you can, the compiler won't report an error, it just doesn't make sense on most targets.
I'm not sure what result this combination will give in practice if used, need to check the code.

@Berrysoft
Copy link
Contributor Author

Technically you can, the compiler won't report an error...

No. The compiler will complain about not finding _Unwind_*** symbols when linking.

it just doesn't make sense on most targets.

Yes, at least on this target, it doesn't make sense at all. MinGW won't statically link with msvcrt because of license restrictions, and those who want to use crt-static must be those who want to link with libgcc statically. Even though if the compiler doesn't report any error, they will find that their output binary will still depend on libgcc dynamically because of libstd. They will then understand that their attempt is wrong:)

@mati865
Copy link
Contributor

mati865 commented Oct 20, 2021

MinGW is probably never going to support static CRT (unless MS opens UCRT sources) so +crt-static here is misleading as it'd still link CRT dynamically while other targets with +crt-static do what the feature says and link it statically.

Now this makes linking to shared libgcc as the default which is going make executables depend on libgcc and winpthreads DLLs. There is Rust test which is going to fail in such case.

@Berrysoft
Copy link
Contributor Author

If we treat the unwind library is a part of C runtime, crt-static still means something: link with unwind lib statically. It is a bit misleading, but those who use MinGW should not expect that msvcrt or ucrtbase will be linked dynamically. It is the choice of the C toolchain, and we shouldn't be responsible for that. Anyway, the current behavior will statically link to mingwex, which is a static library contains some libc extensions. So either enable or disable crt-static are both misleading, precisely. Again, it is the choice of the toolchain, but not the fault of Rust maintainers.

Therefore, just crt-static should be OK. It will make the code dirty and ugly (because all other targets are using crt-static) if another specified features are added, for example, -C target-feature=+pc-windows-gnu-c-unwind-static. (For non-misleading wording, we need to exactly specify pc-windows-gnu as it only influences *-pc-windows-gnu targets, not *-uwp-windows-* or *-linux-gnu or *-windows-msvc.)

Even though if there's someday when MS opens their UCRT source, and MinGW could be able to link with UCRT statically, I don't think we need to add another feature to precisely control which and which not to be statically linked. Just static all it can, or dynamic all it can.

And if we decide to make crt-static as default feature (like what musl target does), the default compiler behavior will not change, and the failed test should pass again.

I just mean to give developers a choice to dynamically link to libgcc_s or other C runtime libraries while still statically link to rust crates. They have the choice when using msvc toolchain, or targeting linux/macos. windows-gnu target shouldn't be the tricky one.

@mati865
Copy link
Contributor

mati865 commented Oct 20, 2021

And if we decide to make crt-static as default feature (like what musl target does), the default compiler behavior will not change, and the failed test should pass again.

This will break exceptions across DLLs on 32-bit target which is also has a test IIRC.

Don't get me wrong, I would love to see this working with MinGW targets but I have no idea how to do it correctly from the beginning. I won't oppose merging this if maintainers agree this is the way to go.

To keep current behaviour unchanged this might require adding additional feature like proposed in #72241 (comment) or some kind of heuristic whether to make default +crt-static (when building executables) or -crt-static (for other cases).

@Berrysoft
Copy link
Contributor Author

This will break exceptions across DLLs on 32-bit target which is also has a test IIRC.

Which test? Do you mean it will break because it enables prefer-dynamic? We should specify -crt-static in that case. As I commented earlier, crt-static and prefer-dynamic are incompatible features.

@Berrysoft
Copy link
Contributor Author

As for #72241, I think this pull request solves that: if system-llvm-libunwind feature is enabled, libunwind will be used both for statically and dynamically linking instead of libgcc.

@Berrysoft
Copy link
Contributor Author

To keep current behaviour unchanged...

OK, I have to admit that I'm changing the behavior now. Anyway the compiler behavior will be changed, no matter adding crt-static support, or other features support. I'd like to use crt-static, because it partially makes sense, and requires minimal change to the code base. I just don't like a new feature propersal, because it may require an RFC, or it takes a lot of time talking about the new behavior, or the new names, or something else. People will waste a lot of time on that, but the problem - we need the choice to dynamically link to unwind lib - will still remain unsolved.

Yes, a new feature may be less misleading, and maybe good to those who have OCD. But it simply wastes a lot of time, and finally we will find that, we create a new target feature specified for pc-windows-gnu target, with crt-static not supported, but they are actually complementation. We will then find some tutorials writting, (only an example)

If you want to dynamically link to the C runtime and the unwind runtime, usually you need not to do anything, or -C target-feature=-crt-static sometimes, but you need -C target-feature=-pc-windows-gnu-c-unwind-static when targeting *-pc-windows-gnu. That's because the MinGW toolchain won't statically link to msvcrt or ucrt, and the Rust maintainers think that crt-static is misleading, so they add a new feature for that specific case.

We won't get any more benefit to propersal a new feature, but only much more complexity.

@mati865
Copy link
Contributor

mati865 commented Oct 21, 2021

As for #72241, I think this pull request solves that: if system-llvm-libunwind feature is enabled, libunwind will be used both for statically and dynamically linking instead of libgcc.

I meant the other part of that comment: rustc_detect_dynamic. That issue itself needs another target triple anyway.

@mati865
Copy link
Contributor

mati865 commented Oct 21, 2021

This will break exceptions across DLLs on 32-bit target which is also has a test IIRC.

Which test? Do you mean it will break because it enables prefer-dynamic? We should specify -crt-static in that case. As I commented earlier, crt-static and prefer-dynamic are incompatible features.

I don't remember which one, you would have to run them.
It would fail for two reasons: you are not linking pthread for static case and broken exceptions across DLLs. I have no idea if it used prefer-dynamic or not.

@Berrysoft
Copy link
Contributor Author

That issue itself needs another target triple anyway.

No, it needn't. system-llvm-libunwind is already a feature to control whether libgcc or libunwind should be used. It doesn't need another new target to distinguish gcc toolchain with llvm toolchain.

It would fail for two reasons: you are not linking pthread for static case and broken exceptions across DLLs.

Oh, OK. I finally know why there was -l:libpthread.a in the static link args. I'll add them.

For the exceptions across DLLs, I'll investigate further.

@Berrysoft
Copy link
Contributor Author

@mati865 I ran the test against target i686-pc-windows-gnu and stage0 compiler, but didn't failed any test. Maybe I should test for stage1 compiler? Or have you recalled the test name?

@mati865
Copy link
Contributor

mati865 commented Oct 21, 2021

I'm too busy to look into tests now but I'm fairly sure you need stage1 compiler and std.

@petrochenkov
Copy link
Contributor

@mati865

MinGW is probably never going to support static CRT

The meaning of +crt-static on Windows is kind of blurry, because dynamic linking is done by OS and can be done even if libc is linked statically, e.g. with MSVC we still link some winapi libraries dynamically in +crt-static mode.
So it's pretty easy to interpret +crt-static as "link statically as much C runtime as possible" on this platform.

This is different from ELF-based targets, where we basically remove dynamic linker from the story when linking libc statically, and thus require linking everything statically in this mode.

@Berrysoft
Copy link
Contributor Author

I've tested on Linux that when -C prefer-dynamic and -C target-feature=+crt-static are both specified, the compiler will choose to statically link to C runtime when building executable, and will report error when building dylibs. Therefore, the priority of crt-static is higher than prefer-dynamic.

@mati865
Copy link
Contributor

mati865 commented Oct 21, 2021

That issue itself needs another target triple anyway.

No, it needn't. system-llvm-libunwind is already a feature to control whether libgcc or libunwind should be used. It doesn't need another new target to distinguish gcc toolchain with llvm toolchain.

It needs separate triple for other reasons like using native TLS instead of emutls and UCRT instead of MSVCRT. Neither of them works properly when mixed with GCC made binaries.

windows-binary-no-external-deps is fixed with +crt-static

This still is behavioural change, previously executables had no runtime dependencies on unwinding library out of the box. With this PR it's no longer the case and user has to use +crt-static feature to keep portable binaries. I'm not the one to judge whether it's acceptable.
If we want to keep previous behaviour unchanged I think adding rustc_detect_dynamic from #72241 (comment) is the only way.

@Berrysoft
Copy link
Contributor Author

Neither of them works properly when mixed with GCC made binaries.

OK, but it's another question. Now they belong to the same target (and no one actually trying to mix different toolchains), and I think we need new issues to solve that.

This still is behavioural change...

Yes, it needs more changes and more fixes to keep the behavior. In practical, if +crt-static is enabled, I need to fix for hundreds of tests, but now only two. Anyway, if we finally agree to make +crt-static default, I'll try to do so.

@mati865
Copy link
Contributor

mati865 commented Oct 22, 2021

This still is behavioural change...\n\nYes, it needs more changes and more fixes to keep the behavior. In practical, if +crt-static is enabled, I need to fix for hundreds of tests, but now only two. Anyway, if we finally agree to make +crt-static default, I'll try to do so.

As long as you have to fix the tests it means this target behaviour is changed.
On stable it works somewhat like that:

  • executable == +crt-static
  • library == -crt-static

To keep it we would need new feature that detects which option should be default. User would still use +/-crt-static to change the default.

@Berrysoft
Copy link
Contributor Author

OK, so I'm now asking that if it is acceptable that I'm changing the behavior.

To my opinion, it is much more natural.

@bors
Copy link
Contributor

bors commented Oct 30, 2021

☔ The latest upstream changes (presumably #90205) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor

@Berrysoft
Could you check what happens if you set +crt-static (explicitly or through a default) for an executable, and link that executable to a dylib (the standard library, for example) built with -crt-static.

In theory if we are linking to any dylib, then we should switch the executable to -crt-static too (that's basically what any_dynamic_crate does now), but if executables are +crt-static by default, then such a switch will need to be performed manually by the user.

@petrochenkov
Copy link
Contributor

I'm basically ok with switching the windows-gnu targets to +crt-static by default, given that it's what we are de-facto doing in typical cases (no dynamic crate deps -> libgcc and libpthread are linked statically).

The only case that worries me is the previous comment #90020 (comment) - an executable with dynamic crate deps (e.g. dynamic libstd).
In that case either we should switch to -crt-static automatically (at least by default), or require a user to do it explicitly.

Perhaps we just need more fine-grained options related to crt-static in target specs instead of two bools for executables and dynamic libraries.
This is some generalization task over all our supported targets rather than something windows-gnu-specific, I need to think about it a bit.

@petrochenkov petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2021
@petrochenkov
Copy link
Contributor

I went through major targets supporting dynamic linking and they all seem to expose relatively similar behavior with regards to linking CRT (libc or analogues) dynamically or statically.
Here's some summary.

An executable and all its dynamic dependencies must share a common CRT for things like cross-binary exceptions and cross-binary allocations/deallocations on platform like Windows (which don't do most of OS interactions through CRT), and maybe for other shared resource management as well on platforms like Linux (which do most of OS interactions through CRT).
Next I'll consider different kinds of end binaries and their relationships with dynamic and static CRT.

  • Executable without other dynamic dependencies - static libc is possible and not exotic.
  • Executable with other dynamic dependencies - depends on the dependencies:
    • The dependencies don't produce cross-binary exceptions or allocations (on Windows), and don't link to CRT (on Linux) - static libc is possible and not exotic.
      • Example: the executable only depends on WinAPI DLLs.
    • The dependencies may produce cross-binary exceptions or allocations (on Windows), or link to CRT (on Linux) - static libc is possible, but exotic and shouldn't be done without an explicit request.
      • Example: the executable depends on Rust standard library as a dylib.
  • Dynamic library with or without other dynamic dependencies - depends on itself and dependencies:
    • Neither the library itself nor its dependencies may produce cross-binary exceptions or allocations (on Windows), and the dependencies don't link to CRT (on Linux) - static libc is possible and not exotic.
      • Example: the library is a cdylib with C interface and only depends on WinAPI DLLs.
      • Example: the library is a cdylib which is a plugin for DynamoRIO binary instrumentation framework. It's an ELF, but it will be loaded by libdynamorio.so instead of a standard dynamic linker.
    • The library itself or its dependencies may produce cross-binary exceptions or allocations (on Windows), or the dependencies link to CRT (on Linux) - static libc is possible, but exotic and shouldn't be done without an explicit request.
      • Example: the library is a Rust dylib, or depends on Rust standard library as a dylib.
  • Linking CRT dynamically is always possible and not exotic in all cases above.

Whether a dynamic library may require managing shared resources is ultimately a user's choice.
We can easily take "yes, it requires managing shared resources, and thus requires shared CRT" as a default for both Rust dylibs and native dylibs, and add a linking modifier for opting out, -needs-shareed-crt or something.

Side note: for targets doing dynamic linking in userspace via ELF interpreter field (Linux), we can remove the interpreter field and speedup their launch if there's no dynamic dependencies at all (CRT or others).

So the question is what "with(out) an explicit request" above means.
In general, I'd say -Ctarget-feature=-crt-static is explicit enough because dynamic linking is never exotic, but -Ctarget-feature=+crt-static is not explicit enough.

Considering the above, I think my suggested solution is:

  • Keep the crt_static_respected target spec option, target with crt_static_respected=false behave in some hard-coded way, targets with crt_static_respected=true can be controlled using command-line options.
  • Keep the -Ctarget-feature=+/-crt-static option and the crt_static_default target spec option. Take crt-static as value(-Ctarget-feature=+/-crt-static).unwrap_or(crt_static_default).
  • If crt-static is false, then we link CRT dynamically.
  • If crt-static is true, then we link CRT statically, if the situation is not exotic, otherwise we link CRT dynamically and maybe emit a warning telling the reason.
  • The situation is exotic when
    • This is an executable and it has dynamic dependencies requiring managing shared resources.
    • This is a dynamic library and it has dynamic dependencies requiring managing shared resources.
    • This is a dynamic library and it requires managing shared resources.
    • For dependencies "requires managing shared resources" means that
      • It's a Rust dylib or proc macro crate.
      • It's a native dylib not marked with the -needs-shareed-crt link modifier.
    • For the current dynamic library "requires managing shared resources" means that
      • It's a Rust dylib or proc macro crate
        • Cdylibs are not considered requiring shared resources by default, if your cdylib requires them, just use -Ctarget-feature=-crt-static.
          • Or maybe we can have a target spec option controling whether cdylibs (the current one and native dependencies) require shared resources by default, e.g. it would be false on Windows and true on Linux.
  • Add a new option -Cforce-crt-static.
    • It removes the "non-exotic" requirement from the conditions above and makes crt-static enable static CRT linking even in exotic cases.
    • Example: if +needs-shareed-crt is the default on Windows, then -Cforce-crt-static may be required to build statically while depending on WinAPI DLLs, until the winapi crate adopts -needs-shared-crt modifiers.
  • Remove the crt_static_allows_dylibs target spec option, it's no longer necessary with our scheme.
  • Issue: evaluating effective crt-static requires determining whether our situation is exotic, which requires evaluating some link-time cfgs, which requires evaluating effective crt-static.
    • Fortnately, it's boolean so there are only two values and we can likely try them both speculatively.

@petrochenkov
Copy link
Contributor

What this means for windows gnu - it'll get crt_static_respected=true, crt_static_default=true, and probably cdylibs_need_shared_crt_by_default=false as well because it's still Windows, where cross-binary exceptions and allocations/deallocations are discouraged, and other shared resources are managed through OS/WinAPI instead of CRT.

@petrochenkov
Copy link
Contributor

@Berrysoft
To clarify, I don't suggest you to implement the design above, it'll likely need an MCP anyway.
Not sure what to do with this PR short term though.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Dec 3, 2021
@Berrysoft
Copy link
Contributor Author

@petrochenkov
Hmm, you've given such a detailed suggestion, and I'm sorry about responsing late.

Maybe you're right, and we need to rethink about all these problems.

@mati865
Copy link
Contributor

mati865 commented Dec 3, 2021

  • Example: if +needs-shareed-crt is the default on Windows, then -Cforce-crt-static may be required to build statically while depending on WinAPI DLLs

What about MSVC where CRT can be statically linked?

@petrochenkov
Copy link
Contributor

@mati865
That line is mostly about MSVC actually.
Majority of the Windows mentions and examples above have MSVC in mind.

@mati865
Copy link
Contributor

mati865 commented Dec 3, 2021

Well I find it confusing because MSVC is the only Windows target right now that allows statically linking CRT into executables. So judging by the name I'd expect if I pass -Cforce-crt-static then CRT would be really static.

@petrochenkov
Copy link
Contributor

@mati865
By CRT for windows-gnu I mean the same thing as Berrysoft in this PR - libgcc and libpthread (but not e.g. msvcrt or kernel32), #90020 (comment).
I.e. the components for which the platform gives this choice at all.

The design in #90020 (comment) still works in this case - shared libgcc/libpthread are indeed necessary for managing shared resources (cross-binary exceptions).

@joelpalmer joelpalmer added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2022
@Dylan-DPC
Copy link
Member

@Berrysoft any updates on this?

@JohnCSimon
Copy link
Member

@Berrysoft
Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this. Thanks for your contribution.

@rustbot label: +S-inactive

@JohnCSimon JohnCSimon closed this Mar 20, 2022
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider dynamically link to libgcc_s when targeting windows-gnu