-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Link MSVC default lib in core #122268
Link MSVC default lib in core #122268
Conversation
rustbot has assigned @Mark-Simulacrum. Use r? to explicitly pick a reviewer |
This comment has been minimized.
This comment has been minimized.
This is the case on all unix targets too. The status quo allows the user to choose between linking libc themself or doing something like compiling compiler-builtins with the mem feature for memcpy and friends and implementing their own interfacing with the OS. This PR will force linking against msvcrt on Windows even when then user wants to avoid all external dependencies other than kernel32.dll (or even forgo that one if they only want to support a single Windows build). |
Consider on Linux, this compiles with #![no_std]
#![crate_type="cdylib"]
#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
loop {}
} Whereas on Windows it gives a link error due to missing symbol. This gets worse once you start actually doing anything useful yet it'll continue to work when compiled on Linux.
Using |
Only by accident as all functions in libcore that depend on symbols like memcpy are omitted by the linker. If I add something as simple as |
I can compile that with |
I tried on Linux and it seemed to work:
The produced |
I've updated the OP to remove the contentious parts and stick with facts I'm confident on. But see the discussion of the original above. |
This looks like a job for link time cfg - rust-lang/libc#1433 (comment) |
rust-lang/libc#3178 is doing something that I'd expect, but I haven't seen it before. (It's generally unfortunate that libc is not a subtree in the rust-lang/rust repo, given that it's a pretty fundamental part of the standard library and is often modified together with it to add new targets, and for other reasons.) |
The linker itself doesn't add any libraries or object files. However, a bare
While having a "debug" cfg would be great for the specific problem of debug libraries, I don't think it's quite sufficient in general. We can't hard code every possible CRT that may exist (e.g. custom ones for special environments) so it'd be great for std to let people just provide whatever one they want. Using the same way of doing things as C/C++ is also nice as it makes integration easier without needing to do special Rust things or hack around it. |
This was discussed in the libs meeting. It was decided to accept this. It was felt that a bigger discussion on use cases for @rustbot ready |
💔 Test failed - checks-actions |
Failed in miri on i686-pc-windows-gnu. However this PR only affects msvc, aside from changing some i686-pc-windows-gnu miri
@bors retry miri i686-pc-windows-gnu std |
That CI failure is #123583 |
Oh right. I knew that. |
☀️ Test successful - checks-actions |
Finished benchmarking commit (a8a88fe): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 677.009s -> 677.239s (0.03%) |
I think this breaks us in Chromium. We pass (Details: https://issues.chromium.org/issues/336318586#comment8) The PR mentions /nodefaultlib. But that disables all the other default libs too (?). Or did you mean passing that to rustc, not to link.exe? |
This can of course be reverted but I meant with an argument such as |
Currently we don't. From what I understand, that's because that will always get you the release version of the CRT, and in debug builds we want to link against the debug version. This here also always adds the release libraries. Maybe this here can be delayed until there are toggles for both dynamic/static and release/debug? |
For If that works, that's a workaround that should work for us. Seems a bit weird having to pass that since we didn't ask for it in the first place, but eh, sure. Sorting out #39016 before auto-linking libraries here might still be nicer – this broke us, it might break others :) |
Yes.
I'm not sure why msvcrt is still being pulled in when you specify libcmt (I'll try to investigate) but the debug variant should definitely override their non-debug counterpart. |
https://chromium-review.googlesource.com/c/chromium/src/+/5476668 is my workaround. Does the PR description over there make sense? What we currently do is pass My PR adds Alternatively, we could pass both Or am I missing something and we should do something altogether? |
The intended use would be something like: msvcrt.lib: no special arguments required That said the fact that just using, e.g. |
I think what's missing is that we were also passing https://chromium-review.googlesource.com/c/chromium/src/+/5477889 should bring this fully up to speed with the new core-linking strategy. |
This more strictly follows the recipe in rust-lang/rust#122268 (comment) And we remove the -Zlink-directives=false for building the libc crate, as that crate no longer provides the CRT link directives. The link directives are now in core, and are done through /defaultlib so that we can remove it in the command line with /nodefaultlib. This allows us to control linking entirely through our GN linking rules, and not during stdlib compilation, giving us support for prebuilt Rust stdlib as well. R=thakis@chromium.org Bug: 5476668 Change-Id: I1466c4f721a9d8cc9ac3465c2e15f866b731a738 Cq-Include-Trybots: luci.chromium.try:android-rust-arm32-rel,android-rust-arm64-rel,android-rust-arm64-dbg,linux-rust-x64-dbg,linux-rust-x64-rel,mac-rust-x64-dbg,win-rust-x64-dbg,win-rust-x64-rel Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5477889 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/main@{#1292464}
This more strictly follows the recipe in rust-lang/rust#122268 (comment) And we remove the -Zlink-directives=false for building the libc crate, as that crate no longer provides the CRT link directives. The link directives are now in core, and are done through /defaultlib so that we can remove it in the command line with /nodefaultlib. This allows us to control linking entirely through our GN linking rules, and not during stdlib compilation, giving us support for prebuilt Rust stdlib as well. R=thakis@chromium.org Bug: 5476668 Change-Id: I1466c4f721a9d8cc9ac3465c2e15f866b731a738 Cq-Include-Trybots: luci.chromium.try:android-rust-arm32-rel,android-rust-arm64-rel,android-rust-arm64-dbg,linux-rust-x64-dbg,linux-rust-x64-rel,mac-rust-x64-dbg,win-rust-x64-dbg,win-rust-x64-rel Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5477889 Reviewed-by: Nico Weber <thakis@chromium.org> Commit-Queue: danakj <danakj@chromium.org> Cr-Commit-Position: refs/heads/main@{#1292464} NOKEYCHECK=True GitOrigin-RevId: 714e31ddaca456ecf74d7f6f309447d6095f7ed4
The Problem
On Windows MSVC, Rust invokes the linker directly. This means only the objects and libraries Rust explicitly passes to the linker are used. In short, this is equivalent to passing
-nodefaultlibs
,-nostartfiles
, etc for gnu compilers.To compensate for this the libc crate links to the necessary libraries. The libc crate is then linked from std, thus when you use std you get the defaults back.or integrate with C/C++.
However, this has a few problems:
no_std
, users are left to manually pass the default lib to the linkerstd
has the opposite problem, using/nodefaultlib
doesn't work as expected because Rust treats them as normal libs. This is a particular problem when you want to use e.g. the debug CRT libraries in their place or integrate with C/C++..The solution
This PR fixes this in two ways:
core
/defaultlib
. This allows users to override it in the normal way (i.e. with/nodefaultlib
).This is more or less equivalent to what the MSVC C compiler does. You can see what this looks like in my second commit, which I'll reproduce here for convenience:
Alternatives
unwind
andstd
but notcore
This bares some discussion so I've t-libs nominated it.