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

Avoid regression in consteval #2286

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

Manishearth
Copy link
Member

@Manishearth Manishearth requested a review from sffc as a code owner July 29, 2022 21:51
Comment on lines -260 to 264
let (data, mut metadata): (usize, usize) = core::mem::transmute(bytes);
// n.b. be careful here, this might hit https://github.com/rust-lang/rust/issues/99923
let data = bytes.as_ptr();
let mut metadata = bytes.len();
metadata /= core::mem::size_of::<T::ULE>();
Self::Borrowed(core::mem::transmute((data, metadata)))
Copy link

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yay!

@Manishearth Manishearth merged commit a32efd0 into unicode-org:main Jul 29, 2022
@@ -257,7 +257,9 @@ where
/// `bytes` need to be an output from [`ZeroSlice::as_bytes()`].
pub const unsafe fn from_bytes_unchecked(bytes: &'a [u8]) -> Self {
// &[u8] and &[T::ULE] are the same slice with different length metadata.
let (data, mut metadata): (usize, usize) = core::mem::transmute(bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW the old code most likely has UB. Quoting from the transmute docs:

Transmuting pointers to integers in a const context is undefined behavior. Any attempt to use the resulting value for integer operations will abort const-evaluation.

So, if from_bytes_unchecked is ever called with a non-zero-sized slice (or a zero-sized slice that actually uses a pointer to some allocated memory), then this is definitely UB. This PR is a UB-fix, not a rustc-regression-workaround.

Copy link
Member Author

@Manishearth Manishearth Jul 30, 2022

Choose a reason for hiding this comment

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

Yeah I'm aware it's UB.

As I've stated in rust-lang/rust#99923, I still consider things like this a regression as long as the docs on undefined behavior are incomplete, scattered, and in flux, and avoiding UB is not practically possible for unsafe Rust programmers.

@Manishearth Manishearth deleted the databake-regression branch July 30, 2022 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants