Conversation
| Anonymous: D3D12_RESOURCE_BARRIER_0 { | ||
| Transition: std::mem::ManuallyDrop::new(D3D12_RESOURCE_TRANSITION_BARRIER { | ||
| pResource: ManuallyDrop::new(resource), | ||
| pResource: unsafe { std::mem::transmute_copy(resource) }, |
There was a problem hiding this comment.
@kennykerr what's the guidance for this? Seems the typical user might go for a safe ManuallyDrop::new(Some(resource.clone())), not immediately realizing that this keeps the refcount on resource up, leaking it.
IMO the previous ManuallyDrop implementation was clearer in terms of sneakily holding a borrow for which the refcount wasn't increased.
Perhaps a much more invasive change would be having lifetimes on structs: &'a X has the same layout as *const X/*mut X.
There was a problem hiding this comment.
I addressed that briefly here: #2386 - while the custom ManuallyDrop was more concise, it did hide the fact that you were doing something very dangerous by sharing the reference. I'd love to use lifetimes but yes that's way more invasive and in many cases the lifetime isn't easy to reason about. So here at least we're acknowledging you need to pay close attention. 🤔
There was a problem hiding this comment.
Oh, I just saw your example was cloning - yes that would be bad. 😅
I don't know what to say - hopefully developers using Direct3D know enough about COM to realize that would be wrong. Direct3D also has leak detection support so that should help catch bugs like that.
There was a problem hiding this comment.
So here at least we're acknowledging you need to pay close attention.
You already seem to have figured it out, and I hope that most Rust programmers' alarm bells ring upon seeing ManuallyDrop::new(x.clone()) (in whatever context), but it is still rather easy to make this mistake as it's the only safe way to write this. Even worse when already having ownership of x and not needing to clone at all making it rather invisible (apart needing to call ManuallyDrop::new()) 😬
In my own code I didn't see the leak detector triggering (on resources), only resource overlapping further down the line - might have to dig a bit further.
I addressed that briefly here: #2386
Great to have that, even better if it could be part of some obvious/up-front documentation? Or an API that nudges towards this altogether? Not sure how it would look like, or if borrows would be more beneficial in the end.
2 participants
Thanks to #2360, I can now get rid of the weird custom
ManuallyDropin favor of the standard type. Previously, the parameter binding support depended on a customManuallyDropimplementation.Fixes: #2294