never construct value on stack in new_box_zeroed #1601
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1601 +/- ##
=======================================
Coverage 87.70% 87.70%
=======================================
Files 15 15
Lines 5565 5565
=======================================
Hits 4881 4881
Misses 684 684 ☔ View full report in Codecov by Sentry. |
|
Thanks for this! It looks reasonable, although I have a concern with the cited Box docs: rust-lang/unsafe-code-guidelines#529 Once that's resolved, I'll approve. |
|
Blocked on rust-lang/rust#129748. |
There was a problem hiding this comment.
Okay, rust-lang/rust#129748 merged, so we're good to continue here.
When you make changes, please squash everything together into the same commit and force-push instead of adding new commits.
src/lib.rs
Outdated
| // SAFETY: Contructing a Box to a ZST from a dangling pointer is | ||
| // explicitly allowed: | ||
| // https://doc.rust-lang.org/std/boxed/index.html#memory-layout |
There was a problem hiding this comment.
Can you update this to abide by our safety comment policy?*
| // SAFETY: Contructing a Box to a ZST from a dangling pointer is | |
| // explicitly allowed: | |
| // https://doc.rust-lang.org/std/boxed/index.html#memory-layout | |
| // SAFETY: Per [1], when `T` is a ZST, `Box<T>`'s only validity requirements are that | |
| // the pointer is non-null and sufficiently aligned. Per [2], `NonNull::dangling` produces | |
| // a pointer which is sufficiently aligned. Since the produced pointer is a `NonNull`, | |
| // it is non-null. | |
| // | |
| // [1] Per https://doc.rust-lang.org/nightly/std/boxed/index.html#memory-layout: | |
| // | |
| // For zero-sized values, the `Box` pointer has to be non-null and sufficiently aligned. | |
| // | |
| // [2] Per https://doc.rust-lang.org/std/ptr/struct.NonNull.html#method.dangling: | |
| // | |
| // Creates a new `NonNull` that is dangling, but well-aligned. |
(I'm sure the line wrapping on that comment is wrong - just typed in the web UI)
* This technically violates the "don't cite nightly docs" policy, but I'm comfortable that this isn't liable to change.
| unsafe { | ||
| return Box::from_raw(NonNull::dangling().as_ptr()); | ||
| } |
There was a problem hiding this comment.
Can you also add a comment that explains why we do this instead of Box::new(Self::new_zeroed())? The argument in your commit message makes sense, but I don't think it's obvious enough that we should expect future readers to figure it out from context.
On lower opt-levels the compiler might not optimize out the `layout.size() == 0` branch and emits code for the if-body. This will cause a stack allocation for `Self`. Avoid calling new_zeroed() and directly construct the Box from a dangling pointer instead. Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Freax13
force-pushed
the
fix/stack-alloc
branch
from
074ee4e
to
71d93cf
Compare
On lower opt-levels the compiler might not optimize out the `layout.size() == 0` branch and emits code for the if-body. This will cause a stack allocation for `Self`. Avoid calling new_zeroed() and directly construct the Box from a dangling pointer instead. Co-authored-by: Joshua Liebow-Feeser <[email protected]>
|
Update on this: I've published 0.8.0-alpha.18, which includes this change. It should be trivial to backport to 0.7 as well (in progress in #1604), but we're having unrelated CI issues that are blocking it from merging. We probably won't have time to burn those CI issues down for at least a week or two, and possibly longer. |
|
Thanks for the update! |
On lower opt-levels the compiler might not optimize out the
layout.size() == 0branch and emits code for the if-body. This will cause a stack allocation forSelf. Avoid calling new_zeroed() and directly construct the Box from a dangling pointer instead.