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

transmute + tuple access + eq on repr(simd)'s inner value seems UB to valgrind #113465

Open
celinval opened this issue Jul 7, 2023 · 8 comments
Labels
A-simd Area: SIMD (Single Instruction Multiple Data) C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way.

Comments

@celinval
Copy link
Contributor

celinval commented Jul 7, 2023

I tried this code:

#![feature(repr_simd)]
use std::mem::transmute;

#[derive(Clone, Debug)]
#[repr(simd)]
struct CustomSimd<T, const LANES: usize>([T; LANES]);

fn main() {
    let mut input : CustomSimd<u8, 10> = CustomSimd([0u8; 10]);
    input.0[0] = u8::MAX;
    let data = unsafe { transmute::<_, CustomSimd<u8, 10>>(input).0 };
    assert_eq!(data, [u8::MAX, 0, 0, 0, 0, 0, 0, 0, 0, 0]);
}

I expected to see this happen: I expected this test to succeed since the destination type is the same as the source type.

Instead, this happened: This test fails. When running it with valgrind, I get errors such as:

Conditional jump or move depends on uninitialised value(s)

Meta

I used the playground version:

Nightly channel
Build using the Nightly version: 1.72.0-nightly
(2023-07-06 85bf07972a1041b9e253)
@celinval celinval added the C-bug Category: This is a bug. label Jul 7, 2023
@celinval
Copy link
Contributor Author

celinval commented Jul 7, 2023

@saethlin
Copy link
Member

saethlin commented Jul 8, 2023

This sounds like a repeat of rust-lang/portable-simd#339 (we don't have an issue for the problem just a PR for a fix, the report is on Zulip)

@workingjubilee
Copy link
Contributor

workingjubilee commented Jul 8, 2023

When we finish removing the SIMD tuples, we should catch attempting to use .N to extract an inner value from these types during lowering.

//
// NOTE: Accessing the inner array directly in any way (e.g. by using the `.0` field syntax) or
// directly constructing an instance of the type (i.e. `let vector = Simd(array)`) should be
// avoided, as it will likely become illegal on `#[repr(simd)]` structs in the future. It also
// causes rustc to emit illegal LLVM IR in some cases.
#[repr(simd)]
pub struct Simd<T, const N: usize>([T; N])
where
LaneCount<N>: SupportedLaneCount,
T: SimdElement;

@workingjubilee workingjubilee added A-simd Area: SIMD (Single Instruction Multiple Data) requires-nightly This issue requires a nightly compiler in some way. labels Jul 8, 2023
@workingjubilee
Copy link
Contributor

@celinval Could you explain your use case for wanting Simd<u8, 10> specifically?

@workingjubilee workingjubilee changed the title Transmuting a repr(simd) structure triggers undefined behavior Tuple access on repr(simd)'s inner value seems UB to valgrind Jul 8, 2023
@workingjubilee workingjubilee changed the title Tuple access on repr(simd)'s inner value seems UB to valgrind transmute + tuple access + eq on repr(simd)'s inner value seems UB to valgrind Jul 8, 2023
@celinval
Copy link
Contributor Author

@celinval Could you explain your use case for wanting Simd<u8, 10> specifically?

Yes, sorry, I know it's a very specialized use case. I am trying to model simd_bitmask intrinsic inside our software verification tool (Kani). I was attempting to create a function that could be used wherever the intrinsic is used.

In my implementation, I was using transmute to extract the content of the Simd structure, so I was trying to transmute the input into a Simd structure I own (with the same layout). The code crashed when I tried to do that though. While debugging, I realized that even transmuting the Simd structure to the original one triggered UB. Hence, this issue.

@celinval
Copy link
Contributor Author

celinval commented Jul 12, 2023

When we finish removing the SIMD tuples, we should catch attempting to use .N to extract an inner value from these types during lowering.

I'm not sure I understand the tuple access part. I'm assuming you mean the member access, right? Is that also being removed?

This sounds like a repeat of rust-lang/portable-simd#339 (we don't have an issue for the problem just a PR for a fix, the report is on Zulip)

Yes, that seems like a similar problem. I will try the workaround from the PR you mentioned. Thanks

edit: @workingjubilee

@workingjubilee
Copy link
Contributor

workingjubilee commented Jul 12, 2023

I'm not sure I understand the tuple access part. I'm assuming you mean the member access, right? Is that also being removed?

Yes, we are discussing it, as repr(simd) has some of the same... "philosophical problems" as repr(packed) did. It isn't immediately unsound, like with repr(packed), which allowed creating references that weren't actually aligned (and alignment is normally a property of references). However, inspecting the inner array of a repr(simd) type directly via member access can easily cause codegen problems which lead to miscompilations.

@celinval
Copy link
Contributor Author

@workingjubilee should I leave this issue open till then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-simd Area: SIMD (Single Instruction Multiple Data) C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way.
Projects
None yet
Development

No branches or pull requests

3 participants