-
Notifications
You must be signed in to change notification settings - Fork 352
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
Treat thread-local statics on main thread as static roots for leakage analysis #2931
Conversation
I did some extra logging, and the issue turns out to be that all the static roots you added are deallocated at the end of the program, causing the leaked allocations within to not be detected anymore. If you do not leak, then the thread local statics (even of the main thread) get freed. As an example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3c4634970fddcc8784f4376a8e1241fd passes without leaking. This means we cannot just apply what was proposed in #2881 (comment) Knowing this, do you still think it's worth it to add this extra logic for something that goes away if the main thread TLS actually cleans up its memory? |
Oh right, we'd have to also skip deallocating main thread TLS.
|
@rustbot author |
Does this mean the value actually doesn't live for the lifetime of the program, or is it still valid to consider the program as ended before the main thread cleans up its TLS?
Would this mean missing out on analysis of main thread TLS destructors? E.g. if a destructor contained UB we'd no longer be able to detect it? Instead of skipping deallocation, could we run the leakage check before the main thread cleans up? @rustbot label -S-waiting-on-author +S-waiting-on-review |
We should definitely run the destructor. We just shouldn't deallocate the backing storage. |
tests/fail/leak_in_tls.rs
Outdated
pub fn main() { | ||
thread_local! { | ||
static REF: Cell<Option<&'static i32>> = Cell::new(None); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this will test that we allow leaks in one of the 2 kinds of TLS (TLS statics vs OS-supported TLS keys). We should make sure to have tests covering both (by directly using the underlying mechanisms).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #[thread_local]
s to the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want one test that uses #[thread_local]
and one that uses pthread TLS keys directly.
Or we only treat #[thread_local]
as static roots? Then we cover all targets with target_thread_local
. Doing special magic to library stuff like the pthread keys is somewhat dubious anyway...
23ad36d
to
81ea21b
Compare
looks like the msvc thread locals need a similar treatment. I think they have a completely separate code path |
@rustbot author |
separate from this path, you mean? miri/src/shims/windows/foreign_items.rs Lines 241 to 269 in 6f688d5
|
Sorry for pointing you to Lines 250 to 252 in 8bbc93c
by code that instead marks the
|
@max-heller this is waiting for you to resolve the remaining comments. See the open threads above, in particular here and here. Once this is ready for review again, please use |
Addressed the open requests, sorry for the delay. |
903dc41
to
6605b0c
Compare
No worries, it's not like I had fast reaction times on this PR in the past. :) |
This comment was marked as outdated.
This comment was marked as outdated.
@rustbot author |
@rustbot ready |
@rustbot author |
fbab71a
to
5bc7af2
Compare
@rustbot ready |
Awesome, thanks for staying with us until this is finally done. :) |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Miri currently treats allocations as leaked if they're only referenced in thread-local statics. For threads other than the main thread, this is correct, since the thread can terminate before the program does, but references in the main thread's locals should be treated as living for the duration of the program since the thread lives for the duration of the program.
This PR adds thread-local statics and TLS keys as "static roots" for leakage analysis, but does not yet bless the example program from #2881. See #2881 (comment)
Closes #2881