From 98c4d6f42e1920cd11c3f7d1016d976a6bbb0d22 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Fri, 27 Sep 2024 17:00:21 -0700 Subject: [PATCH 1/4] [naga] Permit only structs as binding array elements. (#6333) Require `T` to be a struct in `binding_array`; do not permit arrays. In #5428, the validator was changed to accept binding array types that the SPIR-V backend couldn't properly emit. Specifically, the validator was changed to accept `binding_array>`, but the SPIR-V backend wasn't changed to wrap the binding array elements in a SPIR-V struct type, as Vulkan requires. So the type would be accepted by the validator, and then rejected by the backend. --- naga/src/valid/type.rs | 1 - naga/tests/validation.rs | 110 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/naga/src/valid/type.rs b/naga/src/valid/type.rs index 0c44660772..c0c25dab79 100644 --- a/naga/src/valid/type.rs +++ b/naga/src/valid/type.rs @@ -677,7 +677,6 @@ impl super::Validator { // Currently Naga only supports binding arrays of structs for non-handle types. match gctx.types[base].inner { crate::TypeInner::Struct { .. } => {} - crate::TypeInner::Array { .. } => {} _ => return Err(TypeError::BindingArrayBaseTypeNotStruct(base)), }; } diff --git a/naga/tests/validation.rs b/naga/tests/validation.rs index f20ae688b8..b8a30c49b0 100644 --- a/naga/tests/validation.rs +++ b/naga/tests/validation.rs @@ -496,3 +496,113 @@ fn main(input: VertexOutput) {{ } } } + +#[allow(dead_code)] +struct BindingArrayFixture { + module: naga::Module, + span: naga::Span, + ty_u32: naga::Handle, + ty_array: naga::Handle, + ty_struct: naga::Handle, + validator: naga::valid::Validator, +} + +impl BindingArrayFixture { + fn new() -> Self { + let mut module = naga::Module::default(); + let span = naga::Span::default(); + let ty_u32 = module.types.insert( + naga::Type { + name: Some("u32".into()), + inner: naga::TypeInner::Scalar(naga::Scalar::U32), + }, + span, + ); + let ty_array = module.types.insert( + naga::Type { + name: Some("array".into()), + inner: naga::TypeInner::Array { + base: ty_u32, + size: naga::ArraySize::Constant(std::num::NonZeroU32::new(10).unwrap()), + stride: 4, + }, + }, + span, + ); + let ty_struct = module.types.insert( + naga::Type { + name: Some("S".into()), + inner: naga::TypeInner::Struct { + members: vec![naga::StructMember { + name: Some("m".into()), + ty: ty_u32, + binding: None, + offset: 0, + }], + span: 4, + }, + }, + span, + ); + let validator = naga::valid::Validator::new(Default::default(), Default::default()); + BindingArrayFixture { + module, + span, + ty_u32, + ty_array, + ty_struct, + validator, + } + } +} + +#[test] +fn binding_arrays_hold_structs() { + let mut t = BindingArrayFixture::new(); + let _binding_array = t.module.types.insert( + naga::Type { + name: Some("binding_array_of_struct".into()), + inner: naga::TypeInner::BindingArray { + base: t.ty_struct, + size: naga::ArraySize::Dynamic, + }, + }, + t.span, + ); + + assert!(t.validator.validate(&t.module).is_ok()); +} + +#[test] +fn binding_arrays_cannot_hold_arrays() { + let mut t = BindingArrayFixture::new(); + let _binding_array = t.module.types.insert( + naga::Type { + name: Some("binding_array_of_array".into()), + inner: naga::TypeInner::BindingArray { + base: t.ty_array, + size: naga::ArraySize::Dynamic, + }, + }, + t.span, + ); + + assert!(t.validator.validate(&t.module).is_err()); +} + +#[test] +fn binding_arrays_cannot_hold_scalars() { + let mut t = BindingArrayFixture::new(); + let _binding_array = t.module.types.insert( + naga::Type { + name: Some("binding_array_of_scalar".into()), + inner: naga::TypeInner::BindingArray { + base: t.ty_u32, + size: naga::ArraySize::Dynamic, + }, + }, + t.span, + ); + + assert!(t.validator.validate(&t.module).is_err()); +} From 04032905efa67865bd19c95610b755d70127e1c3 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 26 Sep 2024 21:05:24 -0700 Subject: [PATCH 2/4] [naga spv-out] Replace `match` with equivalent `!=`. --- naga/src/back/spv/block.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/naga/src/back/spv/block.rs b/naga/src/back/spv/block.rs index 9fb9485860..f0c3bfa848 100644 --- a/naga/src/back/spv/block.rs +++ b/naga/src/back/spv/block.rs @@ -200,10 +200,7 @@ impl<'w> BlockContext<'w> { fn is_intermediate(&self, expr_handle: Handle) -> bool { match self.ir_function.expressions[expr_handle] { crate::Expression::GlobalVariable(handle) => { - match self.ir_module.global_variables[handle].space { - crate::AddressSpace::Handle => false, - _ => true, - } + self.ir_module.global_variables[handle].space != crate::AddressSpace::Handle } crate::Expression::LocalVariable(_) => true, crate::Expression::FunctionArgument(index) => { From 259592b926ef13d20a1c64b65df392f8cd5c8176 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 26 Sep 2024 21:06:03 -0700 Subject: [PATCH 3/4] [naga spv-out] Update Vulkan spec section number, and provide link. --- naga/src/back/spv/helpers.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/naga/src/back/spv/helpers.rs b/naga/src/back/spv/helpers.rs index 1fb447e384..15c241d44e 100644 --- a/naga/src/back/spv/helpers.rs +++ b/naga/src/back/spv/helpers.rs @@ -85,7 +85,7 @@ impl crate::AddressSpace { /// Return true if the global requires a type decorated with `Block`. /// -/// Vulkan spec v1.3 §15.6.2, "Descriptor Set Interface", says: +/// In the Vulkan spec 1.3.296, the section [Descriptor Set Interface][dsi] says: /// /// > Variables identified with the `Uniform` storage class are used to /// > access transparent buffer backed resources. Such variables must @@ -98,6 +98,8 @@ impl crate::AddressSpace { /// > - laid out explicitly using the `Offset`, `ArrayStride`, and /// > `MatrixStride` decorations as specified in §15.6.4, "Offset /// > and Stride Assignment." +/// +/// [dsi]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#interfaces-resources-descset // See `back::spv::GlobalVariable::access_id` for details. pub fn global_needs_wrapper(ir_module: &crate::Module, var: &crate::GlobalVariable) -> bool { match var.space { From 2021e7f29f0204a0b6b22bc59844af1e93984465 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 26 Sep 2024 21:10:13 -0700 Subject: [PATCH 4/4] [naga spv-out] Document and refactor `write_runtime_array_length`. Document and refactor `naga::back::spv::BlockContext::write_runtime_array_length`. Don't try to handle finding the length of a particular element of a `binding_array>`. The SPIR-V backend doesn't wrap that type correctly anyway; #6333 changes the validator to forbid such types. Instead, assume that the elements of a `binding_array` are always structs whose final members may be a runtime-sized array. Pull out consistency checks after the analysis of the array expression, so that we always carry out all the checks regardless of what path we took to produce the information. --- naga/src/back/spv/index.rs | 174 +++++++++++++++++++++++++------------ 1 file changed, 119 insertions(+), 55 deletions(-) diff --git a/naga/src/back/spv/index.rs b/naga/src/back/spv/index.rs index 84b81b63d2..0295d895b2 100644 --- a/naga/src/back/spv/index.rs +++ b/naga/src/back/spv/index.rs @@ -38,98 +38,162 @@ impl<'w> BlockContext<'w> { /// /// Given `array`, an expression referring a runtime-sized array, return the /// instruction id for the array's length. + /// + /// Runtime-sized arrays may only appear in the values of global + /// variables, which must have one of the following Naga types: + /// + /// 1. A runtime-sized array. + /// 2. A struct whose last member is a runtime-sized array. + /// 3. A binding array of 2. + /// + /// Thus, the expression `array` has the form of: + /// + /// - An optional [`AccessIndex`], for case 2, applied to... + /// - An optional [`Access`] or [`AccessIndex`], for case 3, applied to... + /// - A [`GlobalVariable`]. + /// + /// The SPIR-V generated takes into account wrapped globals; see + /// [`global_needs_wrapper`]. + /// + /// [`GlobalVariable`]: crate::Expression::GlobalVariable + /// [`AccessIndex`]: crate::Expression::AccessIndex + /// [`Access`]: crate::Expression::Access + /// [`base`]: crate::Expression::Access::base pub(super) fn write_runtime_array_length( &mut self, array: Handle, block: &mut Block, ) -> Result { - // Naga IR permits runtime-sized arrays as global variables, or as the - // final member of a struct that is a global variable, or one of these - // inside a buffer that is itself an element in a buffer bindings array. - // SPIR-V requires that runtime-sized arrays are wrapped in structs. - // See `helpers::global_needs_wrapper` and its uses. - let (opt_array_index_id, global_handle, opt_last_member_index) = match self - .ir_function - .expressions[array] - { + // The index into the binding array, if any. + let binding_array_index_id: Option; + + // The handle to the Naga IR global we're referring to. + let global_handle: Handle; + + // At the Naga type level, if the runtime-sized array is the final member of a + // struct, this is that member's index. + // + // This does not cover wrappers: if this backend wrapped the Naga global's + // type in a synthetic SPIR-V struct (see `global_needs_wrapper`), this is + // `None`. + let opt_last_member_index: Option; + + // Inspect `array` and decide whether we have a binding array and/or an + // enclosing struct. + match self.ir_function.expressions[array] { crate::Expression::AccessIndex { base, index } => { match self.ir_function.expressions[base] { - // The global variable is an array of buffer bindings of structs, - // we are accessing one of them with a static index, - // and the last member of it. crate::Expression::AccessIndex { base: base_outer, index: index_outer, } => match self.ir_function.expressions[base_outer] { + // An `AccessIndex` of an `AccessIndex` must be a + // binding array holding structs whose last members are + // runtime-sized arrays. crate::Expression::GlobalVariable(handle) => { let index_id = self.get_index_constant(index_outer); - (Some(index_id), handle, Some(index)) + binding_array_index_id = Some(index_id); + global_handle = handle; + opt_last_member_index = Some(index); + } + _ => { + return Err(Error::Validation( + "array length expression: AccessIndex(AccessIndex(Global))", + )) } - _ => return Err(Error::Validation("array length expression case-1a")), }, - // The global variable is an array of buffer bindings of structs, - // we are accessing one of them with a dynamic index, - // and the last member of it. crate::Expression::Access { base: base_outer, index: index_outer, } => match self.ir_function.expressions[base_outer] { + // Similarly, an `AccessIndex` of an `Access` must be a + // binding array holding structs whose last members are + // runtime-sized arrays. crate::Expression::GlobalVariable(handle) => { let index_id = self.cached[index_outer]; - (Some(index_id), handle, Some(index)) + binding_array_index_id = Some(index_id); + global_handle = handle; + opt_last_member_index = Some(index); + } + _ => { + return Err(Error::Validation( + "array length expression: AccessIndex(Access(Global))", + )) } - _ => return Err(Error::Validation("array length expression case-1b")), }, - // The global variable is a buffer, and we are accessing the last member. crate::Expression::GlobalVariable(handle) => { - let global = &self.ir_module.global_variables[handle]; - match self.ir_module.types[global.ty].inner { - // The global variable is an array of buffer bindings of run-time arrays. - crate::TypeInner::BindingArray { .. } => (Some(index), handle, None), - // The global variable is a struct, and we are accessing the last member - _ => (None, handle, Some(index)), - } + // An outer `AccessIndex` applied directly to a + // `GlobalVariable`. Since binding arrays can only contain + // structs, this must be referring to the last member of a + // struct that is a runtime-sized array. + binding_array_index_id = None; + global_handle = handle; + opt_last_member_index = Some(index); } - _ => return Err(Error::Validation("array length expression case-1c")), - } - } - // The global variable is an array of buffer bindings of arrays. - crate::Expression::Access { base, index } => match self.ir_function.expressions[base] { - crate::Expression::GlobalVariable(handle) => { - let index_id = self.cached[index]; - let global = &self.ir_module.global_variables[handle]; - match self.ir_module.types[global.ty].inner { - crate::TypeInner::BindingArray { .. } => (Some(index_id), handle, None), - _ => return Err(Error::Validation("array length expression case-2a")), + _ => { + return Err(Error::Validation( + "array length expression: AccessIndex()", + )) } } - _ => return Err(Error::Validation("array length expression case-2b")), - }, - // The global variable is a run-time array. + } crate::Expression::GlobalVariable(handle) => { - let global = &self.ir_module.global_variables[handle]; - if !global_needs_wrapper(self.ir_module, global) { - return Err(Error::Validation("array length expression case-3")); - } - (None, handle, None) + // A direct reference to a global variable. This must hold the + // runtime-sized array directly. + binding_array_index_id = None; + global_handle = handle; + opt_last_member_index = None; } _ => return Err(Error::Validation("array length expression case-4")), }; + // The verifier should have checked this, but make sure the inspection above + // agrees with the type about whether a binding array is involved. + // + // Eventually we do want to support `binding_array>`. This check + // ensures that whoever relaxes the validator will get an error message from + // us, not just bogus SPIR-V. + let global = &self.ir_module.global_variables[global_handle]; + match ( + &self.ir_module.types[global.ty].inner, + binding_array_index_id, + ) { + (&crate::TypeInner::BindingArray { .. }, Some(_)) => {} + (_, None) => {} + _ => { + return Err(Error::Validation( + "array length expression: bad binding array inference", + )) + } + } + + // SPIR-V allows runtime-sized arrays to appear only as the last member of a + // struct. Determine this member's index. let gvar = self.writer.global_variables[global_handle].clone(); let global = &self.ir_module.global_variables[global_handle]; - let (last_member_index, gvar_id) = match opt_last_member_index { - Some(index) => (index, gvar.access_id), - None => { - if !global_needs_wrapper(self.ir_module, global) { - return Err(Error::Validation( - "pointer to a global that is not a wrapped array", - )); - } + let needs_wrapper = global_needs_wrapper(self.ir_module, global); + let (last_member_index, gvar_id) = match (opt_last_member_index, needs_wrapper) { + (Some(index), false) => { + // At the Naga type level, the runtime-sized array appears as the + // final member of a struct, whose index is `index`. We didn't need to + // wrap this, since the Naga type meets SPIR-V's requirements already. + (index, gvar.access_id) + } + (None, true) => { + // At the Naga type level, the runtime-sized array does not appear + // within a struct. We wrapped this in an OpTypeStruct with nothing + // else in it, so the index is zero. OpArrayLength wants the pointer + // to the wrapper struct, so use `gvar.var_id`. (0, gvar.var_id) } + _ => { + return Err(Error::Validation( + "array length expression: bad SPIR-V wrapper struct inference", + )); + } }; - let structure_id = match opt_array_index_id { + + let structure_id = match binding_array_index_id { // We are indexing inside a binding array, generate the access op. Some(index_id) => { let element_type_id = match self.ir_module.types[global.ty].inner {