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

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

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

jimblandy · 2025-02-12T20:47:36Z

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

[naga hlsl-out] Avoid undefined behaviour for integer division, modulo, negation, and abs

jimblandy · 2025-02-12T20:54:47Z

@jamienicol The commit messages are great and much appreciated! But for future PRs, there's a lot there that could serve (even) better as comments in the code, for example:

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 wouldn't be amiss as a comment in the SPIR-V backend.

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.

This would be more useful as comments clarifying the behavior of the Naga IR operators: you'd describe it as Naga IR's semantics, and then note that this aligns with WGSL's operators.

Generally I try to put everything possible into comments in the code, and then let the commit message cite those, and focus on the changes to behavior.

jamienicol · 2025-02-13T12:25:41Z

@jamienicol The commit messages are great and much appreciated! But for future PRs, there's a lot there that could serve (even) better as comments in the code, for example:

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 wouldn't be amiss as a comment in the SPIR-V backend.

This is actually commented here

But I think the comment is absolutely fair and I've pushed extra patches to the PR to add comments for the HLSL and MSL changes.

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.

This would be more useful as comments clarifying the behavior of the Naga IR operators: you'd describe it as Naga IR's semantics, and then note that this aligns with WGSL's operators.

Do you mean we should add comments to eg BinaryOperator::Divide etc declaration, in naga/src/lib.rs?

jimblandy · 2025-02-13T22:59:23Z

Do you mean we should add comments to eg BinaryOperator::Divide etc declaration, in naga/src/lib.rs?

Yeah, although I admit none of the other operators are documented, so it's going to look a little weird. May be a bigger project than belongs in this PR.

naga/src/back/spv/writer.rs

jimblandy · 2025-02-14T22:57:38Z

I've fully reviewed this commit and it looks fine to me:

[naga spv-out] Avoid undefined behaviour for integer division and modulo

jimblandy

Okay, this all looks fantastic! Thank you!

…, 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.

…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.

Since every `match` arm ends up looking up the type of the operation's first argument, just do that once. This avoids a repetitive lookup for `Abs`.

Replace the link to the resolved WGSL spec issue about floating-point division by zero (gpuweb/gpuweb#2798) with links to the Direct3D 11 functional specification (which Direct3D 12 inherits) and the DXIL specification, explaining that HLSL does what WGSL wants here.

…vision, modulo, abs(), and unary negation Explain we need the wrapper functions not just to avoid undefined behaviour (or unspecified in the case of division), but also to ensure we adhere to the WGSL spec.

…ivision, modulo, abs(), and unary negation Explain we need the wrapper functions not just to avoid undefined behaviour, but also to ensure we adhere to the WGSL spec. Additionally link to issue gfx-rs#7109 in cases where our workaround needs follow-up work for non-32-bit integer types.

Move the code to generate the definition of an overflow-safe divide/modulo SPIR-V function into its own Rust function, to reduce indentation and clarify influences. This commit isn't intended to cause any change in behavior.

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

Backends should not need two passes to handle wrapped functions #7106

Open

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 UB #7109

Open

jimblandy force-pushed the overflow-edgecases branch 5 times, most recently from dfe1d7d to 6c7d5fa Compare February 12, 2025 19:11

jimblandy force-pushed the overflow-edgecases branch 2 times, most recently from cc41985 to 8b2790d Compare February 14, 2025 22:33

jimblandy reviewed Feb 14, 2025

View reviewed changes

naga/src/back/spv/writer.rs Show resolved Hide resolved

jimblandy force-pushed the overflow-edgecases branch from 8b2790d to 2078e4c Compare February 14, 2025 22:59

jimblandy approved these changes Feb 14, 2025

View reviewed changes

jimblandy enabled auto-merge (rebase) February 14, 2025 23:02

jimblandy disabled auto-merge February 14, 2025 23:02

jamienicol and others added 11 commits February 14, 2025 15:03

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

1f3d758

[hlsl-out] Clean up repetition in write_mapped_math_functions.

fad3311

Since every `match` arm ends up looking up the type of the operation's first argument, just do that once. This avoids a repetitive lookup for `Abs`.

[naga msl-out] Document the need for wrapper functions for integer di…

b836664

…vision, modulo, abs(), and unary negation Explain we need the wrapper functions not just to avoid undefined behaviour (or unspecified in the case of division), but also to ensure we adhere to the WGSL spec.

[naga spv-out] Factor out wrapped divide/module generation.

c6ce6ce

Move the code to generate the definition of an overflow-safe divide/modulo SPIR-V function into its own Rust function, to reduce indentation and clarify influences. This commit isn't intended to cause any change in behavior.

jimblandy force-pushed the overflow-edgecases branch from 2078e4c to c6ce6ce Compare February 14, 2025 23:09

jimblandy enabled auto-merge (rebase) February 14, 2025 23:10

jimblandy merged commit f90f19c into gfx-rs:trunk Feb 14, 2025
34 checks passed

jamienicol deleted the overflow-edgecases branch February 14, 2025 23:46

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

jimblandy commented Feb 12, 2025

jimblandy commented Feb 12, 2025 •

edited

Loading

jamienicol commented Feb 13, 2025

jimblandy commented Feb 13, 2025

jimblandy commented Feb 14, 2025

jimblandy left a comment

	"{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

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

jimblandy commented Feb 12, 2025

jimblandy commented Feb 12, 2025 • edited Loading

jamienicol commented Feb 13, 2025

jimblandy commented Feb 13, 2025

jimblandy commented Feb 14, 2025

jimblandy left a comment

Choose a reason for hiding this comment

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

jimblandy commented Feb 12, 2025 •

edited

Loading