[Merged by Bors] - remove potential ub in render_resource_wrapper#7279
[Merged by Bors] - remove potential ub in render_resource_wrapper#7279robtfm wants to merge 5 commits intobevyengine:mainfrom
Conversation
robtfm
added
A-Rendering
| pub struct $wrapper_type(Option<std::sync::Arc<Box<()>>>); | ||
| #[derive(Debug)] | ||
| // safety invariant: when self.0 is Some(ptr), it comes from `into_raw` of an Arc<$wgpu_type> with a strong ref. | ||
| pub struct $wrapper_type(Option<*const ()>); |
There was a problem hiding this comment.
*const () is Copy. Do you still need this to be an Option?
There was a problem hiding this comment.
yes, so that when we consume the arc in try_unwrap we don't then consume it again in Drop
There was a problem hiding this comment.
Can we just forget or ManuallyDrop self in try_unwrap?
There was a problem hiding this comment.
i mentioned that on the original issue here. i can if you like, i think it would be functionally the same, but i feel it would make the safety invariant less clear to verify (that the arc is valid iff the pointer is Some).
are you looking to avoid using extra space for the option?
There was a problem hiding this comment.
Both the extra space and the extra branch on all of these operations. If that's removed, we can probably use this implementation for the release mode too and avoid the need for a cfg block. (Provided it works out in the benchmarks/stress tests).
Not too big a deal if we avoid this. This will likely all go away once the upstream compiler issue is fixed.
There was a problem hiding this comment.
option removed.
its not committed here but i've tested in release, i see no impact on many_foxes and the compile time for changing code in bevy_render drops by around 40% there as well. not sure what the best benchmark would be though. either way i guess that should probably be a separate pr.
Co-authored-by: James Liu <contact@jamessliu.com>
Co-authored-by: James Liu <contact@jamessliu.com>
|
have you verified that this still keeps the compile time wins? I am rather confused as to how this is doing anything |
BoxyUwU
left a comment
BoxyUwU
left a comment
There was a problem hiding this comment.
This feels like a really bad tradeoff to me and we should remove this abstraction seeing as how it was implemented wrong and resulted in bevy being unsound for like 4+ months 🤷♀️
yes it keeps the improvement. the rustc issue linked in the code and the discussion on #5950 talk about the why, but it may not even be that exact issue. will need to recheck after a compiler fix. |
BoxyUwU
added
the
S-Ready-For-Final-Review
Seconding this sentiment. There's a lot of tricky complexity here. That said, fix seems fine and it's good to resolve this particular instance of UB. bors r+ |
# Objective [as noted](#5950 (comment)) by james, transmuting arcs may be UB. we now store a `*const ()` pointer internally, and only rely on `ptr.cast::<()>().cast::<T>() == ptr`. as a happy side effect this removes the need for boxing the value, so todo: potentially use this for release mode as well
|
Pull request successfully merged into main. Build succeeded:
|
bors
bot
changed the title
4 participants
Objective
as noted by james, transmuting arcs may be UB.
we now store a
*const ()pointer internally, and only rely onptr.cast::<()>().cast::<T>() == ptr.as a happy side effect this removes the need for boxing the value, so todo: potentially use this for release mode as well