r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

For no_std, users are left to manually pass the default lib to the linker, otherwise they won't even have so much as memcpy.

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).

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.

Consider on Linux, this compiles with rustc -C panic=abort:

#![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.

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).

Using /defaultlib puts it after other import libraries on the commandline so if all needed symbols are already satisfied then it won't be include. And /nodefaultlib can be used to remove it from consideration completely.

Consider on Linux, this compiles with rustc -C panic=abort:

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 core::ffi::CStr::from_bytes_with_nul(b"foo\0").unwrap();, the resulting dylib will reference bcmp, memcpy, memset and rust_eh_personality. It links successfully, but only because the linker expects that an external dynamic library will resolve these symbols. If I try to LD_PRELOAD the resulting dylib, it will complain about being unable to find rust_eh_personality. And linking an executable against it would likely result in the same error.

I can compile that with rustc nightly-x86_64-pc-windows-gnu nostd.rs -C panic=abort by just providing the rust_eh_personality lang item. msvcrt.dll is linked automatically.

I tried on Linux and it seemed to work:

$ cat nostd.rs
#![no_std]
#![crate_type="cdylib"]

#[no_mangle]
pub unsafe extern "C" fn do_stuff(ptr: *const u8, len: usize, out: *mut u8) {
    let a = core::slice::from_raw_parts(ptr, len);
    let b = core::slice::from_raw_parts_mut(out, len);
    b.copy_from_slice(a);
}

#[panic_handler]
fn panic(_info: &core::panic::PanicInfo) -> ! {
    loop {}
}
$ rustc nostd.rs -C panic=abort -C opt-level=3
$ cat exe.rs
fn main() {
    let a = [1u8; 10];
    let mut b = [0u8; 10];
    unsafe { do_stuff(a.as_ptr(), a.len(), b.as_mut_ptr()) }
    println!("{:?}", b);
}

#[link(name = "nostd")]
extern "C" {
    fn do_stuff(ptr: *const u8, len: usize, out: *mut u8);
}

$ rustc exe.rs -L ./
$ LD_LIBRARY_PATH=./ ./exe
[1, 1, 1, 1, 1, 1, 1, 1, 1, 1]

The produced libnostd.so seemed to contain glibc specific stuff and not just be a pure Linux binary.

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

Hmmm.
https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/spec/base/windows_msvc.rs#L20-L29

This is a particular problem when you want to use e.g. the debug CRT libraries in their place or integrate with C/C

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.)

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.

Hmmm. https://github.com/rust-lang/rust/blob/master/compiler/rustc_target/src/spec/base/windows_msvc.rs#L20-L29

The linker itself doesn't add any libraries or object files. However, a bare /nodefaultlib disables all uses of /defaultlib which can have non-obvious effects, E.g. when you link msvcrt.lib it includes an embedded .directve, which also adds the ucrt and vcruntime libraries as default libraries. So if you do a blanket /nodefaultlib those other libraries will be removed even if you add msvcrt.lib normally. See also C runtime (CRT) and C standard library (STL) .lib files,

This is a particular problem when you want to use e.g. the debug CRT libraries in their place or integrate with C/C

This looks like a job for link time cfg - rust-lang/libc#1433 (comment)

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 no_std on non-embedded targets is sorely needed to help guide making these sorts of decisions, but in the meantime this should be ok. Using /defautlib means the library is easily overridden when necessary,

@rustbot ready

⌛ Testing commit 87e1dd0 with merge fdef32a...

The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)

💔 Test failed - checks-actions

