Handle more cases of undefined behaviour due to signed integer overflow or division by zero #7012

jamienicol · 2025-01-28T12:18:27Z

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:

INT_MIN / -1
INT_MIN % -1
signed or unsigned / 0
signed or unsigned % 0
abs(INT_MIN)
-INT_MIN

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

Run cargo fmt.
Run taplo format.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- --target wasm32-unknown-emscripten
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

jimblandy

Still reviewing, some comments so far:

jimblandy · 2025-02-07T20:04:38Z

naga/src/back/hlsl/help.rs

+                        let level = crate::back::Level(1);
+                        writeln!(
+                            self.out,
+                            "{level}return val >= 0 ? val : asint(~asuint(val) + 1u);"


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));"

ah unsigned negation never even occurred to me. Can't see anything in the respective specs, but the shaders appear to pass validation so presumably it's supported.

jimblandy · 2025-02-07T20:05:05Z

naga/src/back/hlsl/help.rs

+                        writeln!(self.out, " val) {{")?;
+
+                        let level = crate::back::Level(1);
+                        writeln!(self.out, "{level}return asint(~asuint(val) + 1u);",)?;


Similarly:

Suggested change

writeln!(self.out, "{level}return asint(~asuint(val) + 1u);",)?;

writeln!(self.out, "{level}return asint(-asuint(val));",)?;

jimblandy · 2025-02-07T20:11:38Z

naga/src/back/hlsl/help.rs

+                    func_ctx
+                        .resolve_type(expr_handle, &module.types)
+                        .scalar_kind(),


Isn't this just expr_ty?

jimblandy · 2025-02-07T20:12:15Z

naga/src/back/hlsl/help.rs

+                        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!(),
+                        };


Aren't these resolve_type calls exactly left_ty and right_ty, already computed above?

jimblandy · 2025-02-07T20:13:49Z

naga/src/back/hlsl/help.rs

+                let (scalar, vector_size) = match *expr_ty {
+                    crate::TypeInner::Scalar(scalar) => (scalar, None),
+                    crate::TypeInner::Vector { size, scalar } => (scalar, Some(size)),
+                    _ => continue,
+                };


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?

done. the helpers in proc/mod.rs seem to have been recently moved to proc/type_methods.rs so that's where I put it

jimblandy · 2025-02-07T20:33:11Z

naga/src/back/hlsl/help.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);


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.

jimblandy · 2025-02-07T20:37:29Z

naga/src/back/hlsl/writer.rs

@@ -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})")?,


Why is this necessary? This should have a comment, at least.

jimblandy · 2025-02-07T20:38:27Z

naga/src/back/hlsl/writer.rs

+                self.write_expr(module, right, func_ctx)?;
+                write!(self.out, ")")?;
+            }
+
            // TODO: handle undefined behavior of BinaryOperator::Modulo


Can we delete this comment too?

I believe we still need to handle division by zero for floating points

Where does HLSL say floating-point division by zero is UB? It seems like they usually follow IEEE.

This table is describing DXIL, and seems to imply that it's defined.

https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/DXIL.rst#fdiv

More links: https://microsoft.github.io/DirectX-Specs/d3d/archive/D3D11_3_FunctionalSpec.htm#3.1%20Floating%20Point%20Rules

Sorry I meant to say modulo not division. But you might be right we can remove the comment, or at least have to update it. The modulus operator says it's undefined unless both operands are positive or negative. But we don't use the modulus operator, we use fmod(). This doesn't say anything about being undefined. But we do need to check whether it has the same semantics as WGSL..

Removed the reference to undefined behaviour, but left the "TODO: if right == 0 return ?" part for now.

jimblandy · 2025-02-07T22:12:19Z

This also fixes #4385, I think.

jimblandy

Still reviewing, just posting some comments

jimblandy · 2025-02-07T22:06:45Z

naga/src/back/hlsl/writer.rs

+pub(crate) const ABS_FUNCTION: &str = "naga_abs";
+pub(crate) const DIV_FUNCTION: &str = "naga_div";
+pub(crate) const MOD_FUNCTION: &str = "naga_mod";
+pub(crate) const NEG_FUNCTION: &str = "naga_neg";


These need to be added to back::hlsl::keywords::RESERVED, so that they won't conflict with the user's identifiers.

jimblandy · 2025-02-07T22:10:18Z

naga/src/back/msl/writer.rs

+pub(crate) const ABS_FUNCTION: &str = "naga_abs";
+pub(crate) const DIV_FUNCTION: &str = "naga_div";
+pub(crate) const MOD_FUNCTION: &str = "naga_mod";
+pub(crate) const NEG_FUNCTION: &str = "naga_neg";


