-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add KindaSortaDangling internal type to make Yoke safe under strong protection #3735
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b585d3e
KindaSortaDangling
Manishearth 3944f0c
Add test
Manishearth 7f2fc98
Use in Yoke
Manishearth ecb1f53
Handle size changes
Manishearth 44258b0
forgot Drop
Manishearth 1387e40
experimental
Manishearth 92e3a3b
Update utils/yoke/tests/miri_retag.rs
Manishearth 9f1518e
comment
Manishearth daf21a6
review fixes
Manishearth 77eb8b7
Update utils/yoke/src/kinda_sorta_dangling.rs
Manishearth 75170c8
rename miri test
Manishearth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
// This file is part of ICU4X. For terms of use, please see the file | ||
// called LICENSE at the top level of the ICU4X source tree | ||
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). | ||
|
||
use core::mem::{ManuallyDrop, MaybeUninit}; | ||
use core::ops::{Deref, DerefMut}; | ||
|
||
/// This type is intended to be similar to the type `MaybeDangling<T>` | ||
/// proposed in [RFC 3336]. | ||
/// | ||
/// The effect of this is that in Rust's safety model, types inside here are not | ||
/// expected to have any memory dependent validity properties (`dereferenceable`, `noalias`). | ||
/// | ||
/// See [#3696] for a testcase where `Yoke` fails this under miri's field-retagging mode. | ||
/// | ||
/// This has `T: 'static` since we don't need anything | ||
/// else and we don't want to have to think (more) about variance over lifetimes or dropck. | ||
/// | ||
/// After [RFC 3336] lands we can use `MaybeDangling` instead. | ||
/// | ||
/// Note that a version of this type also exists publicly as the [`maybe_dangling`] | ||
/// crate; which also exports a patched `ManuallyDrop` with similar semantics and | ||
/// does not require `T: 'static`. Consider using this if you need something more general | ||
/// and are okay with adding dependencies. | ||
/// | ||
/// [RFC 3336]: https://github.com/rust-lang/rfcs/pull/3336 | ||
Manishearth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// [#3696]: https://github.com/unicode-org/icu4x/issues/3696 | ||
/// [`maybe_dangling`](https://docs.rs/maybe-dangling/0.1.0/maybe_dangling/struct.MaybeDangling.html) | ||
#[repr(transparent)] | ||
pub(crate) struct KindaSortaDangling<T: 'static> { | ||
Manishearth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// Safety invariant: This is always an initialized T, never uninit or other | ||
/// invalid bit patterns. Its drop glue will execute during Drop::drop rather than | ||
/// during the drop glue for KindaSortaDangling, which means that we have to be careful about | ||
/// not touching the values as initialized during `drop` after that, but that's a short period of time. | ||
dangle: MaybeUninit<T>, | ||
} | ||
|
||
impl<T: 'static> KindaSortaDangling<T> { | ||
#[inline] | ||
pub(crate) const fn new(dangle: T) -> Self { | ||
KindaSortaDangling { | ||
dangle: MaybeUninit::new(dangle), | ||
} | ||
} | ||
#[inline] | ||
pub(crate) fn into_inner(self) -> T { | ||
// Self has a destructor, we want to avoid having it be called | ||
let manual = ManuallyDrop::new(self); | ||
// Safety: | ||
// We can call assume_init_read() due to the library invariant on this type, | ||
// however since it is a read() we must be careful about data duplication. | ||
// The only code using `self` after this is the drop glue, which we have disabled via | ||
// the ManuallyDrop. | ||
unsafe { manual.dangle.assume_init_read() } | ||
} | ||
} | ||
|
||
impl<T: 'static> Deref for KindaSortaDangling<T> { | ||
type Target = T; | ||
#[inline] | ||
fn deref(&self) -> &T { | ||
// Safety: Safe due to the safety invariant on `dangle`; | ||
// we can always assume initialized | ||
unsafe { self.dangle.assume_init_ref() } | ||
} | ||
} | ||
|
||
impl<T: 'static> DerefMut for KindaSortaDangling<T> { | ||
#[inline] | ||
fn deref_mut(&mut self) -> &mut T { | ||
// Safety: Safe due to the safety invariant on `dangle`; | ||
// we can always assume initialized | ||
unsafe { self.dangle.assume_init_mut() } | ||
} | ||
} | ||
|
||
impl<T: 'static> Drop for KindaSortaDangling<T> { | ||
#[inline] | ||
fn drop(&mut self) { | ||
unsafe { | ||
// Safety: We are reading and dropping a valid initialized T. | ||
// | ||
// As `drop_in_place()` is a `read()`-like duplication operation we must be careful that the original value isn't | ||
// used afterwards. It won't be because this is drop and the only | ||
// code that will run after this is `self`'s drop glue, and that drop glue is empty | ||
// because MaybeUninit has no drop. | ||
// | ||
// We use `drop_in_place()` instead of `let _ = ... .assume_init_read()` to avoid creating a move | ||
// of the inner `T` (without `KindaSortaDangling` protection!) type into a local -- we don't want to | ||
// assert any of `T`'s memory-related validity properties here. | ||
self.dangle.as_mut_ptr().drop_in_place(); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
// This file is part of ICU4X. For terms of use, please see the file | ||
// called LICENSE at the top level of the ICU4X source tree | ||
// (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). | ||
|
||
use yoke::Yoke; | ||
|
||
// Test for strong protection, should pass under miri with -Zmiri-retag-fields | ||
// See https://github.com/unicode-org/icu4x/issues/3696 | ||
|
||
fn example(_: Yoke<&'static [u8], Vec<u8>>) {} | ||
|
||
#[test] | ||
fn run_test() { | ||
example(Yoke::attach_to_cart(vec![0, 1, 2], |data| data)); | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bet the compiler was previously able to "merge" the
T
with whatever other fields were present on the struct, but now with the MaybeUninit it is forced to keep the payload as its own chunk of memory. A bit sad.EDIT: Nevermind, that doesn't make sense, because it's still possible to get a
&T
, so the compiler doesn't have the liberty to fiddle with the layout.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it only makes sense if there's an enum wrapping it, and the only enum I see is the thing that swaps between yokes and staticrefs, and I'm not quite sure how that could have a size impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like if the payload of two variants have a common part, and if one of the payloads separately has enough of a niche for Rust to be able to stuff the
enum
's discriminant inside, then it does so, effectively reducing the size of the wholeenum
. In which case the niche removal ofMaybeUninit
does effectively impact the overall size.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha! thanks for investigating!
Though the
DataPayload
enum here doesn't have variants with common parts before it gets to the yoke