Failed in miri on i686-pc-windows-gnu. However this PR only affects msvc, aside from changing some libc imports to be core::ffi ones so it's unlikely to be caused by this PR. @rust-lang/miri

  error: memory leaked: alloc27197638 (Rust heap, size: 8, align: 4), allocated here:
  Error:     --> /checkout/library/alloc/src/alloc.rs:100:9
       |
  100  |         __rust_alloc(layout.size(), layout.align())
       |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
       |
       = note: BACKTRACE:
       = note: inside `realstd::alloc::alloc` at /checkout/library/alloc/src/alloc.rs:100:9: 100:52
       = note: inside `realstd::alloc::Global::alloc_impl` at /checkout/library/alloc/src/alloc.rs:183:73: 183:86
       = note: inside `<realstd::alloc::Global as core::alloc::Allocator>::allocate` at /checkout/library/alloc/src/alloc.rs:243:9: 243:39
       = note: inside `alloc_crate::alloc::exchange_malloc` at /checkout/library/alloc/src/alloc.rs:332:11: 332:34
       = note: inside `realstd::boxed::Box::<realstd::sys::thread_local::os_local::Value<u8>>::new` at /checkout/library/alloc/src/boxed.rs:218:9: 218:20
       = note: inside `realstd::thread::local_impl::Key::<u8>::try_initialize::<{closure@/checkout/library/std/src/sys/thread_local/os_local.rs:28:27: 28:34}>` at /checkout/library/std/src/sys/thread_local/os_local.rs:145:37: 145:94
       = note: inside `realstd::thread::local_impl::Key::<u8>::get::<{closure@/checkout/library/std/src/sys/thread_local/os_local.rs:28:27: 28:34}>` at /checkout/library/std/src/sys/thread_local/os_local.rs:127:18: 127:43
  note: inside `sync::reentrant_lock::current_thread_unique_ptr::X::__getit`
      --> /checkout/library/std/src/sys/thread_local/os_local.rs:28:17
       |
  11   |   pub macro thread_local_inner {
       |   ----------------------------
       |   |
       |   in this expansion of `$crate::thread::local_impl::thread_local_inner!` (#2)
       |   in this expansion of `$crate::thread::local_impl::thread_local_inner!` (#3)
  ...
  28   | /                 __KEY.get(move || {
  29   | |                     if let $crate::option::Option::Some(init) = _init {
  30   | |                         if let $crate::option::Option::Some(value) = init.take() {
  187 | |     () => {};
  ...   |
  195 | |         $crate::thread::local_impl::thread_local_inner!($(#[$attr])* $vis $name, $t, const $init);
      | |         ----------------------------------------------------------------------------------------- in this macro invocation (#2)
  ...   |
  207 | |     );
  208 | | }
      | |_- in this expansion of `thread_local!` (#1)
      = note: inside `realstd::thread::LocalKey::<u8>::try_with::<{closure@library/std/src/sync/reentrant_lock.rs:319:12: 319:15}, usize>` at /checkout/library/std/src/thread/local.rs:285:37: 285:55
      = note: inside `realstd::thread::LocalKey::<u8>::with::<{closure@library/std/src/sync/reentrant_lock.rs:319:12: 319:15}, usize>` at /checkout/library/std/src/thread/local.rs:262:9: 262:25
  note: inside `sync::reentrant_lock::current_thread_unique_ptr`
     --> library/std/src/sync/reentrant_lock.rs:319:5
      |
  319 |     X.with(|x| <*const _>::addr(x))
      |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  note: inside `sync::reentrant_lock::ReentrantLock::<()>::try_lock`
     --> library/std/src/sync/reentrant_lock.rs:226:27
      |
  226 |         let this_thread = current_thread_unique_ptr();
      |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  note: inside closure
     --> library/std/src/sync/reentrant_lock/tests.rs:47:20
      |
  47  |         let lock = l2.try_lock();
      |                    ^^^^^^^^^^^^^

@bors retry miri i686-pc-windows-gnu std

⌛ Testing commit 87e1dd0 with merge a8a88fe...

That CI failure is #123583

Oh right. I knew that.

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing a8a88fe to master...

Finished benchmarking commit (a8a88fe): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Cycles

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.009s -> 677.239s (0.03%)
Artifact size: 315.96 MiB -> 315.99 MiB (0.01%)

I think this breaks us in Chromium. We pass -Clink-arg=libcmt.lib to link to libcmt (due to #39016 I'm told), which means rustc doesn't know about libcmt. With this, it also links to msvcrt, and things fall apart.

(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 /nodefaultlib:msvcrt. Though hm you're not using -C target-feature= crt-static for the static runtime?

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 /nodefaultlib:msvcrt, you're saying we should add -Clink-arg=/nodefaultlib:msvcrt and then that will be later on the link line and it'll undo the addition by this crate, yes?

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 :)

For /nodefaultlib:msvcrt, you're saying we should add -Clink-arg=/nodefaultlib:msvcrt and then that will be later on the link line and it'll undo the addition by this crate, yes?

Yes.

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.

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 -Clink-arg=libcmtd.lib and nothing else. Since we don't pass an explicit -Ctarget-feature= crt-static, rustc thinks we want the dynamic runtime and adds msvcrt.lib in addition to the libcmtd.lib we add, and that causes problems.

My PR adds -Clink-arg=/nodefaultlib:libcmt.lib -Clink-arg=/nodefaultlib:msvcrt.lib to basically revert this change here through flags in our build system. That's what you recommended above.

Alternatively, we could pass both -Clink-arg=libcmtd.lib and -Ctarget-feature= crt-staticand hope that every symbol exported by libcmt.lib is also exported by libcmtd.lib, so that the first one is never used.

Or am I missing something and we should do something altogether?

The intended use would be something like:

msvcrt.lib: no special arguments required
msvcrtd.lib: -Clink-arg=/nodefaultlib:msvcrt -lmsvcrtd
libcmt.lib: -Ctarget-feature= crt-static
libcmtd.lib: -Ctarget-feature= crt-static -Clink-arg=/nodefaultlib:libcmt -llibcmtd

That said the fact that just using, e.g. -l libcmt alone doesn't work suggests either a) your build has something expecting the dynamic crt when it shouldn't or b) it doesn't and this should be reverted.

I think what's missing is that we were also passing -Zlink-directives=false to the libc crate, which used to be what was pulling in the CRT. Now it's in core instead, so we would need to move the -Zlink-directives=false there to maintain equivalency. But the /nodefaultlib looks better to me now. And we can drop the -Zlink-directives=false.

https://chromium-review.googlesource.com/c/chromium/src/ /5477889 should bring this fully up to speed with the new core-linking strategy.