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.

Sign up for GitHub

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

entry: disallow explicit/interior mutability for read-only storage classes. #1009

Merged
merged 1 commit into from
Apr 4, 2023
Merged

entry: disallow explicit/interior mutability for read-only storage classes. #1009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
86 changes: 72 additions & 14 deletions crates/rustc_codegen_spirv/src/codegen_cx/entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand All @@ -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()
),
)
}
Expand Down Expand Up @@ -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(
Expand All @@ -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)
}

Expand Down
17 changes: 17 additions & 0 deletions tests/ui/storage_class/mutability-errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
) {
}
80 changes: 80 additions & 0 deletions tests/ui/storage_class/mutability-errors.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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