Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report overflow when shl overflows into sign bit #6175

Closed
sagudev opened this issue Aug 28, 2024 · 1 comment · Fixed by #6186
Closed

Report overflow when shl overflows into sign bit #6175

sagudev opened this issue Aug 28, 2024 · 1 comment · Fixed by #6186

Comments

@sagudev
Copy link
Contributor

sagudev commented Aug 28, 2024

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 #6156 (comment)

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
Copy link
Member

teoxoy commented Aug 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants
@sagudev @teoxoy and others