Handle more cases of undefined behaviour due to signed integer overflow or division by zero #7012
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jimblandy
reviewed
Feb 7, 2025
| let level = crate::back::Level(1); | ||
| writeln!( | ||
| self.out, | ||
| "{level}return val >= 0 ? val : asint(~asuint(val) + 1u);" |
There was a problem hiding this comment.
Doesn't HLSL support unary - on uint? C++ does. So this could just be
Suggested change
| "{level}return val >= 0 ? val : asint(~asuint(val) + 1u);" | |
| "{level}return val >= 0 ? val : asint(-asuint(val));" |
| writeln!(self.out, " val) {{")?; | ||
|
|
||
| let level = crate::back::Level(1); | ||
| writeln!(self.out, "{level}return asint(~asuint(val) + 1u);",)?; |
There was a problem hiding this comment.
Similarly:
Suggested change
| writeln!(self.out, "{level}return asint(~asuint(val) + 1u);",)?; | |
| writeln!(self.out, "{level}return asint(-asuint(val));",)?; |
Comment on lines
+1160
to
+1162
| func_ctx | ||
| .resolve_type(expr_handle, &module.types) | ||
| .scalar_kind(), |
Comment on lines
+1168
to
+1177
| let left_wrapped_ty = match *func_ctx.resolve_type(left, &module.types) { | ||
| crate::TypeInner::Scalar(scalar) => (scalar, None), | ||
| crate::TypeInner::Vector { size, scalar } => (scalar, Some(size)), | ||
| _ => unreachable!(), | ||
| }; | ||
| let right_wrapped_ty = match *func_ctx.resolve_type(right, &module.types) { | ||
| crate::TypeInner::Scalar(scalar) => (scalar, None), | ||
| crate::TypeInner::Vector { size, scalar } => (scalar, Some(size)), | ||
| _ => unreachable!(), | ||
| }; |
There was a problem hiding this comment.
Aren't these resolve_type calls exactly left_ty and right_ty, already computed above?
Comment on lines
+1106
to
+1110
| let (scalar, vector_size) = match *expr_ty { | ||
| crate::TypeInner::Scalar(scalar) => (scalar, None), | ||
| crate::TypeInner::Vector { size, scalar } => (scalar, Some(size)), | ||
| _ => continue, | ||
| }; |
There was a problem hiding this comment.
You repeat this match a lot. Pity the poor reader who has to check that these are all the same, and wonder why if they're not. How about turning it into an impl TypeInner utility function (vector_size_and_scalar, say) in naga/src/proc/mod.rs?
| let level = crate::back::Level(1); | ||
| match kind { | ||
| crate::ScalarKind::Sint => { | ||
| let min = 2u64.pow(expr_ty.scalar_width().unwrap() as u32 * 8 - 1); |
There was a problem hiding this comment.
The unwrap is kind of a code smell: if you know it's not going to panic, then you must have done some prior match that you ought to have been able to get the data out of.
This is actually the negative of the minimum, so the name is a little confusing.
Suggestion: drop the explicit - in the format string and then use:
Suggested change
| let min = 2u64.pow(expr_ty.scalar_width().unwrap() as u32 * 8 - 1); | |
| let min = -1_i64 << (expr_ty.scalar_width().unwrap() as u32 * 8 - 1); |
That should get you the right minimum even for 64-bit integer types.
| @@ -2579,7 +2583,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> { | |||
| crate::Literal::F64(value) => write!(self.out, "{value:?}L")?, | |||
| crate::Literal::F32(value) => write!(self.out, "{value:?}")?, | |||
| crate::Literal::U32(value) => write!(self.out, "{value}u")?, | |||
| crate::Literal::I32(value) => write!(self.out, "{value}")?, | |||
| crate::Literal::I32(value) => write!(self.out, "int({value})")?, | |||
There was a problem hiding this comment.
Why is this necessary? This should have a comment, at least.
| self.write_expr(module, right, func_ctx)?; | ||
| write!(self.out, ")")?; | ||
| } | ||
|
|
||
| // TODO: handle undefined behavior of BinaryOperator::Modulo |
There was a problem hiding this comment.
Can we delete this comment too?
|
This also fixes #4385, I think. |
…, negation, and abs Adds infrastructure to the MSL backend for emitting helper functions, based upon the existing HLSL backend equivalent. Emit helper functions for MathFunction::Abs and UnaryOperator::Negate with a signed integer scalar or vector operand. And for BinaryOperator::Divide and BinaryOperator::Modulo with signed or unsigned integer scalar or vector operands. Abs and Negate are written to avoid signed integer overflow when the operand equals INT_MIN. This is achieved by bitcasting the value to unsigned, flipping the bits and adding one, then bitcasting the result back to signed. Division and Modulo avoid undefined bevaviour for INT_MIN / -1 and divide-by-zero by using 1 for the divisor instead. Additionally we avoid undefined behaviour when using the modulo operator when either or both operands are negative by using the equation from the WGSL spec, using division, subtraction and multiplication, rather than MSL's modulus operator. Lastly, as the usage of the wrapper function for unary integer negation makes the negation_avoids_prefix_decrement() testcase less interesting, we extend it to additionally test negating floats.
There is no literal suffix in HLSL for the integer type. We can, however, wrap integer literals in a constructor style cast. This avoids ambiguity when passing literals to overloaded functions, which we'll make use of in the subsequent patch in this PR.
…, subtraction, and multiplication Though not explicitly specified one way or the other, we have been informed by DirectX engineers that signed integer overflow may be undefined behaviour in some cases. To avoid this, we therefore bitcast signed operands to unsigned prior to performing addition, subtraction, or multiplication, then bitcast the result back to signed. As signed types are represented as two's complement, this gives the correct result whilst avoid any potential undefined behaviour. Unfortunately HLSL's bitcast functions asint() and asuint() only work for the 32-bit int and uint types. We therefore only apply this workaround for 32-bit signed arithmetic. Support for other bit widths could be added in the future, but extra care must be taken when converting from unsigned to signed to avoid undefined or implemented-defined behaviour.
…o, negation, and abs Emit helper functions for MathFunction::Abs and UnaryOperator::Negate with a signed integer scalar or vector operand. And for BinaryOperator::Divide and BinaryOperator::Modulo with signed or unsigned integer scalar or vector operands. Abs and Negate are written to avoid signed integer overflow when the operand equals INT_MIN. This is achieved by bitcasting the value to unsigned, flipping the bits and adding one, then bitcasting the result back to signed. As HLSL's bitcast functions asint() and asuint() only work for 32-bit types, we only use this workaround in such cases. Division and Modulo avoid undefined bevaviour for INT_MIN / -1 and divide-by-zero by using 1 for the divisor instead. Additionally we avoid undefined behaviour when using the modulo operator on operands of mixed signedness by using the equation from the WGSL spec, using division, subtraction and multiplication, rather than HLSL's modulus operator.
Integer division or modulo is undefined behaviour in SPIR-V when the divisor is zero, or when the dividend is the most negative number representable by the result type and the divisor is negative one. This patch makes us avoid this undefined behaviour and instead ensures we adhere to the WGSL spec in these cases: for divisions the expression evaluates to the value of the dividend, and for modulos the expression evaluates to zero. Similarily to how we handle these cases for the MSL and HLSL backends, prior to emitting each function we emit code for any helper functions required by that function's expressions. In this case that is helper functions for integer division and modulo. Then, when emitting the actual function's body, if we encounter an expression which needs wrapped we instead emit a function call to the helper.
jimblandy
force-pushed
the
overflow-edgecases
branch
from
46e2945 to
f82adbd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is quite large so is in separate commits both for reviewability and future readability - please land with a rebase rather than squash!!
Connections
Fixes #6961
Description
Signed arithmetic overflow is undefined behaviour as specified in the metal spec, and we have been informed that we cannot rely on it being safe in HLSL either. The simple cases (addition, subtraction, multiplication) are safe in SPIRV, and were handled in #6666 for metal, but still affect the HLSL backend. Additionally, some edge cases remain undefined behaviour for various backends:
This patch series ensures all these cases are handled if necessary for the MSL, HLSL, and SPIRV backends, in line with the WGSL spec's requirements.
Testing
Inspected test snapshots and ensure they still pass validation. Manual testing that compute shader outputs correct values for various operations.
Checklist
cargo fmt.taplo format.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.