Closed RalfJung closed 5 months ago
Or maybe this is caused by Miri not properly running the on-thread-exit hook on Windows? The support for that is a bit fragile as Miri does not support these magic linker sections we rely on. Instead Miri hard-codes the path to this static and calls it. But with the test crate we may have that static twice (once in std and once in the test crate) and only one of them gets called...
However I would expect that this logic means that the thread_local impl from the real std is used, so the one from the test crate should not matter.
Maybe we should stop using this hack and implement support for this magic linker section instead. I just don't know if there's a good way to ask the compiler for "all static
in a particular link_section across all crates".
I tried to figure out how TLS is currently implemented on Windows, but I got lost in the dark forest that is std::sys
.^^ I can't figure out where this call (imp::create
) would go on Windows; nothing in the Windows folder declares a create
function.
EDIT: Oh, looks like sys_common::thread_local_key
is Unix-only. I got fooled by the "common" in the name and it not being in pal
.
The Windows specific bits happen here: https://github.com/rust-lang/rust/blob/e43aef0ef9cc9d2d12c138b36fd817f7c41d0152/library/std/src/sys/pal/windows/thread_local_key.rs
Note though that the StaticKey
(and related) bits aren't actually used if #[cfg(target_thread_local)]
.
The bug was reported on i686-pc-windows-gnu. Not sure if that's currently target_thread_local
or not... I think it is? Only the 32bit MSVC target is not? But I keep forgetting which one exactly it is so I may be wrong.
However I think even with target_thread_local
, dtors are handled the same way -- via thread::local_impl::Key::<T>::register_dtor
.
32-bit msvc is currently not target_thread_local
. However, that may change soon.
register_keyless_dtor
is used when target_thread_local
is enabled, otherwise register_dtor
is used. Both ultimately use the special .CRT$XLB
linker magic.
Ah, register_dtor
is actually register_keyless_dtor
.
pub use super::thread_local_key::register_keyless_dtor as register_dtor;
This is such a maze...^^
You're not the first to notice that: https://github.com/rust-lang/rust/issues/110897
The bug was reported on i686-pc-windows-gnu. Not sure if that's currently
target_thread_local
or not... I think it is? Only the 32bit MSVC target is not? But I keep forgetting which one exactly it is so I may be wrong.
All tier 1 windows-gnu
targets have target_thread_local
disabled.
There are similar tier 2 targets windows-gnullvm
which have working TLS and have target_thread_local
enabled.
Okay, thanks for clarifying.
Is there an issue tracking why the windows-gnu targets can't use target_thread_local?
Some of binaries produced by tests inside ui/
crash with some code that I can'it recall 😉
That could be discrepancy between our default LLVM backend which uses Windows native TLS and GNU tools which use emuTLS but I didn't dedicated enough time to it and I'm not planning currently.
I tried enabling native TLS in #102243, the result being access violations when running rustc. Things may have changed since then, though.
I've tried it multiple times over the years and the result was always the same, this time it could work if emutls is enabled as well: https://github.com/rust-lang/rust/pull/117873 but I don't have high hopes here and I think it'd still require some fixes.
EDIT: As I anticipated it needs more work:
Seems to randomly happen in CI as well https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/memleak https://github.com/rust-lang/rust/pull/124248#issuecomment-2068731334
So, it was not some issue with the global destructor after all. And it seems to always hit only the reentrant lock, even though that is not the only thread-local we have.
Maybe there's an actual race condition leading to occasional memory leaks here?
Minimal reproducer so far:
use std::thread;
pub(crate) fn current_thread_unique_ptr() {
thread_local! { static X: u8 = const { 0 } }
X.with(|_x| {})
}
fn main() {
let j2 = thread::spawn(current_thread_unique_ptr);
current_thread_unique_ptr();
j2.join().unwrap();
}
The leak only occurs on the windows-gnu targets.
Presumably this is due to the TLS dtor when not has_thread_local
.
All the msvc targets currently use the #[cfg(target_thread_local)]
code.
Meanwhile, https://github.com/rust-lang/rust/pull/124270 should work around the CI issue.
FWIW the issue reproduces with and without const { ... }
.
Hm, that's very strange... supposedly there's Box::new
being called here to create a Box with content std::sys::thread_local::os_local::Value<u8>
:
... there's no call to Box
there that I can see?!?
The span specifically points at the move ||
.
Oh never mind, I copied the span from the closure not the stacktrace.^^
It's this Box:
One key ingredient to the leak seems to be not loading the latest value here:
Cc @joboet
Ah, I think I got it. :) https://github.com/rust-lang/rust/pull/124281
🏅
I've only seen this once, even though the test should be covered by miri-test-libstd every day:
Cc @joboet @m-ou-se @Amanieu