From ab02ec79048bd902ccbfa304a5284cc07bc08898 Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Thu, 13 Jan 2022 16:27:28 -0500 Subject: [PATCH] layouter: rich and careful errors --- src/back/hlsl/storage.rs | 4 +-- src/back/msl/writer.rs | 17 ++++++------ src/front/glsl/functions.rs | 5 +++- src/front/glsl/types.rs | 10 +++---- src/front/spv/mod.rs | 4 +-- src/proc/layouter.rs | 54 +++++++++++++++++++++++++++++-------- src/proc/mod.rs | 24 +++++++++-------- src/valid/function.rs | 7 ++--- src/valid/mod.rs | 6 ++--- src/valid/type.rs | 7 +++-- 10 files changed, 90 insertions(+), 48 deletions(-) diff --git a/src/back/hlsl/storage.rs b/src/back/hlsl/storage.rs index b235a061fb..08909802b1 100644 --- a/src/back/hlsl/storage.rs +++ b/src/back/hlsl/storage.rs @@ -148,7 +148,7 @@ impl super::Writer<'_, W> { } => { write!(self.out, "{{")?; let count = module.constants[const_handle].to_array_length().unwrap(); - let stride = module.types[base].inner.span(&module.constants); + let stride = module.types[base].inner.size(&module.constants).unwrap(); let iter = (0..count).map(|i| (TypeResolution::Handle(base), stride * i)); self.write_storage_load_sequence(module, var_handle, iter, func_ctx)?; write!(self.out, "}}")?; @@ -311,7 +311,7 @@ impl super::Writer<'_, W> { writeln!(self.out, ";")?; // then iterate the stores let count = module.constants[const_handle].to_array_length().unwrap(); - let stride = module.types[base].inner.span(&module.constants); + let stride = module.types[base].inner.size(&module.constants).unwrap(); for i in 0..count { self.temp_access_chain.push(SubAccess::Offset(i * stride)); let sv = StoreValue::TempIndex { diff --git a/src/back/msl/writer.rs b/src/back/msl/writer.rs index dd4e93b222..9533e5076f 100644 --- a/src/back/msl/writer.rs +++ b/src/back/msl/writer.rs @@ -349,7 +349,7 @@ fn should_pack_struct_member( } let ty_inner = &module.types[member.ty].inner; - let last_offset = member.offset + ty_inner.span(&module.constants); + let last_offset = member.offset + ty_inner.size(&module.constants).unwrap(); let next_offset = match members.get(index + 1) { Some(next) => next.offset, None => span, @@ -750,22 +750,23 @@ impl Writer { None => return Err(Error::Validation), }; - let (span, stride) = match context.module.types[array_ty].inner { + let (size, stride) = match context.module.types[array_ty].inner { crate::TypeInner::Array { base, stride, .. } => ( context.module.types[base] .inner - .span(&context.module.constants), + .size(&context.module.constants) + .unwrap(), stride, ), _ => return Err(Error::Validation), }; - // When the stride length is larger than the span, the final element's stride of + // When the stride length is larger than the size, the final element's stride of // bytes would have padding following the value. But the buffer size in // `buffer_sizes.sizeN` may not include this padding - it only needs to be large // enough to hold the actual values' bytes. // - // So subtract off the span to get a byte size that falls at the start or within + // So subtract off the size to get a byte size that falls at the start or within // the final element. Then divide by the stride size, to get one less than the // length, and then add one. This works even if the buffer size does include the // stride padding, since division rounds towards zero (MSL 2.4 ยง6.1). It will fail @@ -774,10 +775,10 @@ impl Writer { // prevent that. write!( self.out, - "(_buffer_sizes.size{idx} - {offset} - {span}) / {stride}", + "(_buffer_sizes.size{idx} - {offset} - {size}) / {stride}", idx = handle.index(), offset = offset, - span = span, + size = size, stride = stride, )?; Ok(()) @@ -2379,7 +2380,7 @@ impl Writer { writeln!(self.out, "{}char _pad{}[{}];", back::INDENT, index, pad)?; } let ty_inner = &module.types[member.ty].inner; - last_offset = member.offset + ty_inner.span(&module.constants); + last_offset = member.offset + ty_inner.size(&module.constants).unwrap(); let member_name = &self.names[&NameKey::StructMember(handle, index as u32)]; diff --git a/src/front/glsl/functions.rs b/src/front/glsl/functions.rs index 14f30bdfb7..d565b00222 100644 --- a/src/front/glsl/functions.rs +++ b/src/front/glsl/functions.rs @@ -1293,7 +1293,10 @@ impl Parser { offset: span, }); - span += self.module.types[ty].inner.span(&self.module.constants); + span += self.module.types[ty] + .inner + .size(&self.module.constants) + .unwrap(); let len = expressions.len(); let load = expressions.append(Expression::Load { pointer }, Default::default()); diff --git a/src/front/glsl/types.rs b/src/front/glsl/types.rs index e008ec3822..106104378e 100644 --- a/src/front/glsl/types.rs +++ b/src/front/glsl/types.rs @@ -262,14 +262,14 @@ impl Parser { array_specifier .map(|(size, size_meta)| { meta.subsume(size_meta); + let stride = self.module.types[base] + .inner + .size(&self.module.constants) + .unwrap(); self.module.types.insert( Type { name: None, - inner: TypeInner::Array { - base, - size, - stride: self.module.types[base].inner.span(&self.module.constants), - }, + inner: TypeInner::Array { base, size, stride }, }, meta, ) diff --git a/src/front/spv/mod.rs b/src/front/spv/mod.rs index e1de8bdaf6..f6f7db14dd 100644 --- a/src/front/spv/mod.rs +++ b/src/front/spv/mod.rs @@ -4078,7 +4078,7 @@ impl> Parser { size: crate::ArraySize::Constant(length_const.handle), stride: match decor.array_stride { Some(stride) => stride.get(), - None => module.types[base].inner.span(&module.constants), + None => module.types[base].inner.size(&module.constants).unwrap(), }, }; self.lookup_type.insert( @@ -4115,7 +4115,7 @@ impl> Parser { size: crate::ArraySize::Dynamic, stride: match decor.array_stride { Some(stride) => stride.get(), - None => module.types[base].inner.span(&module.constants), + None => module.types[base].inner.size(&module.constants).unwrap(), }, }; self.lookup_type.insert( diff --git a/src/proc/layouter.rs b/src/proc/layouter.rs index d9b1f2f159..637257c17e 100644 --- a/src/proc/layouter.rs +++ b/src/proc/layouter.rs @@ -30,8 +30,29 @@ impl ops::Index> for Layouter { } #[derive(Clone, Copy, Debug, PartialEq, thiserror::Error)] -#[error("Base type {0:?} is out of bounds")] -pub struct InvalidBaseType(pub Handle); +pub enum TypeLayoutError { + #[error("Array element type {0:?} doesn't exist")] + InvalidArrayElementType(Handle), + #[error("Struct member[{0}] type {1:?} doesn't exist")] + InvalidStructMemberType(u32, Handle), + #[error("Zero width is not supported")] + ZeroWidth, + #[error(transparent)] + Size(#[from] super::ProcError), +} + +#[derive(Clone, Copy, Debug, PartialEq, thiserror::Error)] +#[error("Error laying out type {ty:?}: {inner}")] +pub struct LayoutError { + pub ty: Handle, + pub inner: TypeLayoutError, +} + +impl TypeLayoutError { + fn with(self, ty: Handle) -> LayoutError { + LayoutError { ty, inner: self } + } +} impl Layouter { pub fn clear(&mut self) { @@ -62,19 +83,24 @@ impl Layouter { (start..start + span, alignment) } + #[allow(clippy::or_fun_call)] pub fn update( &mut self, types: &UniqueArena, constants: &Arena, - ) -> Result<(), InvalidBaseType> { + ) -> Result<(), LayoutError> { use crate::TypeInner as Ti; for (ty_handle, ty) in types.iter().skip(self.layouts.len()) { - let size = ty.inner.span(constants); + let size = ty + .inner + .size(constants) + .map_err(|error| TypeLayoutError::Size(error).with(ty_handle))?; let layout = match ty.inner { Ti::Scalar { width, .. } | Ti::Atomic { width, .. } => TypeLayout { size, - alignment: Alignment::new(width as u32).ok_or(InvalidBaseType(ty_handle))?, + alignment: Alignment::new(width as u32) + .ok_or(TypeLayoutError::ZeroWidth.with(ty_handle))?, }, Ti::Vector { size: vec_size, @@ -88,7 +114,8 @@ impl Layouter { } else { 2 }; - Alignment::new(count * width as u32).ok_or(InvalidBaseType(ty_handle))? + Alignment::new(count * width as u32) + .ok_or(TypeLayoutError::ZeroWidth.with(ty_handle))? }, }, Ti::Matrix { @@ -99,7 +126,8 @@ impl Layouter { size, alignment: { let count = if rows >= crate::VectorSize::Tri { 4 } else { 2 }; - Alignment::new(count * width as u32).ok_or(InvalidBaseType(ty_handle))? + Alignment::new(count * width as u32) + .ok_or(TypeLayoutError::ZeroWidth.with(ty_handle))? }, }, Ti::Pointer { .. } | Ti::ValuePointer { .. } => TypeLayout { @@ -115,16 +143,20 @@ impl Layouter { alignment: if base < ty_handle { self[base].alignment } else { - return Err(InvalidBaseType(base)); + return Err(TypeLayoutError::InvalidArrayElementType(base).with(ty_handle)); }, }, Ti::Struct { span, ref members } => { let mut alignment = Alignment::new(1).unwrap(); - for member in members { + for (index, member) in members.iter().enumerate() { alignment = if member.ty < ty_handle { alignment.max(self[member.ty].alignment) } else { - return Err(InvalidBaseType(member.ty)); + return Err(TypeLayoutError::InvalidStructMemberType( + index as u32, + member.ty, + ) + .with(ty_handle)); }; } TypeLayout { @@ -137,7 +169,7 @@ impl Layouter { alignment: Alignment::new(1).unwrap(), }, }; - debug_assert!(ty.inner.span(constants) <= layout.size); + debug_assert!(size <= layout.size); self.layouts.push(layout); } diff --git a/src/proc/mod.rs b/src/proc/mod.rs index 00ec69c605..79fd078ecd 100644 --- a/src/proc/mod.rs +++ b/src/proc/mod.rs @@ -9,16 +9,16 @@ mod typifier; use std::cmp::PartialEq; pub use index::{BoundsCheckPolicies, BoundsCheckPolicy, IndexableLength}; -pub use layouter::{Alignment, InvalidBaseType, Layouter, TypeLayout}; +pub use layouter::{Alignment, LayoutError, Layouter, TypeLayout, TypeLayoutError}; pub use namer::{EntryPointIndex, NameKey, Namer}; pub use terminator::ensure_block_returns; pub use typifier::{ResolveContext, ResolveError, TypeResolution}; -#[derive(Clone, Debug, thiserror::Error, PartialEq)] +#[derive(Clone, Copy, Debug, thiserror::Error, PartialEq)] pub enum ProcError { - #[error("type is not indexable, and has no length (validation error)")] + #[error("Type is not indexable, and has no length (validation error)")] TypeNotIndexable, - #[error("array length is wrong kind of constant (validation error)")] + #[error("Array length {0:?} is wrong kind of constant (validation error)")] InvalidArraySizeConstant(crate::Handle), } @@ -94,8 +94,8 @@ impl super::TypeInner { } } - pub fn span(&self, constants: &super::Arena) -> u32 { - match *self { + pub fn size(&self, constants: &super::Arena) -> Result { + Ok(match *self { Self::Scalar { kind: _, width } | Self::Atomic { kind: _, width } => width as u32, Self::Vector { size, @@ -119,8 +119,10 @@ impl super::TypeInner { } => { let count = match size { super::ArraySize::Constant(handle) => { - // Bad array lengths will be caught during validation. - constants[handle].to_array_length().unwrap_or(1) + let constant = constants + .try_get(handle) + .ok_or(ProcError::InvalidArraySizeConstant(handle))?; + constant.to_array_length().unwrap_or(1) } // A dynamically-sized array has to have at least one element super::ArraySize::Dynamic => 1, @@ -129,7 +131,7 @@ impl super::TypeInner { } Self::Struct { span, .. } => span, Self::Image { .. } | Self::Sampler { .. } => 0, - } + }) } /// Return the canoncal form of `self`, or `None` if it's already in @@ -447,7 +449,7 @@ fn test_matrix_size() { rows: crate::VectorSize::Tri, width: 4 } - .span(&constants), - 48 + .size(&constants), + Ok(48), ); } diff --git a/src/valid/function.rs b/src/valid/function.rs index d0eeab1b5c..491cee54bb 100644 --- a/src/valid/function.rs +++ b/src/valid/function.rs @@ -824,6 +824,7 @@ impl super::Validator { module: &crate::Module, mod_info: &ModuleInfo, ) -> Result> { + #[cfg_attr(not(feature = "validate"), allow(unused_mut))] let mut info = mod_info.process_function(fun, module, self.flags)?; #[cfg(feature = "validate")] @@ -842,13 +843,13 @@ impl super::Validator { #[cfg(feature = "validate")] for (index, argument) in fun.arguments.iter().enumerate() { - let ty = module.types.get_handle(argument.ty).ok_or( + let ty = module.types.get_handle(argument.ty).ok_or_else(|| { FunctionError::InvalidArgumentType { index, name: argument.name.clone().unwrap_or_default(), } - .with_span_handle(argument.ty, &module.types), - )?; + .with_span_handle(argument.ty, &module.types) + })?; match ty.inner.pointer_class() { Some(crate::StorageClass::Private) | Some(crate::StorageClass::Function) diff --git a/src/valid/mod.rs b/src/valid/mod.rs index a7e57a8667..91807e002e 100644 --- a/src/valid/mod.rs +++ b/src/valid/mod.rs @@ -10,7 +10,7 @@ use crate::arena::{Arena, UniqueArena}; use crate::{ arena::Handle, - proc::{InvalidBaseType, Layouter}, + proc::{LayoutError, Layouter}, FastHashSet, }; use bit_set::BitSet; @@ -127,7 +127,7 @@ pub enum ConstantError { #[derive(Clone, Debug, thiserror::Error)] pub enum ValidationError { #[error(transparent)] - Layouter(#[from] InvalidBaseType), + Layouter(#[from] LayoutError), #[error("Type {handle:?} '{name}' is invalid")] Type { handle: Handle, @@ -278,7 +278,7 @@ impl Validator { self.layouter .update(&module.types, &module.constants) .map_err(|e| { - let InvalidBaseType(handle) = e; + let handle = e.ty; ValidationError::from(e).with_span_handle(handle, &module.types) })?; diff --git a/src/valid/type.rs b/src/valid/type.rs index cda1fa64a0..570e8efcc8 100644 --- a/src/valid/type.rs +++ b/src/valid/type.rs @@ -326,7 +326,8 @@ impl super::Validator { return Err(TypeError::InvalidArrayBaseType(base)); } - let base_size = types[base].inner.span(constants); + //Note: `unwrap()` is fine, since `Layouter` goes first and calls it + let base_size = types[base].inner.size(constants).unwrap(); if stride < base_size { return Err(TypeError::InsufficientArrayStride { stride, base_size }); } @@ -478,7 +479,9 @@ impl super::Validator { }); } } - let base_size = types[member.ty].inner.span(constants); + + //Note: `unwrap()` is fine because `Layouter` goes first and checks this + let base_size = types[member.ty].inner.size(constants).unwrap(); min_offset = member.offset + base_size; if min_offset > span { return Err(TypeError::MemberOutOfBounds {