diff --git a/CHANGELOG.md b/CHANGELOG.md index 69ee3f87d3..7bddbb4542 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed 🩹 +- [PR#1009](https://github.com/EmbarkStudios/rust-gpu/pull/1009) fixed [#1008](https://github.com/EmbarkStudios/rust-gpu/issues/1008) by reinstating mutability checks for entry-point parameters pointing into read-only storage classes (e.g. `#[spirv(uniform)] x: &mut u32` is now again an error) - [PR#995](https://github.com/EmbarkStudios/rust-gpu/pull/995) fixed [#994](https://github.com/EmbarkStudios/rust-gpu/issues/994) by using `OpAtomicFAddEXT` instead of `OpAtomicFMaxEXT` in `atomic_f_add`. ## [0.6.1] diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs b/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs index 93a720492f..50c2ab0a62 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs @@ -10,6 +10,7 @@ use rspirv::spirv::{ }; use rustc_codegen_ssa::traits::{BaseTypeMethods, BuilderMethods}; use rustc_data_structures::fx::FxHashMap; +use rustc_errors::MultiSpan; use rustc_hir as hir; use rustc_middle::span_bug; use rustc_middle::ty::layout::{LayoutOf, TyAndLayout}; @@ -180,14 +181,38 @@ impl<'tcx> CodegenCx<'tcx> { attrs: &AggregatedSpirvAttributes, ) -> (Word, StorageClass) { // FIXME(eddyb) attribute validation should be done ahead of time. - // FIXME(eddyb) also take into account `&T` interior mutability, - // i.e. it's only immutable if `T: Freeze`, which we should check. // FIXME(eddyb) also check the type for compatibility with being // part of the interface, including potentially `Sync`ness etc. - let (value_ty, mutbl, is_ref) = match *layout.ty.kind() { + // FIXME(eddyb) really need to require `T: Sync` for references + // (especially relevant with interior mutability!). + let (value_ty, explicit_mutbl, is_ref) = match *layout.ty.kind() { ty::Ref(_, pointee_ty, mutbl) => (pointee_ty, mutbl, true), _ => (layout.ty, hir::Mutability::Not, false), }; + let effective_mutbl = match explicit_mutbl { + // NOTE(eddyb) `T: !Freeze` used to detect "`T` has interior mutability" + // (i.e. "`&T` is a shared+mutable reference"), more specifically `T` + // containing `UnsafeCell` (but not behind any indirection), which + // includes many safe abstractions (e.g. `Cell`, `RefCell`, `Atomic*`). + hir::Mutability::Not + if is_ref && !value_ty.is_freeze(self.tcx, ty::ParamEnv::reveal_all()) => + { + hir::Mutability::Mut + } + _ => explicit_mutbl, + }; + // FIXME(eddyb) this should maybe be an extension method on `StorageClass`? + let expected_mutbl_for = |storage_class| match storage_class { + StorageClass::UniformConstant + | StorageClass::Input + | StorageClass::Uniform + | StorageClass::PushConstant => hir::Mutability::Not, + + // FIXME(eddyb) further categorize this by which want interior + // mutability (+`Sync`!), likely almost all of them, and which + // can be per-lane-owning `&mut T`. + _ => hir::Mutability::Mut, + }; let spirv_ty = self.layout_of(value_ty).spirv_type(hir_param.ty_span, self); // Some types automatically specify a storage class. Compute that here. let element_ty = match self.lookup_type(spirv_ty) { @@ -220,15 +245,6 @@ impl<'tcx> CodegenCx<'tcx> { let attr_storage_class = attrs.storage_class.map(|storage_class_attr| { let storage_class = storage_class_attr.value; - let expected_mutbl = match storage_class { - StorageClass::UniformConstant - | StorageClass::Input - | StorageClass::Uniform - | StorageClass::PushConstant => hir::Mutability::Not, - - _ => hir::Mutability::Mut, - }; - if !is_ref { self.tcx.sess.span_fatal( hir_param.ty_span, @@ -237,7 +253,7 @@ impl<'tcx> CodegenCx<'tcx> { (expected `&{}T`)", layout.ty, storage_class, - expected_mutbl.prefix_str() + expected_mutbl_for(storage_class).prefix_str() ), ) } @@ -275,7 +291,7 @@ impl<'tcx> CodegenCx<'tcx> { // If storage class was not inferred nor specified, compute the default (i.e. input/output) let storage_class = inferred_storage_class_from_ty .or(attr_storage_class) - .unwrap_or_else(|| match (is_ref, mutbl) { + .unwrap_or_else(|| match (is_ref, explicit_mutbl) { (false, _) => StorageClass::Input, (true, hir::Mutability::Mut) => StorageClass::Output, (true, hir::Mutability::Not) => self.tcx.sess.span_fatal( @@ -287,6 +303,48 @@ impl<'tcx> CodegenCx<'tcx> { ), }); + // Validate reference mutability against the *final* storage class. + if is_ref { + // FIXME(eddyb) booleans make the condition a bit more readable. + let ref_is_read_only = effective_mutbl == hir::Mutability::Not; + let storage_class_requires_read_only = + expected_mutbl_for(storage_class) == hir::Mutability::Not; + if !ref_is_read_only && storage_class_requires_read_only { + let mut err = self.tcx.sess.struct_span_err( + hir_param.ty_span, + format!( + "entry-point requires {}...", + match explicit_mutbl { + hir::Mutability::Not => "interior mutability", + hir::Mutability::Mut => "a mutable reference", + } + ), + ); + { + let note_message = + format!("...but storage class `{storage_class:?}` is read-only"); + let (note_label_span, note_label) = + if let Some(storage_class_attr) = attrs.storage_class { + ( + storage_class_attr.span, + format!("`{storage_class:?}` specified in attribute"), + ) + } else { + ( + hir_param.ty_span, + format!("`{storage_class:?}` implied by type"), + ) + }; + // HACK(eddyb) have to use `MultiSpan` directly for labels, + // as there's no `span_label` equivalent for `span_note`s. + let mut note_multi_span: MultiSpan = vec![note_label_span].into(); + note_multi_span.push_span_label(note_label_span, note_label); + err.span_note(note_multi_span, note_message); + } + err.emit(); + } + } + (spirv_ty, storage_class) } diff --git a/tests/ui/storage_class/mutability-errors.rs b/tests/ui/storage_class/mutability-errors.rs new file mode 100644 index 0000000000..e31ed99c80 --- /dev/null +++ b/tests/ui/storage_class/mutability-errors.rs @@ -0,0 +1,17 @@ +// Tests that using `&mut` (or interior mutability) with read-only storage classes +// does actually error (see `mutability-errors.stderr` for the error messages). +// build-fail + +use core::sync::atomic::AtomicU32; +use spirv_std::{image::Image2d, spirv}; + +#[spirv(fragment)] +pub fn main( + #[spirv(descriptor_set = 0, binding = 0)] implicit_uniform_constant_mut: &mut Image2d, + #[spirv(uniform_constant, descriptor_set = 0, binding = 0)] uniform_constant_mut: &mut Image2d, + #[spirv(uniform, descriptor_set = 0, binding = 0)] uniform_mut: &mut u32, + #[spirv(uniform, descriptor_set = 0, binding = 0)] uniform_interior_mut: &AtomicU32, + #[spirv(push_constant)] push_constant_mut: &mut u32, + #[spirv(push_constant)] push_constant_interior_mut: &AtomicU32, +) { +} diff --git a/tests/ui/storage_class/mutability-errors.stderr b/tests/ui/storage_class/mutability-errors.stderr new file mode 100644 index 0000000000..4ed42a0b45 --- /dev/null +++ b/tests/ui/storage_class/mutability-errors.stderr @@ -0,0 +1,80 @@ +error: entry-point requires a mutable reference... + --> $DIR/mutability-errors.rs:10:78 + | +10 | #[spirv(descriptor_set = 0, binding = 0)] implicit_uniform_constant_mut: &mut Image2d, + | ^^^^^^^^^^^^ + | +note: ...but storage class `UniformConstant` is read-only + --> $DIR/mutability-errors.rs:10:78 + | +10 | #[spirv(descriptor_set = 0, binding = 0)] implicit_uniform_constant_mut: &mut Image2d, + | ^^^^^^^^^^^^ `UniformConstant` implied by type + +warning: redundant storage class specifier, storage class is inferred from type + --> $DIR/mutability-errors.rs:11:13 + | +11 | #[spirv(uniform_constant, descriptor_set = 0, binding = 0)] uniform_constant_mut: &mut Image2d, + | ^^^^^^^^^^^^^^^^ + +error: entry-point requires a mutable reference... + --> $DIR/mutability-errors.rs:11:87 + | +11 | #[spirv(uniform_constant, descriptor_set = 0, binding = 0)] uniform_constant_mut: &mut Image2d, + | ^^^^^^^^^^^^ + | +note: ...but storage class `UniformConstant` is read-only + --> $DIR/mutability-errors.rs:11:13 + | +11 | #[spirv(uniform_constant, descriptor_set = 0, binding = 0)] uniform_constant_mut: &mut Image2d, + | ^^^^^^^^^^^^^^^^ `UniformConstant` specified in attribute + +error: entry-point requires a mutable reference... + --> $DIR/mutability-errors.rs:12:69 + | +12 | #[spirv(uniform, descriptor_set = 0, binding = 0)] uniform_mut: &mut u32, + | ^^^^^^^^ + | +note: ...but storage class `Uniform` is read-only + --> $DIR/mutability-errors.rs:12:13 + | +12 | #[spirv(uniform, descriptor_set = 0, binding = 0)] uniform_mut: &mut u32, + | ^^^^^^^ `Uniform` specified in attribute + +error: entry-point requires interior mutability... + --> $DIR/mutability-errors.rs:13:78 + | +13 | #[spirv(uniform, descriptor_set = 0, binding = 0)] uniform_interior_mut: &AtomicU32, + | ^^^^^^^^^^ + | +note: ...but storage class `Uniform` is read-only + --> $DIR/mutability-errors.rs:13:13 + | +13 | #[spirv(uniform, descriptor_set = 0, binding = 0)] uniform_interior_mut: &AtomicU32, + | ^^^^^^^ `Uniform` specified in attribute + +error: entry-point requires a mutable reference... + --> $DIR/mutability-errors.rs:14:48 + | +14 | #[spirv(push_constant)] push_constant_mut: &mut u32, + | ^^^^^^^^ + | +note: ...but storage class `PushConstant` is read-only + --> $DIR/mutability-errors.rs:14:13 + | +14 | #[spirv(push_constant)] push_constant_mut: &mut u32, + | ^^^^^^^^^^^^^ `PushConstant` specified in attribute + +error: entry-point requires interior mutability... + --> $DIR/mutability-errors.rs:15:57 + | +15 | #[spirv(push_constant)] push_constant_interior_mut: &AtomicU32, + | ^^^^^^^^^^ + | +note: ...but storage class `PushConstant` is read-only + --> $DIR/mutability-errors.rs:15:13 + | +15 | #[spirv(push_constant)] push_constant_interior_mut: &AtomicU32, + | ^^^^^^^^^^^^^ `PushConstant` specified in attribute + +error: aborting due to 6 previous errors; 1 warning emitted +