As with HLSL, you need to tell the namer about these.

jimblandy · 2025-02-11T01:31:16Z

I like the infrastructure added to the Metal backend for this. Filed #7105 to take advantage of it more.

jimblandy · 2025-02-11T02:12:53Z

I've fully reviewed the following commits and they look good to me:

[naga msl-out] Avoid undefined behaviour for integer division, modulo, negation, and abs
[naga hlsl-out] Wrap int literals in constructor-style cast
[naga hlsl-out] Avoid undefined behaviour for signed integer addition, subtraction, and multiplication

…, 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 and using the negation operator, 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.

jimblandy · 2025-02-11T21:57:11Z

Filed #7109 for non-32-bit integer overflow UB in HLSl.

…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, using the negation operator, 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 behaviour 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.

jamienicol requested a review from a team January 28, 2025 12:18

cwfitzgerald assigned jimblandy Jan 29, 2025

jimblandy reviewed Feb 7, 2025

View reviewed changes

jimblandy force-pushed the overflow-edgecases branch from 46e2945 to f82adbd Compare February 8, 2025 02:05

jamienicol force-pushed the overflow-edgecases branch 3 times, most recently from 902a598 to f44b83b Compare February 10, 2025 10:29

jimblandy reviewed Feb 10, 2025

View reviewed changes

jimblandy mentioned this pull request Feb 11, 2025

Naga Metal backend should write dot products as a wrapped function #7105

Open

jimblandy mentioned this pull request Feb 11, 2025

Metal backend should not need two passes to handle wrapped functions #7106

Open

jamienicol added 3 commits February 11, 2025 09:09

jamienicol force-pushed the overflow-edgecases branch from f44b83b to cec38f3 Compare February 11, 2025 09:12

jimblandy mentioned this pull request Feb 11, 2025

Non-32-bit integer overflow should be well-defined in Naga-enhanced WGSL, but Naga generates HLSL with IB #7109

Open

jamienicol added 2 commits February 11, 2025 17:34

jimblandy force-pushed the overflow-edgecases branch from df8ff83 to ab352b9 Compare February 12, 2025 01:35

[naga hlsl-out] Use Scalar associated constants in patterns.

c1ddb37

jimblandy force-pushed the overflow-edgecases branch from ab352b9 to c1ddb37 Compare February 12, 2025 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle more cases of undefined behaviour due to signed integer overflow or division by zero #7012

Handle more cases of undefined behaviour due to signed integer overflow or division by zero #7012

jamienicol commented Jan 28, 2025 •

edited

Loading

jimblandy left a comment

jimblandy Feb 7, 2025

jamienicol Feb 10, 2025

jimblandy Feb 7, 2025

jimblandy Feb 7, 2025

jamienicol Feb 10, 2025

jimblandy Feb 7, 2025 •

edited

Loading

jamienicol Feb 10, 2025

jimblandy Feb 7, 2025

jamienicol Feb 10, 2025

jimblandy Feb 7, 2025

jimblandy Feb 7, 2025

jamienicol Feb 10, 2025

jimblandy Feb 7, 2025

jamienicol Feb 10, 2025 •

edited

Loading

jimblandy Feb 10, 2025

jimblandy Feb 10, 2025

jimblandy Feb 11, 2025

jamienicol Feb 11, 2025

jamienicol Feb 11, 2025

jimblandy commented Feb 7, 2025

jimblandy left a comment

jimblandy Feb 7, 2025

jimblandy Feb 7, 2025

jimblandy commented Feb 11, 2025

jimblandy commented Feb 11, 2025 •

edited

Loading

jimblandy commented Feb 11, 2025

	"{level}return val >= 0 ? val : asint(~asuint(val) + 1u);"
	"{level}return val >= 0 ? val : asint(-asuint(val));"

	writeln!(self.out, "{level}return asint(~asuint(val) + 1u);",)?;
	writeln!(self.out, "{level}return asint(-asuint(val));",)?;

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

Handle more cases of undefined behaviour due to signed integer overflow or division by zero #7012

Are you sure you want to change the base?

Handle more cases of undefined behaviour due to signed integer overflow or division by zero #7012

Conversation

jamienicol commented Jan 28, 2025 • edited Loading

jimblandy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblandy Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamienicol Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblandy commented Feb 7, 2025

jimblandy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimblandy commented Feb 11, 2025

jimblandy commented Feb 11, 2025 • edited Loading

jimblandy commented Feb 11, 2025

jamienicol commented Jan 28, 2025 •

edited

Loading

jimblandy Feb 7, 2025 •

edited

Loading

jamienicol Feb 10, 2025 •

edited

Loading

jimblandy commented Feb 11, 2025 •

edited

Loading