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

Valgrind leak check reports a "possibly lost" leak on std::thread::current() #135608

Open
purplesyringa opened this issue Jan 17, 2025 · 8 comments
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@purplesyringa
Copy link
Contributor

purplesyringa commented Jan 17, 2025

Code

I tried this code:

fn main() {
    let _ = std::thread::current();
}

...under valgrind.

I expected to see this happen: no leaks

Instead, this happened:

==2008936== Memcheck, a memory error detector
==2008936== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.
==2008936== Using Valgrind-3.24.0 and LibVEX; rerun with -h for copyright info
==2008936== Command: target/debug/cringe
==2008936== 
==2008936== 
==2008936== HEAP SUMMARY:
==2008936==     in use at exit: 48 bytes in 1 blocks
==2008936==   total heap usage: 8 allocs, 7 frees, 2,120 bytes allocated
==2008936== 
==2008936== 48 bytes in 1 blocks are possibly lost in loss record 1 of 1
==2008936==    at 0x48447A8: malloc (vg_replace_malloc.c:446)
==2008936==    by 0x138B77: alloc (alloc.rs:96)
==2008936==    by 0x138B77: alloc_impl (alloc.rs:192)
==2008936==    by 0x138B77: allocate (alloc.rs:254)
==2008936==    by 0x138B77: {closure#0}<std::thread::Inner> (sync.rs:484)
==2008936==    by 0x138B77: allocate_for_layout<core::mem::maybe_uninit::MaybeUninit<std::thread::Inner>, alloc::sync::{impl#14}::new_uninit::{closure_env#0}<std::thread::Inner>, fn(*mut u8) -> *mut alloc::sync::ArcInner<core::mem::maybe_uninit::MaybeUninit<std::thread::Inner>>> (sync.rs:1948)
==2008936==    by 0x138B77: new_uninit<std::thread::Inner> (sync.rs:482)
==2008936==    by 0x138B77: std::thread::Thread::new (mod.rs:1429)
==2008936==    by 0x138909: std::thread::current::init_current (current.rs:227)
==2008936==    by 0x11D516: cringe::main (main.rs:2)
==2008936==    by 0x11D44A: core::ops::function::FnOnce::call_once (function.rs:250)
==2008936==    by 0x11D3CD: std::sys::backtrace::__rust_begin_short_backtrace (backtrace.rs:152)
==2008936==    by 0x11D3A0: std::rt::lang_start::{{closure}} (rt.rs:194)
==2008936==    by 0x13852F: call_once<(), (dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (function.rs:284)
==2008936==    by 0x13852F: do_call<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panicking.rs:587)
==2008936==    by 0x13852F: try<i32, &(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe)> (panicking.rs:550)
==2008936==    by 0x13852F: catch_unwind<&(dyn core::ops::function::Fn<(), Output=i32> + core::marker::Sync + core::panic::unwind_safe::RefUnwindSafe), i32> (panic.rs:358)
==2008936==    by 0x13852F: {closure#0} (rt.rs:163)
==2008936==    by 0x13852F: do_call<std::rt::lang_start_internal::{closure_env#0}, isize> (panicking.rs:587)
==2008936==    by 0x13852F: try<isize, std::rt::lang_start_internal::{closure_env#0}> (panicking.rs:550)
==2008936==    by 0x13852F: catch_unwind<std::rt::lang_start_internal::{closure_env#0}, isize> (panic.rs:358)
==2008936==    by 0x13852F: std::rt::lang_start_internal (rt.rs:159)
==2008936==    by 0x11D386: std::rt::lang_start (rt.rs:193)
==2008936==    by 0x11D54D: main (in /home/purplesyringa/cringe/target/debug/cringe)
==2008936== 
==2008936== LEAK SUMMARY:
==2008936==    definitely lost: 0 bytes in 0 blocks
==2008936==    indirectly lost: 0 bytes in 0 blocks
==2008936==      possibly lost: 48 bytes in 1 blocks
==2008936==    still reachable: 0 bytes in 0 blocks
==2008936==         suppressed: 0 bytes in 0 blocks
==2008936== 
==2008936== For lists of detected and suppressed errors, rerun with: -s
==2008936== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

This is basically #133574 again. Calling this a regression might be stretching it a bit too far, but the consensus on that issue seemed to be that we should fix this if we can.

Version it worked on

It most recently worked on: Rust 1.85.0-beta.2, as far as I'm aware

Version with regression

rustc --version --verbose:

rustc 1.86.0-nightly (419b3e2d3 2025-01-15)
binary: rustc
commit-hash: 419b3e2d3e350822550eee0e82eeded4d324d584
commit-date: 2025-01-15
host: x86_64-unknown-linux-gnu
release: 1.86.0-nightly
LLVM version: 19.1.6
@purplesyringa purplesyringa added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Jan 17, 2025
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. A-thread Area: `std::thread` regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed regression-untriaged Untriaged performance or correctness regression. labels Jan 17, 2025
@jieyouxu
Copy link
Member

Is this a full leakcheck?

@purplesyringa
Copy link
Contributor Author

Yes.

@jieyouxu jieyouxu removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jan 17, 2025
@jieyouxu
Copy link
Member

Based on the consensus in #133574, non-default full leakcheck is "convenience if we can, but no hard guarantees", removing regression and prioritize labels.

@jieyouxu jieyouxu changed the title Memory leak from std::thread::current() (again) Valgrind full leak check reports a leak on std::thread::current() Jan 17, 2025
@jieyouxu
Copy link
Member

(I edited the issue title to better reflect this requires valgrind full leak check)

@purplesyringa
Copy link
Contributor Author

purplesyringa commented Jan 17, 2025

To be clear, all a full leak check does is enable more logs. I get this even without full leak check:

==2009708== HEAP SUMMARY:
==2009708==     in use at exit: 48 bytes in 1 blocks
==2009708==   total heap usage: 8 allocs, 7 frees, 2,120 bytes allocated
==2009708== 
==2009708== LEAK SUMMARY:
==2009708==    definitely lost: 0 bytes in 0 blocks
==2009708==    indirectly lost: 0 bytes in 0 blocks
==2009708==      possibly lost: 48 bytes in 1 blocks
==2009708==    still reachable: 0 bytes in 0 blocks
==2009708==         suppressed: 0 bytes in 0 blocks
==2009708== Rerun with --leak-check=full to see details of leaked memory

So you can still see those 48 "possibly lost" bytes and wonder where they come from. IMO renaming this issue is misleading, and, again, IMO, feel free to disagree -- it's still a kind of regression.

@jieyouxu jieyouxu changed the title Valgrind full leak check reports a leak on std::thread::current() Valgrind leak check reports a leak on std::thread::current() Jan 17, 2025
@jieyouxu jieyouxu changed the title Valgrind leak check reports a leak on std::thread::current() Valgrind leak check reports a "possibly lost" leak on std::thread::current() Jan 17, 2025
@jieyouxu
Copy link
Member

I did the initial triaging, I'll let the T-libs maintainers re-triage if this should be considered a regression. Thanks for the further clarifications.

@workingjubilee
Copy link
Member

@joboet Is this an inevitable result of your preferred design for handling the thread allocations?

@joboet
Copy link
Member

joboet commented Jan 17, 2025

This is basically an instance of #28129. After #127912, the destructor for the TLS variable behind thread::current() is registered through a TLS key in order to support running the destructor after the destructors of normal TLS variables. Unfortunately, TLS key destructors are not run for the main thread on Linux (destructors registered with __cxa_thread_atexit_impl like those for normal TLS variables are run, however), resulting in a leak. #123550 eliminated the Thread allocation for the main thread, which fixed the leak, but I reverted it in #132654 because the added complexity was not worth it.

I think this is fixed by #134085, which uses atexit to fix #28129, but has some side-effects which T-libs will need to consider as well. If that doesn't work, I think we should just accept this – the only thing being leaked is basically static memory which will be cleaned up by the kernel anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-thread Area: `std::thread` C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants