Implement support for multiple TLS destructors on macOS #3739
Conversation
|
It is however UB to set another TLS dtor while TLS dtors are running, as far as I know. So Miri should probably detect that. Curiously this could also mean one can cause UB in the std test suite, once that |
src/shims/tls.rs
Outdated
| /// Add a thread local storage for the given thread. This function is used to | ||
| /// implement `_tlv_atexit` shim on MacOS. | ||
| /// FIXME: also implement `__cxa_thread_atexit_impl` for Linux using this, see | ||
| /// <https://sourceware.org/glibc/wiki/Destructor%20support%20for%20thread_local%20variables> |
There was a problem hiding this comment.
This seems better suited for an issue rather than a FIXME in the code.
src/shims/tls.rs
Outdated
| for (instance, data) in dtors.into_iter().rev() { | ||
| trace!("Running macos dtor {:?} on {:?} at {:?}", instance, data, thread_id); | ||
|
|
||
| this.call_function( |
There was a problem hiding this comment.
No this is not right. call_function just pushes the stack frame, so what this does is push a stack frame for each of these functions and then run them all, making the stack look as if the 2nd dtor was called by the first.
There's a big surrounding interpreter loop here, and the next dtor needs to be started only once this dtor finishes running, when the stack is empty again. See what we do for WindowsDtors for how that can be implemented.
There was a problem hiding this comment.
To give a bit of context here, each thread has a TlsDtorsState and each time that thread's stack is empty, the main loop calls on_stack_empty on it. If that returns Pending, the thread sticks around. This may also push a new stack frame, in which case that now gets executed, until the stack is empty and on_stack_empty is called again. This repeats until on_stack_empty returns Ready, at which point the thread is fully terminated and never scheduled again.
It isn't in newer versions of macOS, they fixed that bug. |
|
And that is an intentional guarantee? Seems like the behavior is to run the newest things first. So probably what should happen is quite different from Please make sure the test covers "registering a dtor inside the dtor", then. |
|
It seems very unlikely that they would remove that fix. But of course, this is Apple we're talking about, so I don't think we'll ever get a proper guarantee. It doesn't matter too much for us anyway, since we still support the older platforms that crash. |
joboet
force-pushed
the
macos_tls_dtors
branch
3 times, most recently
from
c9bc481 to
de25371
Compare
src/shims/tls.rs
Outdated
| @@ -202,6 +191,8 @@ impl<'tcx> TlsData<'tcx> { | |||
| for TlsEntry { data, .. } in self.keys.values_mut() { | |||
| data.remove(&thread_id); | |||
| } | |||
|
|
|||
| self.macos_thread_dtors.remove(&thread_id); | |||
There was a problem hiding this comment.
Aren't these always empty because we ran the destructors?
There was a problem hiding this comment.
Yes, but it seems neater to free the memory of the Vec here.
There was a problem hiding this comment.
Can you add an assertion ensuring the vector we pop is empty?
tests/pass/tls/macos_tlv_atexit.rs
Outdated
| where | ||
| F: FnOnce() + 'static, | ||
| { | ||
| unsafe extern "C" fn run<F>(data: *mut u8) |
There was a problem hiding this comment.
Please add a comment saying that data is expected to actually be a Box<F>
tests/pass/tls/macos_tlv_atexit.rs
Outdated
| where | ||
| F: FnOnce() + 'static, | ||
| { | ||
| unsafe { (*Box::from_raw(data as *mut F))() } |
There was a problem hiding this comment.
It took me a bit to realize you are also calling the function here. Please let-bind the Box::from_raw(data as *mut F) so that this becomes more clear.
|
Looks good, thanks. :) |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
With rust-lang/miri#3739 merged, the deduplication hack is no longer necessary.
std: use duplicate thread local state in tests With rust-lang/miri#3739 merged, the deduplication hack is no longer necessary.
std: use duplicate thread local state in tests With rust-lang/miri#3739 merged, the deduplication hack is no longer necessary.
Rollup merge of rust-lang#128135 - joboet:reduplicate_tls, r=tgross35 std: use duplicate thread local state in tests With rust-lang/miri#3739 merged, the deduplication hack is no longer necessary.
I want to get rid of this
#[cfg]block instd, but it is currently required for miri, as it does not support multiple macOS TLS destructors. This is not true for the platform itself, however, as can be observed in the implementation.