Skip to content
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

size_of_val_raw: for length 0 this is safe to call #126152

Merged
merged 1 commit into from
Jul 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions library/core/src/alloc/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ impl Layout {
/// - a [slice], then the length of the slice tail must be an initialized
/// integer, and the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// For the special case where the dynamic tail length is 0, this function
/// is safe to call.
/// - a [trait object], then the vtable part of the pointer must point
/// to a valid vtable for the type `T` acquired by an unsizing coercion,
/// and the size of the *entire value*
Expand Down
8 changes: 8 additions & 0 deletions library/core/src/mem/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,12 @@ pub const fn size_of_val<T: ?Sized>(val: &T) -> usize {
/// - a [slice], then the length of the slice tail must be an initialized
/// integer, and the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// For the special case where the dynamic tail length is 0, this function
/// is safe to call.
// NOTE: the reason this is safe is that if an overflow were to occur already with size 0,
// then we would stop compilation as even the "statically known" part of the type would
// already be too big (or the call may be in dead code and optimized away, but then it
// doesn't matter).
/// - a [trait object], then the vtable part of the pointer must point
/// to a valid vtable acquired by an unsizing coercion, and the size
/// of the *entire value* (dynamic tail length + statically sized prefix)
Expand Down Expand Up @@ -506,6 +512,8 @@ pub const fn align_of_val<T: ?Sized>(val: &T) -> usize {
/// - a [slice], then the length of the slice tail must be an initialized
/// integer, and the size of the *entire value*
/// (dynamic tail length + statically sized prefix) must fit in `isize`.
/// For the special case where the dynamic tail length is 0, this function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing tests exercising this specifically (in the "success" sense, perhaps miri tests or a run-pass test w/ an assert_eq!)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds the relevant test. I don't think there's a useful "success" test here, what do you have in mind?

Copy link
Member

@compiler-errors compiler-errors Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, when we're calling size_of_val_raw on a struct with a tail who is a slice of size zero or something and the pointer is invalid or something (is that what this is guaranteeing??).

I just think it's weird that we're adding assumptions guaranteeing additional special cases for safety here, but adding tests that guarantee an error. I think that's because of the note you had above:

NOTE: the reason this is safe is that if an overflow were to occur already with size 0, then we would stop compilation as even the "statically known" part of the type would already be too big (or the call may be in dead code and optimized away, but then it doesn't matter).

but I'm just confused why a "negative" test is guaranteed for a new "positive" property, if you see what I mean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I can see how that's confusing. But a test that just checks that size_of_val_raw returns the right result for some small slice seems entirely pointless to me, the functionality of that intrinsic is already covered by existing tests. This includes all the Miri tests that use size_of_val/size_of_val_raw. The key point is that compilation is guaranteed to fail if an overflow were to happen, and that's what the new test ensures.

/// is safe to call.
/// - a [trait object], then the vtable part of the pointer must point
/// to a valid vtable acquired by an unsizing coercion, and the size
/// of the *entire value* (dynamic tail length + statically sized prefix)
Expand Down
18 changes: 18 additions & 0 deletions tests/ui/layout/size-of-val-raw-too-big.rs
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
//@ build-fail
//@ compile-flags: --crate-type lib
//@ only-32bit Layout computation rejects this layout for different reasons on 64-bit.
//@ error-pattern: too big for the current architecture
#![feature(core_intrinsics)]
#![allow(internal_features)]

// isize::MAX is fine, but with the padding for the unsized tail it is too big.
#[repr(C)]
pub struct Example([u8; isize::MAX as usize], [u16]);

// We guarantee that with length 0, `size_of_val_raw` (which calls the `size_of_val` intrinsic)
// is safe to call. The compiler aborts compilation if a length of 0 would overflow.
// So let's construct a case where length 0 just barely overflows, and ensure that
// does abort compilation.
pub fn check(x: *const Example) -> usize {
unsafe { std::intrinsics::size_of_val(x) }
}
4 changes: 4 additions & 0 deletions tests/ui/layout/size-of-val-raw-too-big.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: values of the type `Example` are too big for the current architecture

error: aborting due to 1 previous error

Loading