inline asm!: ban OpReturn/OpReturnValue (they're always UB).

CHANGELOG.md

@@ -27,6 +27,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 -->
 
+## [Unreleased]
+
+### Fixed 🩹
+- [PR#1006](https://github.com/EmbarkStudios/rust-gpu/pull/1006) fixed [#1002](https://github.com/EmbarkStudios/rust-gpu/issues/1002) by rewriting away all `spirv-std` uses of `asm!("OpReturnValue %result")` and disallowing `OpReturn`/`OpReturnValue` from inline `asm!` (as it's always UB to leave `asm!` blocks in any way other than falling through their end)
+
 ## [0.6.0]
 
 ### Added ⭐

crates/rustc_codegen_spirv/src/abi.rs

@@ -593,6 +593,8 @@ fn dig_scalar_pointee<'tcx>(
     })
 }
 
+// FIXME(eddyb) all `ty: TyAndLayout` variables should be `layout: TyAndLayout`,
+// the type is really more "Layout with Ty" (`.ty` field + `Deref`s to `Layout`).
 fn trans_aggregate<'tcx>(cx: &CodegenCx<'tcx>, span: Span, ty: TyAndLayout<'tcx>) -> Word {
     fn create_zst<'tcx>(cx: &CodegenCx<'tcx>, span: Span, ty: TyAndLayout<'tcx>) -> Word {
         SpirvType::Adt {
@@ -613,16 +615,22 @@ fn trans_aggregate<'tcx>(cx: &CodegenCx<'tcx>, span: Span, ty: TyAndLayout<'tcx>
         ),
         FieldsShape::Union(_) => {
             assert!(!ty.is_unsized(), "{ty:#?}");
-            if ty.size.bytes() == 0 {
-                create_zst(cx, span, ty)
+
+            // Represent the `union` with its largest case, which should work
+            // for at least `MaybeUninit<T>` (which is between `T` and `()`),
+            // but also potentially some other ones as well.
+            // NOTE(eddyb) even if long-term this may become a byte array, that
+            // only works for "data types" and not "opaque handles" (images etc.).
+            let largest_case = (0..ty.fields.count())
+                .map(|i| ty.field(cx, i))
+                .max_by_key(|case| case.size);
+
+            if let Some(case) = largest_case {
+                assert_eq!(ty.size, case.size);
+                case.spirv_type(span, cx)
             } else {
-                let byte = SpirvType::Integer(8, false).def(span, cx);
-                let count = cx.constant_u32(span, ty.size.bytes() as u32);
-                SpirvType::Array {
-                    element: byte,
-                    count,
-                }
-                .def(span, cx)
+                assert_eq!(ty.size, Size::ZERO);
+                create_zst(cx, span, ty)
             }
         }
         FieldsShape::Array { stride, count } => {

crates/rustc_codegen_spirv/src/builder/spirv_asm.rs

@@ -163,7 +163,7 @@ impl<'a, 'tcx> AsmBuilderMethods<'tcx> for Builder<'a, 'tcx> {
             (false, AsmBlock::Open) => (),
             (false, AsmBlock::End(terminator)) => {
                 self.err(&format!(
-                    "trailing terminator {terminator:?} requires `options(noreturn)`"
+                    "trailing terminator `Op{terminator:?}` requires `options(noreturn)`"
                 ));
             }
         }
@@ -356,6 +356,19 @@ impl<'cx, 'tcx> Builder<'cx, 'tcx> {
             }
 
             op => {
+                // NOTE(eddyb) allowing the instruction to be added below avoids
+                // spurious "`noreturn` requires a terminator at the end" errors.
+                if let Op::Return | Op::ReturnValue = op {
+                    self.struct_err(&format!(
+                        "using `Op{op:?}` to return from within `asm!` is disallowed"
+                    ))
+                    .note(
+                        "resuming execution, without falling through the end \
+                        of the `asm!` block, is always undefined behavior",
+                    )
+                    .emit();
+                }
+
                 self.emit()
                     .insert_into_block(dr::InsertPoint::End, inst)
                     .unwrap();
@@ -370,7 +383,9 @@ impl<'cx, 'tcx> Builder<'cx, 'tcx> {
                     }
                     AsmBlock::End(terminator) => {
                         if op != Op::Label {
-                            self.err(&format!("expected OpLabel after terminator {terminator:?}"));
+                            self.err(&format!(
+                                "expected `OpLabel` after terminator `Op{terminator:?}`"
+                            ));
                         }
 
                         AsmBlock::Open

crates/spirv-std/src/arch.rs

@@ -250,15 +250,18 @@ pub trait IndexUnchecked<T> {
 impl<T> IndexUnchecked<T> for [T] {
     #[cfg(target_arch = "spirv")]
     unsafe fn index_unchecked(&self, index: usize) -> &T {
-        asm!(
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
+        asm! {
             "%slice_ptr = OpLoad _ {slice_ptr_ptr}",
             "%data_ptr = OpCompositeExtract _ %slice_ptr 0",
-            "%val_ptr = OpAccessChain _ %data_ptr {index}",
-            "OpReturnValue %val_ptr",
+            "%result = OpAccessChain _ %data_ptr {index}",
+            "OpStore {result_slot} %result",
             slice_ptr_ptr = in(reg) &self,
             index = in(reg) index,
-            options(noreturn)
-        )
+            result_slot = in(reg) result_slot.as_mut_ptr(),
+        }
+        result_slot.assume_init()
     }
 
     #[cfg(not(target_arch = "spirv"))]
@@ -268,15 +271,18 @@ impl<T> IndexUnchecked<T> for [T] {
 
     #[cfg(target_arch = "spirv")]
     unsafe fn index_unchecked_mut(&mut self, index: usize) -> &mut T {
-        asm!(
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
+        asm! {
             "%slice_ptr = OpLoad _ {slice_ptr_ptr}",
             "%data_ptr = OpCompositeExtract _ %slice_ptr 0",
-            "%val_ptr = OpAccessChain _ %data_ptr {index}",
-            "OpReturnValue %val_ptr",
+            "%result = OpAccessChain _ %data_ptr {index}",
+            "OpStore {result_slot} %result",
             slice_ptr_ptr = in(reg) &self,
             index = in(reg) index,
-            options(noreturn)
-        )
+            result_slot = in(reg) result_slot.as_mut_ptr(),
+        }
+        result_slot.assume_init()
     }
 
     #[cfg(not(target_arch = "spirv"))]
@@ -288,13 +294,16 @@ impl<T> IndexUnchecked<T> for [T] {
 impl<T, const N: usize> IndexUnchecked<T> for [T; N] {
     #[cfg(target_arch = "spirv")]
     unsafe fn index_unchecked(&self, index: usize) -> &T {
-        asm!(
-            "%val_ptr = OpAccessChain _ {array_ptr} {index}",
-            "OpReturnValue %val_ptr",
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
+        asm! {
+            "%result = OpAccessChain _ {array_ptr} {index}",
+            "OpStore {result_slot} %result",
             array_ptr = in(reg) self,
             index = in(reg) index,
-            options(noreturn)
-        )
+            result_slot = in(reg) result_slot.as_mut_ptr(),
+        }
+        result_slot.assume_init()
     }
 
     #[cfg(not(target_arch = "spirv"))]
@@ -304,13 +313,16 @@ impl<T, const N: usize> IndexUnchecked<T> for [T; N] {
 
     #[cfg(target_arch = "spirv")]
     unsafe fn index_unchecked_mut(&mut self, index: usize) -> &mut T {
-        asm!(
-            "%val_ptr = OpAccessChain _ {array_ptr} {index}",
-            "OpReturnValue %val_ptr",
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
+        asm! {
+            "%result = OpAccessChain _ {array_ptr} {index}",
+            "OpStore {result_slot} %result",
             array_ptr = in(reg) self,
             index = in(reg) index,
-            options(noreturn)
-        )
+            result_slot = in(reg) result_slot.as_mut_ptr(),
+        }
+        result_slot.assume_init()
     }
 
     #[cfg(not(target_arch = "spirv"))]

crates/spirv-std/src/image.rs

@@ -94,6 +94,8 @@ pub type Cubemap = crate::Image!(cube, type=f32, sampled, __crate_root=crate);
 /// See SPIR-V OpTypeImage specification for the meaning of integer parameters.
 #[spirv(generic_image_type)]
 #[derive(Copy, Clone)]
+// HACK(eddyb) avoids "transparent newtype of `_anti_zst_padding`" misinterpretation.
+#[repr(C)]
 pub struct Image<
     SampledType: SampleType<FORMAT>,
     const DIM: u32,          // Dimensionality,
@@ -103,7 +105,9 @@ pub struct Image<
     const SAMPLED: u32,      // Sampled,
     const FORMAT: u32,       // ImageFormat,
 > {
-    _x: u32,
+    // HACK(eddyb) avoids the layout becoming ZST (and being elided in one way
+    // or another, before `#[spirv(generic_image_type)]` can special-case it).
+    _anti_zst_padding: core::mem::MaybeUninit<u32>,
     _marker: core::marker::PhantomData<SampledType>,
 }
 

crates/spirv-std/src/ray_tracing.rs

@@ -7,8 +7,12 @@ use core::arch::asm;
 /// acceleration structure handle as defined in the client API specification.
 #[spirv(acceleration_structure)]
 #[derive(Copy, Clone)]
+// HACK(eddyb) avoids "transparent newtype of `_anti_zst_padding`" misinterpretation.
+#[repr(C)]
 pub struct AccelerationStructure {
-    pub(crate) _private: u32,
+    // HACK(eddyb) avoids the layout becoming ZST (and being elided in one way
+    // or another, before `#[spirv(acceleration_structure)]` can special-case it).
+    _anti_zst_padding: core::mem::MaybeUninit<u32>,
 }
 
 impl AccelerationStructure {
@@ -19,16 +23,16 @@ impl AccelerationStructure {
     #[doc(alias = "OpConvertUToAccelerationStructureKHR")]
     #[inline]
     pub unsafe fn from_u64(id: u64) -> AccelerationStructure {
-        // Since we can't represent an uninitalized opaque type in Rust at the
-        // moment, we need to create and return the acceleration structure entirely
-        // in assembly.
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
         asm! {
             "%ret = OpTypeAccelerationStructureKHR",
             "%result = OpConvertUToAccelerationStructureKHR %ret {id}",
-            "OpReturnValue %result",
+            "OpStore {result_slot} %result",
             id = in(reg) id,
-            options(noreturn)
+            result_slot = in(reg) result_slot.as_mut_ptr(),
         }
+        result_slot.assume_init()
     }
 
     /// Converts a vector of two 32 bit integers into an [`AccelerationStructure`].
@@ -38,17 +42,17 @@ impl AccelerationStructure {
     #[doc(alias = "OpConvertUToAccelerationStructureKHR")]
     #[inline]
     pub unsafe fn from_vec(id: impl Vector<u32, 2>) -> AccelerationStructure {
-        // Since we can't represent an uninitalized opaque type in Rust at the
-        // moment, we need to create and return the acceleration structure entirely
-        // in assembly.
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
         asm! {
             "%ret = OpTypeAccelerationStructureKHR",
             "%id = OpLoad _ {id}",
             "%result = OpConvertUToAccelerationStructureKHR %ret %id",
-            "OpReturnValue %result",
+            "OpStore {result_slot} %result",
             id = in(reg) &id,
-            options(noreturn),
+            result_slot = in(reg) result_slot.as_mut_ptr(),
         }
+        result_slot.assume_init()
     }
 
     #[spirv_std_macros::gpu_only]
@@ -186,8 +190,12 @@ pub enum CommittedIntersection {
 
 /// A ray query type which is an opaque object representing a ray traversal.
 #[spirv(ray_query)]
+// HACK(eddyb) avoids "transparent newtype of `_anti_zst_padding`" misinterpretation.
+#[repr(C)]
 pub struct RayQuery {
-    _private: u32,
+    // HACK(eddyb) avoids the layout becoming ZST (and being elided in one way
+    // or another, before `#[spirv(ray_query)]` can special-case it).
+    _anti_zst_padding: core::mem::MaybeUninit<u32>,
 }
 
 /// Constructs an uninitialized ray query variable. Using the syntax

crates/spirv-std/src/runtime_array.rs

@@ -7,9 +7,12 @@ use core::marker::PhantomData;
 /// Hence, this type represents something very similar to a slice, but with no way of knowing its
 /// length.
 #[spirv(runtime_array)]
+// HACK(eddyb) avoids "transparent newtype of `_anti_zst_padding`" misinterpretation.
+#[repr(C)]
 pub struct RuntimeArray<T> {
-    // spooky! this field does not exist, so if it's referenced in rust code, things will explode
-    _do_not_touch: u32,
+    // HACK(eddyb) avoids the layout becoming ZST (and being elided in one way
+    // or another, before `#[spirv(runtime_array)]` can special-case it).
+    _anti_zst_padding: core::mem::MaybeUninit<u32>,
     _phantom: PhantomData<T>,
 }
 
@@ -25,13 +28,16 @@ impl<T> RuntimeArray<T> {
     /// and lead to UB.
     #[spirv_std_macros::gpu_only]
     pub unsafe fn index(&self, index: usize) -> &T {
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
         asm! {
             "%result = OpAccessChain _ {arr} {index}",
-            "OpReturnValue %result",
+            "OpStore {result_slot} %result",
             arr = in(reg) self,
             index = in(reg) index,
-            options(noreturn),
+            result_slot = in(reg) result_slot.as_mut_ptr(),
         }
+        result_slot.assume_init()
     }
 
     /// Index the array, returning a mutable reference to an element. Unfortunately, because the
@@ -43,12 +49,15 @@ impl<T> RuntimeArray<T> {
     /// and lead to UB.
     #[spirv_std_macros::gpu_only]
     pub unsafe fn index_mut(&mut self, index: usize) -> &mut T {
+        // FIXME(eddyb) `let mut result = T::default()` uses (for `asm!`), with this.
+        let mut result_slot = core::mem::MaybeUninit::uninit();
         asm! {
             "%result = OpAccessChain _ {arr} {index}",
-            "OpReturnValue %result",
+            "OpStore {result_slot} %result",
             arr = in(reg) self,
             index = in(reg) index,
-            options(noreturn),
+            result_slot = in(reg) result_slot.as_mut_ptr(),
         }
+        result_slot.assume_init()
     }
 }

crates/spirv-std/src/sampler.rs

@@ -2,6 +2,10 @@
 /// sample an image.
 #[spirv(sampler)]
 #[derive(Copy, Clone)]
+// HACK(eddyb) avoids "transparent newtype of `_anti_zst_padding`" misinterpretation.
+#[repr(C)]
 pub struct Sampler {
-    _x: u32,
+    // HACK(eddyb) avoids the layout becoming ZST (and being elided in one way
+    // or another, before `#[spirv(sampler)]` can special-case it).
+    _anti_zst_padding: core::mem::MaybeUninit<u32>,
 }

tests/ui/image/gather_err.stderr

@@ -9,9 +9,9 @@ error[E0277]: the trait bound `Image<f32, 0, 2, 0, 0, 1, 0>: HasGather` is not s
               Image<SampledType, 3, DEPTH, ARRAYED, 0, SAMPLED, FORMAT>
               Image<SampledType, 4, DEPTH, ARRAYED, 0, SAMPLED, FORMAT>
 note: required by a bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, spirv_std::::image::{impl#1}::{constant#0}, SAMPLED, FORMAT>::gather`
-   --> $SPIRV_STD_SRC/image.rs:163:15
+   --> $SPIRV_STD_SRC/image.rs:167:15
     |
-163 |         Self: HasGather,
+167 |         Self: HasGather,
     |               ^^^^^^^^^ required by this bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, spirv_std::::image::{impl#1}::{constant#0}, SAMPLED, FORMAT>::gather`
 
 error[E0277]: the trait bound `Image<f32, 2, 2, 0, 0, 1, 0>: HasGather` is not satisfied
@@ -25,9 +25,9 @@ error[E0277]: the trait bound `Image<f32, 2, 2, 0, 0, 1, 0>: HasGather` is not s
               Image<SampledType, 3, DEPTH, ARRAYED, 0, SAMPLED, FORMAT>
               Image<SampledType, 4, DEPTH, ARRAYED, 0, SAMPLED, FORMAT>
 note: required by a bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, spirv_std::::image::{impl#1}::{constant#0}, SAMPLED, FORMAT>::gather`
-   --> $SPIRV_STD_SRC/image.rs:163:15
+   --> $SPIRV_STD_SRC/image.rs:167:15
     |
-163 |         Self: HasGather,
+167 |         Self: HasGather,
     |               ^^^^^^^^^ required by this bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, spirv_std::::image::{impl#1}::{constant#0}, SAMPLED, FORMAT>::gather`
 
 error: aborting due to 2 previous errors

tests/ui/image/query/query_levels_err.stderr

@@ -10,9 +10,9 @@ error[E0277]: the trait bound `Image<f32, 4, 2, 0, 0, 1, 0>: HasQueryLevels` is
               Image<SampledType, 2, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>
               Image<SampledType, 3, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>
 note: required by a bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>::query_levels`
-   --> $SPIRV_STD_SRC/image.rs:813:15
+   --> $SPIRV_STD_SRC/image.rs:817:15
     |
-813 |         Self: HasQueryLevels,
+817 |         Self: HasQueryLevels,
     |               ^^^^^^^^^^^^^^ required by this bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>::query_levels`
 
 error: aborting due to previous error

tests/ui/image/query/query_lod_err.stderr

@@ -10,9 +10,9 @@ error[E0277]: the trait bound `Image<f32, 4, 2, 0, 0, 1, 0>: HasQueryLevels` is
               Image<SampledType, 2, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>
               Image<SampledType, 3, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>
 note: required by a bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>::query_lod`
-   --> $SPIRV_STD_SRC/image.rs:839:15
+   --> $SPIRV_STD_SRC/image.rs:843:15
     |
-839 |         Self: HasQueryLevels,
+843 |         Self: HasQueryLevels,
     |               ^^^^^^^^^^^^^^ required by this bound in `Image::<SampledType, DIM, DEPTH, ARRAYED, MULTISAMPLED, SAMPLED, FORMAT>::query_lod`
 
 error: aborting due to previous error