Skip to content

Commit c4365b9

Browse files
andyleisersonjimblandy
andyleiserson
and
jimblandy
authored
fix(naga): Improve validation of non-constructible types (gfx-rs#8873)
* fix(naga): Improve validation of non-constructible types Fixes gfx-rs#4720 Fixes the test cases in gfx-rs#7393, but not the issue itself * Zero-constructing texture_2d is no longer caught by front end. * Define `TypeInner::is_constructible`; use it in `front::wgsl`. Add a new method `TypeInner::is_constructible`, that determines whether WGSL considers a type [constructible]. Use this to generate errors in the WGSL front end. Assert in validation that it matches the judgment of `Validator::validate_type`. [constructible]: https://gpuweb.github.io/gpuweb/wgsl/#constructible-types --------- Co-authored-by: Jim Blandy <[email protected]>
1 parent 8f50bb9 commit c4365b9

10 files changed

Lines changed: 597 additions & 93 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ By @cwfitzgerald in [#8999](https://github.com/gfx-rs/wgpu/pull/8999).
130130
- Check that if the shader outputs `frag_depth`, then the pipeline must have a depth attachment. By @andyleiserson in [#8856](https://github.com/gfx-rs/wgpu/pull/8856).
131131
- Fix incorrect acceptance of some swizzle selectors that are not valid for their operand, e.g. `const v = vec2<i32>(); let r = v.xyz`. By @andyleiserson in [#8949](https://github.com/gfx-rs/wgpu/pull/8949).
132132
- Fixed calculation of the total number of bindings in a pipeline layout when validating against device limits. By @andyleiserson in [#8997](https://github.com/gfx-rs/wgpu/pull/8997).
133+
- Reject non-constructible types (runtime- and override-sized arrays, and structs containing non-constructible types) in more places where they should not be allowed. By @andyleiserson in [#8873](https://github.com/gfx-rs/wgpu/pull/8873).
133134

134135
#### Vulkan
135136
- Fixed a variety of mesh shader SPIR-V writer issues from the original implementation. By @inner-daemons in [#8756](https://github.com/gfx-rs/wgpu/pull/8756)

‎cts_runner/test.lst‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -378,13 +378,18 @@ webgpu:shader,validation,expression,call,builtin,textureNumSamples:*
378378
fails-if(vulkan) webgpu:shader,validation,expression,call,builtin,textureSampleBaseClampToEdge:*
379379
webgpu:shader,validation,expression,call,builtin,textureStore:*
380380
webgpu:shader,validation,expression,call,builtin,trunc:*
381-
// FAIL: others in `value_constructor` due to https://github.com/gfx-rs/wgpu/issues/4720, possibly more
382381
webgpu:shader,validation,expression,call,builtin,value_constructor:array_value:*
382+
webgpu:shader,validation,expression,call,builtin,value_constructor:array_zero_value:*
383383
webgpu:shader,validation,expression,call,builtin,value_constructor:matrix_zero_value:*
384384
webgpu:shader,validation,expression,call,builtin,value_constructor:must_use:ctor="mat4x4";use=true
385+
webgpu:shader,validation,expression,call,builtin,value_constructor:partial_eval:*
385386
webgpu:shader,validation,expression,call,builtin,value_constructor:scalar_value:*
386387
webgpu:shader,validation,expression,call,builtin,value_constructor:scalar_zero_value:*
387388
webgpu:shader,validation,expression,call,builtin,value_constructor:struct_value:*
389+
webgpu:shader,validation,expression,call,builtin,value_constructor:struct_zero_value:*
390+
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_copy:*
391+
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_elementwise:*
392+
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_mixed:*
388393
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_splat:*
389394
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_zero_value:*
390395
webgpu:shader,validation,expression,call,builtin,workgroupUniformLoad:*

‎naga/src/front/wgsl/lower/construction.rs‎

Lines changed: 35 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -152,33 +152,35 @@ impl<'source> Lowerer<'source, '_> {
152152

153153
let expr;
154154
match (components, constructor) {
155-
// Empty constructor
156-
(Components::None, dst_ty) => match dst_ty {
157-
Constructor::Type((result_ty, _)) => {
158-
expr = crate::Expression::ZeroValue(result_ty);
159-
}
160-
Constructor::PartialVector { size } => {
161-
// vec2(), vec3(), vec4() return vectors of abstractInts; the same
162-
// is not true of the similar constructors for matrices or arrays.
163-
// See https://www.w3.org/TR/WGSL/#vec2-builtin et seq.
164-
let result_ty = ctx.module.types.insert(
165-
crate::Type {
166-
name: None,
167-
inner: crate::TypeInner::Vector {
168-
size,
169-
scalar: crate::Scalar::ABSTRACT_INT,
170-
},
155+
// Zero-value constructor with explicit type.
156+
(Components::None, Constructor::Type((result_ty, inner)))
157+
if inner.is_constructible(&ctx.module.types) =>
158+
{
159+
expr = crate::Expression::ZeroValue(result_ty);
160+
}
161+
// Zero-value constructor, vector with type inference
162+
(Components::None, Constructor::PartialVector { size }) => {
163+
// vec2(), vec3(), vec4() return vectors of abstractInts; the same
164+
// is not true of the similar constructors for matrices or arrays.
165+
// See https://www.w3.org/TR/WGSL/#vec2-builtin et seq.
166+
let result_ty = ctx.module.types.insert(
167+
crate::Type {
168+
name: None,
169+
inner: crate::TypeInner::Vector {
170+
size,
171+
scalar: crate::Scalar::ABSTRACT_INT,
171172
},
172-
span,
173-
);
174-
expr = crate::Expression::ZeroValue(result_ty);
175-
}
176-
Constructor::PartialMatrix { .. } | Constructor::PartialArray => {
177-
// We have no arguments from which to infer the result type, so
178-
// partial constructors aren't acceptable here.
179-
return Err(Box::new(Error::TypeNotInferable(ty_span)));
180-
}
181-
},
173+
},
174+
span,
175+
);
176+
expr = crate::Expression::ZeroValue(result_ty);
177+
}
178+
// Zero-value constructor, matrix or array with type inference
179+
(Components::None, Constructor::PartialMatrix { .. } | Constructor::PartialArray) => {
180+
// We have no arguments from which to infer the result type, so
181+
// partial constructors aren't acceptable here.
182+
return Err(Box::new(Error::TypeNotInferable(ty_span)));
183+
}
182184

183185
// Scalar constructor & conversion (scalar -> scalar)
184186
(
@@ -530,8 +532,11 @@ impl<'source> Lowerer<'source, '_> {
530532
expr = crate::Expression::Compose { ty, components };
531533
}
532534

533-
// Array constructor, explicit type
534-
(components, Constructor::Type((ty, &crate::TypeInner::Array { base, .. }))) => {
535+
// Array constructor, explicit type.
536+
(
537+
components,
538+
Constructor::Type((ty, inner @ &crate::TypeInner::Array { base, .. })),
539+
) if inner.is_constructible(&ctx.module.types) => {
535540
let mut components = components.into_components_vec();
536541
ctx.try_automatic_conversions_slice(&mut components, &Tr::Handle(base), ty_span)?;
537542
expr = crate::Expression::Compose { ty, components };
@@ -540,8 +545,8 @@ impl<'source> Lowerer<'source, '_> {
540545
// Struct constructor
541546
(
542547
components,
543-
Constructor::Type((ty, &crate::TypeInner::Struct { ref members, .. })),
544-
) => {
548+
Constructor::Type((ty, inner @ &crate::TypeInner::Struct { ref members, .. })),
549+
) if inner.is_constructible(&ctx.module.types) => {
545550
let mut components = components.into_components_vec();
546551
let struct_ty_span = ctx.module.types.get_span(ty);
547552

‎naga/src/front/wgsl/lower/mod.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1811,14 +1811,28 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
18111811

18121812
let mut ectx = ctx.as_expression(block, &mut emitter);
18131813

1814-
let (_ty, initializer) = self.type_and_init(
1814+
let (ty, initializer) = self.type_and_init(
18151815
l.name,
18161816
Some(l.init),
18171817
explicit_ty,
18181818
AbstractRule::Concretize,
18191819
&mut ectx,
18201820
)?;
18211821

1822+
// We have this special check here for `let` declarations because the
1823+
// validator doesn't check them (they are comingled with other things in
1824+
// `named_expressions`; see <https://github.com/gfx-rs/wgpu/issues/7393>).
1825+
// The check could go in `type_and_init`, but then we'd have to
1826+
// distinguish whether override-sized is allowed. The error ought to use
1827+
// the type's span, but `module.types.get_span(ty)` is `Span::UNDEFINED`
1828+
// (see <https://github.com/gfx-rs/wgpu/issues/7951>).
1829+
if ctx.module.types[ty]
1830+
.inner
1831+
.is_dynamically_sized(&ctx.module.types)
1832+
{
1833+
return Err(Box::new(Error::TypeNotConstructible(l.name.span)));
1834+
}
1835+
18221836
// We passed `Some()` to `type_and_init`, so we
18231837
// will get a lowered initializer expression back.
18241838
let initializer =

‎naga/src/proc/type_methods.rs‎

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -344,10 +344,18 @@ impl crate::TypeInner {
344344
left.as_ref().unwrap_or(self) == right.as_ref().unwrap_or(rhs)
345345
}
346346

347+
/// Returns true if `self` is runtime- or override-sized.
347348
pub fn is_dynamically_sized(&self, types: &crate::UniqueArena<crate::Type>) -> bool {
348349
use crate::TypeInner as Ti;
349350
match *self {
350-
Ti::Array { size, .. } => size == crate::ArraySize::Dynamic,
351+
Ti::Array {
352+
size: crate::ArraySize::Constant(_),
353+
..
354+
} => false,
355+
Ti::Array {
356+
size: crate::ArraySize::Pending(_) | crate::ArraySize::Dynamic,
357+
..
358+
} => true,
351359
Ti::Struct { ref members, .. } => members
352360
.last()
353361
.map(|last| types[last.ty].inner.is_dynamically_sized(types))
@@ -356,6 +364,35 @@ impl crate::TypeInner {
356364
}
357365
}
358366

367+
/// Returns true if `self` is a constructible type.
368+
pub fn is_constructible(&self, types: &crate::UniqueArena<crate::Type>) -> bool {
369+
use crate::TypeInner as Ti;
370+
match *self {
371+
Ti::Array { base, size, .. } => {
372+
let fixed_size = match size {
373+
ir::ArraySize::Constant(_) => true,
374+
ir::ArraySize::Pending(_) | ir::ArraySize::Dynamic => false,
375+
};
376+
fixed_size && types[base].inner.is_constructible(types)
377+
}
378+
Ti::Struct { ref members, .. } => members
379+
.iter()
380+
.all(|member| types[member.ty].inner.is_constructible(types)),
381+
Ti::Atomic(_)
382+
| Ti::Pointer { .. }
383+
| Ti::ValuePointer { .. }
384+
| Ti::Image { .. }
385+
| Ti::Sampler { .. }
386+
| Ti::AccelerationStructure { .. }
387+
| Ti::BindingArray { .. } => false,
388+
Ti::Scalar(_)
389+
| Ti::Vector { .. }
390+
| Ti::Matrix { .. }
391+
| Ti::RayQuery { .. }
392+
| Ti::CooperativeMatrix { .. } => true,
393+
}
394+
}
395+
359396
pub const fn components(&self) -> Option<u32> {
360397
Some(match *self {
361398
Self::Vector { size, .. } => size as u32,

‎naga/src/valid/expression.rs‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,11 @@
11
use super::{compose::validate_compose, FunctionInfo, ModuleInfo, ShaderStages, TypeFlags};
22
use crate::arena::UniqueArena;
3-
use crate::valid::expression::builtin::validate_zero_value;
43
use crate::{
54
arena::Handle,
65
proc::OverloadSet as _,
76
proc::{IndexableLengthError, ResolveError},
87
};
98

10-
pub mod builtin;
11-
129
#[derive(Clone, Debug, thiserror::Error)]
1310
#[cfg_attr(test, derive(PartialEq))]
1411
pub enum ExpressionError {
@@ -38,8 +35,8 @@ pub enum ExpressionError {
3835
InvalidSwizzleComponent(crate::SwizzleComponent, crate::VectorSize),
3936
#[error(transparent)]
4037
Compose(#[from] super::ComposeError),
41-
#[error(transparent)]
42-
ZeroValue(#[from] super::ZeroValueError),
38+
#[error("Cannot construct zero value of {0:?} because it is not a constructible type")]
39+
InvalidZeroValue(Handle<crate::Type>),
4340
#[error(transparent)]
4441
IndexableLength(#[from] IndexableLengthError),
4542
#[error("Operation {0:?} can't work with {1:?}")]
@@ -460,7 +457,9 @@ impl super::Validator {
460457
}
461458
E::Constant(_) | E::Override(_) => ShaderStages::all(),
462459
E::ZeroValue(ty) => {
463-
validate_zero_value(ty, module.to_ctx())?;
460+
if !mod_info[ty].contains(TypeFlags::CONSTRUCTIBLE) {
461+
return Err(ExpressionError::InvalidZeroValue(ty));
462+
}
464463
ShaderStages::all()
465464
}
466465
E::Compose { ref components, ty } => {

‎naga/src/valid/expression/builtin.rs‎

Lines changed: 0 additions & 26 deletions
This file was deleted.

‎naga/src/valid/mod.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ use crate::{
2727
use crate::span::{AddSpan as _, WithSpan};
2828
pub use analyzer::{ExpressionInfo, FunctionInfo, GlobalUse, Uniformity, UniformityRequirements};
2929
pub use compose::ComposeError;
30-
pub use expression::builtin::ZeroValueError;
3130
pub use expression::{check_literal_value, LiteralError};
3231
pub use expression::{ConstExpressionError, ExpressionError};
3332
pub use function::{CallError, FunctionError, LocalVariableError, SubgroupError};
@@ -771,6 +770,10 @@ impl Validator {
771770
}
772771
.with_span_handle(handle, &module.types)
773772
})?;
773+
debug_assert!(
774+
ty_info.flags.contains(TypeFlags::CONSTRUCTIBLE)
775+
== module.types[handle].inner.is_constructible(&module.types)
776+
);
774777
mod_info.type_flags.push(ty_info.flags);
775778
self.types[handle.index()] = ty_info;
776779
}

0 commit comments

Comments
 (0)