layouter: rich and careful errors

This commit is contained in:
Dzmitry Malyshau
2022-01-13 16:27:28 -05:00
parent a069361bf4
commit ab02ec7904
10 changed files with 90 additions and 48 deletions

View File

@@ -148,7 +148,7 @@ impl<W: fmt::Write> 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<W: fmt::Write> 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 {

View File

@@ -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<W: Write> Writer<W> {
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<W: Write> Writer<W> {
// 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<W: Write> Writer<W> {
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)];

View File

@@ -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());

View File

@@ -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,
)

View File

@@ -4078,7 +4078,7 @@ impl<I: Iterator<Item = u32>> Parser<I> {
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<I: Iterator<Item = u32>> Parser<I> {
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(

View File

@@ -30,8 +30,29 @@ impl ops::Index<Handle<crate::Type>> for Layouter {
}
#[derive(Clone, Copy, Debug, PartialEq, thiserror::Error)]
#[error("Base type {0:?} is out of bounds")]
pub struct InvalidBaseType(pub Handle<crate::Type>);
pub enum TypeLayoutError {
#[error("Array element type {0:?} doesn't exist")]
InvalidArrayElementType(Handle<crate::Type>),
#[error("Struct member[{0}] type {1:?} doesn't exist")]
InvalidStructMemberType(u32, Handle<crate::Type>),
#[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<crate::Type>,
pub inner: TypeLayoutError,
}
impl TypeLayoutError {
fn with(self, ty: Handle<crate::Type>) -> 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<crate::Type>,
constants: &Arena<crate::Constant>,
) -> 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);
}

View File

@@ -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<crate::Constant>),
}
@@ -94,8 +94,8 @@ impl super::TypeInner {
}
}
pub fn span(&self, constants: &super::Arena<super::Constant>) -> u32 {
match *self {
pub fn size(&self, constants: &super::Arena<super::Constant>) -> Result<u32, ProcError> {
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),
);
}

View File

@@ -824,6 +824,7 @@ impl super::Validator {
module: &crate::Module,
mod_info: &ModuleInfo,
) -> Result<FunctionInfo, WithSpan<FunctionError>> {
#[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)

View File

@@ -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<crate::Type>,
@@ -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)
})?;

View File

@@ -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 {