Windows TLS: ManuallyDrop instead of mem::forget #79893
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -111,16 +111,13 @@ struct Node { | |||
| } | |||
|
|
|||
| unsafe fn register_dtor(key: Key, dtor: Dtor) { | |||
| let mut node = Box::new(Node { key, dtor, next: ptr::null_mut() }); | |||
| let mut node = ManuallyDrop::new(Box::new(Node { key, dtor, next: ptr::null_mut() })); | |||
There was a problem hiding this comment.
Since we never drop this anyway, couldn't we also just Box::into_raw here and then compare_exchangeing the raw pointer directly? I guess that would mean node.next = head needs to be (*node).next = head and is now unsafe, so maybe the ManuallyDrop is better, but I am somewhat unhappy with the &mut **node (the previous &mut *node was not better imo). I know we can rely on the deref ops on boxes, but it still seems wrong to me to not be using Box::into_raw when this is technically what we want.
There was a problem hiding this comment.
I guess that would mean node.next = head needs to be (*node).next = head and is now unsafe
I went with into_raw first and decided to use ManuallyDrop when I realized this.
|
📌 Commit 594b451 has been approved by |
bors
added
S-waiting-on-bors
| mem::forget(node); | ||
| return; | ||
| } | ||
| match DTORS.compare_exchange(head, &mut **node, SeqCst, SeqCst) { |
There was a problem hiding this comment.
This isn't really related, but shouldn't we use compare_exchange_weak here ?
There was a problem hiding this comment.
Possible; I don't really have a clear idea for what should be used when.
Since it's not related, that should then probably be fixed by someone else in a separate PR. :)
There was a problem hiding this comment.
Fine, I'll do it when this PR is merged.
There was a problem hiding this comment.
@a1phyr Did this end up happening?
Windows TLS: ManuallyDrop instead of mem::forget The Windows TLS implementation still used `mem::forget` instead of `ManuallyDrop`, leading to the usual problem of "using" the `Box` when it should not be used any more.
|
☀️ Test successful - checks-actions |
enable track-raw-ptr tests on Windows With rust-lang/rust#79893 landed, raw-pointer-tracking now works even for the Windows runtime. :)
The Windows TLS implementation still used
mem::forgetinstead ofManuallyDrop, leading to the usual problem of "using" theBoxwhen it should not be used any more.