Skip to content

Commit fa5fb3c

Browse files
andyleisersonandyleiserson
committed
fix(naga): Improve validation of non-constructible types
Fixes #4720 Fixes the test cases in #7393, but not the issue itself
1 parent 8355603 commit fa5fb3c

10 files changed

Lines changed: 583 additions & 93 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ Bottom level categories:
107107
- Check that depth bias is not used with non-triangle topologies. By @andyleiserson in [#8856](https://github.com/gfx-rs/wgpu/pull/8856).
108108
- 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).
109109
- 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).
110+
- 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).
110111

111112
#### GLES
112113

‎cts_runner/test.lst‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,13 +368,18 @@ webgpu:shader,validation,expression,call,builtin,textureNumSamples:*
368368
fails-if(vulkan) webgpu:shader,validation,expression,call,builtin,textureSampleBaseClampToEdge:*
369369
webgpu:shader,validation,expression,call,builtin,textureStore:*
370370
webgpu:shader,validation,expression,call,builtin,trunc:*
371-
// FAIL: others in `value_constructor` due to https://github.com/gfx-rs/wgpu/issues/4720, possibly more
372371
webgpu:shader,validation,expression,call,builtin,value_constructor:array_value:*
372+
webgpu:shader,validation,expression,call,builtin,value_constructor:array_zero_value:*
373373
webgpu:shader,validation,expression,call,builtin,value_constructor:matrix_zero_value:*
374374
webgpu:shader,validation,expression,call,builtin,value_constructor:must_use:ctor="mat4x4";use=true
375+
webgpu:shader,validation,expression,call,builtin,value_constructor:partial_eval:*
375376
webgpu:shader,validation,expression,call,builtin,value_constructor:scalar_value:*
376377
webgpu:shader,validation,expression,call,builtin,value_constructor:scalar_zero_value:*
377378
webgpu:shader,validation,expression,call,builtin,value_constructor:struct_value:*
379+
webgpu:shader,validation,expression,call,builtin,value_constructor:struct_zero_value:*
380+
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_copy:*
381+
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_elementwise:*
382+
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_mixed:*
378383
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_splat:*
379384
webgpu:shader,validation,expression,call,builtin,value_constructor:vector_zero_value:*
380385
webgpu:shader,validation,expression,call,builtin,workgroupUniformLoad:*

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

Lines changed: 38 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -152,33 +152,38 @@ 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. Not everything this
156+
// matches is constructible, but the non-constructible types not
157+
// excluded by `!is_dynamically_sized` are rejected by the
158+
// parser.
159+
(Components::None, Constructor::Type((result_ty, inner)))
160+
if !inner.is_dynamically_sized(&ctx.module.types) =>
161+
{
162+
expr = crate::Expression::ZeroValue(result_ty);
163+
}
164+
// Zero-value constructor, vector with type inference
165+
(Components::None, Constructor::PartialVector { size }) => {
166+
// vec2(), vec3(), vec4() return vectors of abstractInts; the same
167+
// is not true of the similar constructors for matrices or arrays.
168+
// See https://www.w3.org/TR/WGSL/#vec2-builtin et seq.
169+
let result_ty = ctx.module.types.insert(
170+
crate::Type {
171+
name: None,
172+
inner: crate::TypeInner::Vector {
173+
size,
174+
scalar: crate::Scalar::ABSTRACT_INT,
171175
},
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-
},
176+
},
177+
span,
178+
);
179+
expr = crate::Expression::ZeroValue(result_ty);
180+
}
181+
// Zero-value constructor, matrix or array with type inference
182+
(Components::None, Constructor::PartialMatrix { .. } | Constructor::PartialArray) => {
183+
// We have no arguments from which to infer the result type, so
184+
// partial constructors aren't acceptable here.
185+
return Err(Box::new(Error::TypeNotInferable(ty_span)));
186+
}
182187

183188
// Scalar constructor & conversion (scalar -> scalar)
184189
(
@@ -530,8 +535,11 @@ impl<'source> Lowerer<'source, '_> {
530535
expr = crate::Expression::Compose { ty, components };
531536
}
532537

533-
// Array constructor, explicit type
534-
(components, Constructor::Type((ty, &crate::TypeInner::Array { base, .. }))) => {
538+
// Array constructor, explicit type. Override- and runtime-sized arrays are not constructible.
539+
(
540+
components,
541+
Constructor::Type((ty, inner @ &crate::TypeInner::Array { base, .. })),
542+
) if !inner.is_dynamically_sized(&ctx.module.types) => {
535543
let mut components = components.into_components_vec();
536544
ctx.try_automatic_conversions_slice(&mut components, &Tr::Handle(base), ty_span)?;
537545
expr = crate::Expression::Compose { ty, components };
@@ -540,8 +548,8 @@ impl<'source> Lowerer<'source, '_> {
540548
// Struct constructor
541549
(
542550
components,
543-
Constructor::Type((ty, &crate::TypeInner::Struct { ref members, .. })),
544-
) => {
551+
Constructor::Type((ty, inner @ &crate::TypeInner::Struct { ref members, .. })),
552+
) if !inner.is_dynamically_sized(&ctx.module.types) => {
545553
let mut components = components.into_components_vec();
546554
let struct_ty_span = ctx.module.types.get_span(ty);
547555

‎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: 9 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))

‎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: 0 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};

0 commit comments

Comments
 (0)