gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.33k stars 904 forks source link

Report overflow when shl overflows into sign bit #6175

Closed sagudev closed 1 month ago

sagudev commented 1 month ago

Per https://gpuweb.github.io/gpuweb/wgsl/#bit-expr we need to report overflow when value goes into sign bits, while rust's checked_shl only reports overflow when its out of bits.

Originally posted by @sagudev in https://github.com/gfx-rs/wgpu/issues/6156#issuecomment-2314357043

Actually the issue is much worse, checked_shl only performs checks on rhs not for actual overflow. Relevant tint code: https://github.com/google/dawn/blob/03b32d20940d1b0e6d979c30ee327c309de6ed01/src/tint/lang/core/constant/eval.cc#L1977

But I think this should make it work:

diff --git a/naga/src/proc/constant_evaluator.rs b/naga/src/proc/constant_evaluator.rs
index a79944a3f..c4ef04e25 100644
--- a/naga/src/proc/constant_evaluator.rs
+++ b/naga/src/proc/constant_evaluator.rs
@@ -1832,8 +1832,11 @@ impl<'a> ConstantEvaluator<'a> {
                         }),
                         (Literal::I32(a), Literal::U32(b)) => Literal::I32(match op {
                             BinaryOperator::ShiftLeft => a
-                                .checked_shl(b)
-                                .ok_or(ConstantEvaluatorError::ShiftedMoreThan32Bits)?,
+                                .checked_mul(
+                                    1i32.checked_shl(b)
+                                        .ok_or(ConstantEvaluatorError::ShiftedMoreThan32Bits)?,
+                                )
+                                .ok_or(ConstantEvaluatorError::Overflow("<<".to_string()))?,
                             BinaryOperator::ShiftRight => a
                                 .checked_shr(b)
                                 .ok_or(ConstantEvaluatorError::ShiftedMoreThan32Bits)?,
@@ -1859,8 +1862,11 @@ impl<'a> ConstantEvaluator<'a> {
                             BinaryOperator::ExclusiveOr => a ^ b,
                             BinaryOperator::InclusiveOr => a | b,
                             BinaryOperator::ShiftLeft => a
-                                .checked_shl(b)
-                                .ok_or(ConstantEvaluatorError::ShiftedMoreThan32Bits)?,
+                                .checked_mul(
+                                    1u32.checked_shl(b)
+                                        .ok_or(ConstantEvaluatorError::ShiftedMoreThan32Bits)?,
+                                )
+                                .ok_or(ConstantEvaluatorError::Overflow("<<".to_string()))?,
                             BinaryOperator::ShiftRight => a
                                 .checked_shr(b)
                                 .ok_or(ConstantEvaluatorError::ShiftedMoreThan32Bits)?,
teoxoy commented 1 month ago

I deleted the comment as it contained the link as well. The account and comments of the account that initially posted the link were removed by github.