Miri sometimes reports memory leak in thread-local storage on *-pc-windows-gnu #123583
Labels
A-miri
Area: The miri tool
A-thread-locals
Area: Thread local storage (TLS)
C-bug
Category: This is a bug.
O-windows
Operating system: Windows
Comments
rustbot
added
the
needs-triage
|
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 |
|
I tried to figure out how TLS is currently implemented on Windows, but I got lost in the dark forest that is EDIT: Oh, looks like |
|
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 |
|
The bug was reported on i686-pc-windows-gnu. Not sure if that's currently |
|
However I think even with |
|
32-bit msvc is currently not
|
|
Ah, 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: #110897 |
Nilstrieb
added
O-windows
This was referenced Apr 14, 2024
RalfJung
changed the title
All tier 1 |
|
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 |
|
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: #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: Details$ git diff
diff --git a/compiler/rustc_target/src/spec/base/windows_gnu.rs b/compiler/rustc_target/src/spec/base/windows_gnu.rs
index 25f02dc1451..3b2d174071a 100644
--- a/compiler/rustc_target/src/spec/base/windows_gnu.rs
+++ b/compiler/rustc_target/src/spec/base/windows_gnu.rs
@@ -1,4 +1,4 @@
-use crate::spec::LinkSelfContainedDefault;
+use crate::spec::{LinkSelfContainedDefault, TlsModel};
use crate::spec::{add_link_args, crt_objects};
use crate::spec::{cvs, Cc, DebuginfoKind, LinkerFlavor, Lld, SplitDebuginfo, TargetOptions};
use std::borrow::Cow;
@@ -40,6 +40,7 @@ pub fn opts() -> TargetOptions {
//
// See https://github.com/rust-lang/rust/pull/47483 for some more details.
"-lmsvcrt",
+ "-lmingwex", // This one is required for newer mingw-w64 versions
"-luser32",
"-lkernel32",
];
@@ -103,6 +104,8 @@ pub fn opts() -> TargetOptions {
// output DWO, despite using DWARF, doesn't use ELF..
debuginfo_kind: DebuginfoKind::Pdb,
supported_split_debuginfo: Cow::Borrowed(&[SplitDebuginfo::Off]),
+ has_thread_local: true,
+ tls_model: TlsModel::Emulated,
..Default::default()
}
} |
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this issue
Apr 16, 2024
Miri on Windows: run .CRT$XLB linker section on thread-end Hopefully fixes rust-lang/rust#123583 First commit is originally by `@bjorn3` r? `@oli-obk` Cc `@ChrisDenton`
|
Seems to randomly happen in CI as well |
RalfJung
changed the title
|
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? |
RalfJung
changed the title
|
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 All the msvc targets currently use the |
|
Meanwhile, #124270 should work around the CI issue. |
|
FWIW the issue reproduces with and without |
This comment was marked as resolved.
This comment was marked as resolved.
|
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 |
bors
added a commit
to rust-lang-ci/rust
that referenced
this issue
Apr 22, 2024
miri libstd tests: test windows-msvc instead of windows-gnu This avoids rust-lang#123583. MSVC is the more widely used target anyway AFAIK, so probably makes more sense to cover that.
|
Ah, I think I got it. :) |
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Apr 24, 2024
Rollup merge of rust-lang#124281 - RalfJung:win-tls, r=joboet fix weak memory bug in TLS on Windows We need to store the `key` *after* we register the dtor. Now I hope there isn't also some other reason why we have to actually register the dtor last... `@joboet` is there a reason you picked this particular order in rust-lang#102655? Fixes rust-lang#123583
|
🏅 |
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this issue
Apr 25, 2024
fix weak memory bug in TLS on Windows We need to store the `key` *after* we register the dtor. Now I hope there isn't also some other reason why we have to actually register the dtor last... `@joboet` is there a reason you picked this particular order in rust-lang/rust#102655? Fixes rust-lang/rust#123583
bors
added a commit
to rust-lang/miri
that referenced
this issue
Apr 25, 2024
add a test for the TLS memory leak This is a regression test for rust-lang/rust#123583.
RalfJung
pushed a commit
to RalfJung/rust
that referenced
this issue
Apr 25, 2024
add a test for the TLS memory leak This is a regression test for rust-lang#123583.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
The text was updated successfully, but these errors were encountered: