Skip to content

Commit

Permalink
Fix miscompilation in transmute_unchecked introduced by bug in LLVM (
Browse files Browse the repository at this point in the history
…#25)

Currently some casts from byte primitives to two element tuple lead to
miscompilation on **release** builds on Rust `>=1.70.0`:
- `castaway::cast!(123_u8, (u8, u8))` unexpectedly returns `Ok(...)`
that leads to **UB**.
- `castaway::cast!(false, (bool, u16))` leads to `SIGILL: illegal
instruction` runtime error.

Upstream issues:
- Rust: rust-lang/rust#127286
- LLVM: llvm/llvm-project#97702

I suggest considering adding a safe "workaround" to fix the issue in
this crate without having to wait for the upstream fixes. This way we
will have this fixed in older Rust versions as well.

This PR adds size eq `assert` to `transmute_unchecked`. This workaround
was found while preparing an MRE for an upstream issue.

Checked locally with `cargo test --release` for Rust `1.38`, `1.68.0`,
`1.69.0`, `1.70.0`, `1.71.0`, `1.72.0`, `stable`, `beta`, `nightly`.
Generated assembly for other tests cases for the release build seems the
same (checks and casts are optimized away).

Btw: it might also be a good idea to run tests in `--release` mode as
well since the crate relies heavily on optimizing the casts to
zero-cost.
  • Loading branch information
zheland authored Jul 4, 2024
1 parent 7e15c46 commit f397e48
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
10 changes: 10 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -512,5 +512,15 @@ mod tests {
0u8 => Err(0u8),
Some(42u8) => Ok(Some(42u8)),
}

// See https://github.com/rust-lang/rust/issues/127286 for details.
for (u8, u8) as TupleU8U8 {
(1u8, 2u8) => Ok((1u8, 2u8)),
1u8 => Err(1u8),
}
for (bool, u16) as TupleBoolU16 {
(false, 2u16) => Ok((false, 2u16)),
true => Err(true),
}
}
}
16 changes: 15 additions & 1 deletion src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,29 @@ fn non_static_type_id<T: ?Sized>() -> TypeId {
/// size and layout and that it is safe to do this conversion. Which it probably
/// isn't, unless `T` and `U` are identical.
///
/// # Panics
///
/// This function panics if `T` and `U` have different sizes.
///
/// # Safety
///
/// It is up to the caller to uphold the following invariants:
///
/// - `T` must have the same size as `U`
/// - `T` must have the same alignment as `U`
/// - `T` must be safe to transmute into `U`
#[inline(always)]
pub(crate) unsafe fn transmute_unchecked<T, U>(value: T) -> U {
// Assert is necessary to avoid miscompilation caused by a bug in LLVM.
// Without it `castaway::cast!(123_u8, (u8, u8))` returns `Ok(...)` on
// release build profile. `assert` shouldn't be replaced by `assert_eq`
// because with `assert_eq` Rust 1.70 and 1.71 will still miscompile it.
//
// See https://github.com/rust-lang/rust/issues/127286 for details.
assert!(
mem::size_of::<T>() == mem::size_of::<U>(),
"cannot transmute_unchecked if Dst and Src have different size"
);

let dest = ptr::read(&value as *const T as *const U);
mem::forget(value);
dest
Expand Down

0 comments on commit f397e48

Please sign in to comment.