Skip to content

Commit d199493

Browse files
authored
fix(naga): Raise an error on division by zero (gfx-rs#9276)
Add validation about division by zero in valiator. Plus, fix div/rem on i32 in const-eval. Relevant spec <https://gpuweb.github.io/gpuweb/wgsl/#arithmetic-expr> says "If e1 is most negative value in T, and e2 is -1: ..." which means -MIN / -1 is an error. This is exercised by CTS.
1 parent 97e235f commit d199493

2 files changed

Lines changed: 68 additions & 8 deletions

File tree

naga/src/proc/constant_evaluator.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2722,20 +2722,20 @@ impl<'a> ConstantEvaluator<'a> {
27222722
BinaryOperator::Add => a.wrapping_add(b),
27232723
BinaryOperator::Subtract => a.wrapping_sub(b),
27242724
BinaryOperator::Multiply => a.wrapping_mul(b),
2725-
BinaryOperator::Divide => {
2725+
BinaryOperator::Divide => a.checked_div(b).ok_or_else(|| {
27262726
if b == 0 {
2727-
return Err(ConstantEvaluatorError::DivisionByZero);
2727+
ConstantEvaluatorError::DivisionByZero
27282728
} else {
2729-
a.wrapping_div(b)
2729+
ConstantEvaluatorError::Overflow("division".into())
27302730
}
2731-
}
2732-
BinaryOperator::Modulo => {
2731+
})?,
2732+
BinaryOperator::Modulo => a.checked_rem(b).ok_or_else(|| {
27332733
if b == 0 {
2734-
return Err(ConstantEvaluatorError::RemainderByZero);
2734+
ConstantEvaluatorError::RemainderByZero
27352735
} else {
2736-
a.wrapping_rem(b)
2736+
ConstantEvaluatorError::Overflow("remainder".into())
27372737
}
2738-
}
2738+
})?,
27392739
BinaryOperator::And => a & b,
27402740
BinaryOperator::ExclusiveOr => a ^ b,
27412741
BinaryOperator::InclusiveOr => a | b,

naga/src/valid/expression.rs

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,8 @@ pub enum ExpressionError {
156156
lhs_type: crate::TypeInner,
157157
rhs_expr: Handle<crate::Expression>,
158158
},
159+
#[error("Division by zero")]
160+
DivideByZero,
159161
}
160162

161163
#[derive(Clone, Debug, thiserror::Error)]
@@ -319,6 +321,60 @@ impl super::Validator {
319321
}
320322
}
321323

324+
/// Return an error if a constant divisor in `right` evaluates to zero for
325+
/// an integer division or remainder operation.
326+
///
327+
/// This function promises to return an error in cases where (1) the
328+
/// expression is well-typed, (2) `left_ty` is a concrete integer or a
329+
/// vector, and (3) `right` is a const-expression that evaluates to zero.
330+
/// It does not return an error in cases where the expression is not
331+
/// well-typed (e.g. vector dimension mismatch), because those will be
332+
/// rejected elsewhere.
333+
fn validate_constant_divisor(
334+
left_ty: &crate::TypeInner,
335+
right: Handle<crate::Expression>,
336+
module: &crate::Module,
337+
function: &crate::Function,
338+
) -> Result<(), ExpressionError> {
339+
fn contains_zero(
340+
handle: Handle<crate::Expression>,
341+
expressions: &crate::Arena<crate::Expression>,
342+
module: &crate::Module,
343+
) -> bool {
344+
match expressions[handle] {
345+
crate::Expression::Literal(_) | crate::Expression::ZeroValue(_) => module
346+
.to_ctx()
347+
.get_const_val_from::<u32, _>(handle, expressions)
348+
.ok()
349+
.is_some_and(|v| v == 0),
350+
crate::Expression::Splat { value, .. } => contains_zero(value, expressions, module),
351+
crate::Expression::Compose { ref components, .. } => components
352+
.iter()
353+
.any(|&comp| contains_zero(comp, expressions, module)),
354+
crate::Expression::Constant(c) => {
355+
contains_zero(module.constants[c].init, &module.global_expressions, module)
356+
}
357+
_ => false,
358+
}
359+
}
360+
361+
let Some((_, scalar)) = left_ty.vector_size_and_scalar() else {
362+
return Ok(());
363+
};
364+
if !matches!(
365+
scalar.kind,
366+
crate::ScalarKind::Sint | crate::ScalarKind::Uint
367+
) {
368+
return Ok(());
369+
}
370+
371+
if contains_zero(right, &function.expressions, module) {
372+
Err(ExpressionError::DivideByZero)
373+
} else {
374+
Ok(())
375+
}
376+
}
377+
322378
#[allow(clippy::too_many_arguments)]
323379
pub(super) fn validate_expression(
324380
&self,
@@ -1071,6 +1127,10 @@ impl super::Validator {
10711127
if matches!(op, Bo::ShiftLeft | Bo::ShiftRight) {
10721128
Self::validate_constant_shift_amounts(left_inner, right, module, function)?;
10731129
}
1130+
// For integer division or remainder, check if the constant divisor is zero
1131+
if matches!(op, Bo::Divide | Bo::Modulo) {
1132+
Self::validate_constant_divisor(left_inner, right, module, function)?;
1133+
}
10741134
ShaderStages::all()
10751135
}
10761136
E::Select {

0 commit comments

Comments
 (0)