[msl-out] Fix `ReadZeroSkipWrite` bounds check mode for pointer arguments #7323

andyleiserson · 2025-03-12T01:50:50Z

Connections
Fixes #4541

Description
See the description of #4541 for more detail and sample code, but in short, the ReadZeroSkipWrite bounds check policy didn't previously work when the result of the indexing operation was itself a pointer that then gets passed to a function. With this change, a dummy local variable is created for each type that may appear in this manner. If a bounds check fails, the address of the dummy variable is substituted as the argument value for the called function.

This change is based on gfx-rs/naga#2451.

Testing
Added snapshot tests for the test case from the issue and a variety of other cases. I also ran the output from naga through the metal compiler to verify it is now accepted.

Checklist

Run cargo fmt.
Run taplo format.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
Run cargo xtask test to run tests.
If this contains user-facing changes, add a CHANGELOG.md entry.

andyleiserson · 2025-03-14T19:41:44Z

After spending more time with the spec, I think we'd be justified in disallowing this case (pointer to array element) just like pointer to matrix or vector component, at least until we want to claim support for unrestricted_pointer_parameters.

ErichDonGubler · 2025-04-03T17:30:32Z

Two things have been complicating my ability to get to this review:

Some protracted time during which I haven't felt like I could prioritize this against my other work. Despite this, I think this should have been reviewed much earlier, and I apologize for this; reviews from maintainers deserve a much faster turnaround than what I've done here (3 weeks!? 😱).
Actually understanding the different changes in this PR's single commit. It took me a few hours to peel apart the commit into multiple ones that can stand on their own, and I'm feeling confident enough that the changes are distinct enough for me to review meaningfully.

I also pushed some fixup!s that came up in my review; I'll refer to the commits in my feedback that's soon to follow today.

ErichDonGubler

This mostly LGTM, with some minor issues.

_{I used Conventional Comments in this review! I hope they help with clarity and tone. 🙂}

ErichDonGubler · 2025-03-14T20:27:33Z

naga/src/back/msl/writer.rs

+        Item = (
+            Handle<crate::Expression>,
+            index::GuardedIndex,
+            index::IndexableLength,
+        ),


thought: This return type is complicated enough that some follow-up at some point to refactor to a named struct might be nice.

I went ahead and did this, not hard and it's new (or at least newly more complicated) code.

naga/src/back/msl/writer.rs

ErichDonGubler · 2025-04-03T17:32:54Z

naga/src/back/msl/writer.rs

+                        let result_ty_handle = match result_ty {
+                            Some(TypeResolution::Handle(handle)) => handle,
+                            Some(TypeResolution::Value(_)) => {
+                                // I don't have a succinct argument why this is the case.


nitpick: This is a collective artifact, so writing commentary in first person without a note of authorship is, at best, detail without context. Let's either get your name in here, or re-write this to simply acknowledge that this might not be correct (independent of trying to make this correct, which I'll address in a separate comment) and that we can collectively revise as needed.

I reworded the comment, although I'm also curious what your thoughts were on "trying to make this correct" -- I don't see another comment about it. I was hoping for some scrutiny on this from reviewers, so thank you for that.

ErichDonGubler · 2025-04-03T17:37:07Z

naga/src/back/msl/writer.rs

@@ -611,6 +611,13 @@ trait NameKeyExt {
            FunctionOrigin::EntryPoint(idx) => NameKey::EntryPointLocal(idx, local_handle),
        }
    }
+
+    fn oob_local(origin: FunctionOrigin, ty: Handle<crate::Type>) -> NameKey {


question: It's rather confusing to only use the noun local when there are types at play here rather than just local variables.

suggestion: Let's rename to something like s/local/local_type.

I see how it is confusing, but I'm not sure that oob_local_type is better since the thing we're naming really is a local, not a type (it's just that there is one per type, and we name it based on the type it's for).

I added a bit more documentation that may help clarify this. If you still think the names are confusing, maybe s/oob_locals/oob_local_types and s/oob_local/oob_local_for_type?

oob_local_for_type seems like a good option to me!

naga/src/back/msl/writer.rs

ErichDonGubler · 2025-04-03T17:52:46Z

Rebased snapshots onto recent changes merged into trunk from #7414.

ErichDonGubler · 2025-04-09T14:14:58Z

naga/src/back/msl/mod.rs

+## Pointer-typed bounds-checked expressions and OOB locals
+
+MSL (unlike HLSL and GLSL) has native support for pointer-typed function
+arguments. When the [`BoundsCheckPolicy`] is `ReadZeroSkipWrite` and an


nitpick: It'd be nice to doc-link ReadZeroSkipWrite here, but I see that some existing commentary has also not linked it.

andyleiserson · 2025-04-09T19:22:37Z

Github seems not to be triggering the CI jobs any more. I am going to try closing and reopening.

-- Co-authored-by: Liam Murphy <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

…ents Fixes gfx-rs#4541 -- Co-authored-by: Liam Murphy <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

Co-Authored-By: Erich Gubler <[email protected]>

ErichDonGubler self-assigned this Mar 12, 2025

andyleiserson force-pushed the bounds-check branch from c969482 to 1d40580 Compare March 26, 2025 00:24

ErichDonGubler force-pushed the bounds-check branch from 1d40580 to b5303f4 Compare April 3, 2025 17:26

ErichDonGubler requested changes Apr 3, 2025

View reviewed changes

ErichDonGubler added backend: metal Issues with Metal area: naga back-end Outputs of naga shader conversion naga Shader Translator lang: Metal Metal Shading Language labels Apr 3, 2025

ErichDonGubler added this to WebGPU for Firefox Apr 3, 2025

github-project-automation bot moved this to Todo in WebGPU for Firefox Apr 3, 2025

ErichDonGubler moved this from Todo to In Progress in WebGPU for Firefox Apr 3, 2025

ErichDonGubler force-pushed the bounds-check branch from b5303f4 to 778c899 Compare April 3, 2025 17:52

andyleiserson force-pushed the bounds-check branch from da91497 to bb3d4ec Compare April 9, 2025 00:08

ErichDonGubler reviewed Apr 9, 2025

View reviewed changes

ErichDonGubler approved these changes Apr 9, 2025

View reviewed changes

andyleiserson closed this Apr 9, 2025

github-project-automation bot moved this from In Progress to Done in WebGPU for Firefox Apr 9, 2025

andyleiserson reopened this Apr 9, 2025

github-project-automation bot moved this from Done to In Progress in WebGPU for Firefox Apr 9, 2025

andyleiserson and others added 7 commits April 9, 2025 18:30

refactor(msl-out): extract bounds_check_iter helper

ebe190e

-- Co-authored-by: Liam Murphy <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

refactor(msl-out): add NamedKeyExt trait with local helper method

7dcd7c3

-- Co-authored-by: Liam Murphy <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

docs(msl-out): add missing docs. for msl::Writer::access_needs_check

fe6bd4e

-- Co-authored-by: Liam Murphy <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

chore(msl-out): remove TRACE-level logging of expressions

f578878

refactor(msl-out): extract puts_local helper

53cd1d7

-- Co-authored-by: Liam Murphy <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

[msl-out] Fix ReadZeroSkipWrite bounds check mode for pointer argum…

bb87d19

…ents Fixes gfx-rs#4541 -- Co-authored-by: Liam Murphy <[email protected]> Co-Authored-By: Erich Gubler <[email protected]>

refactor(msl-out): create a type for bounds check iter items

1bc6e8d

Co-Authored-By: Erich Gubler <[email protected]>

ErichDonGubler force-pushed the bounds-check branch from 846a199 to 1bc6e8d Compare April 9, 2025 22:35

ErichDonGubler enabled auto-merge (rebase) April 9, 2025 22:36

ErichDonGubler merged commit a0dbe5e into gfx-rs:trunk Apr 9, 2025
37 checks passed

github-project-automation bot moved this from In Progress to Done in WebGPU for Firefox Apr 9, 2025

andyleiserson deleted the bounds-check branch April 10, 2025 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[msl-out] Fix `ReadZeroSkipWrite` bounds check mode for pointer arguments #7323

[msl-out] Fix `ReadZeroSkipWrite` bounds check mode for pointer arguments #7323

Uh oh!

andyleiserson commented Mar 12, 2025

Uh oh!

andyleiserson commented Mar 14, 2025

Uh oh!

ErichDonGubler commented Apr 3, 2025

Uh oh!

ErichDonGubler left a comment

Uh oh!

ErichDonGubler Mar 14, 2025

Uh oh!

andyleiserson Apr 8, 2025

Uh oh!

Uh oh!

ErichDonGubler Apr 3, 2025 •

edited

Loading

Uh oh!

andyleiserson Apr 8, 2025

Uh oh!

ErichDonGubler Apr 3, 2025

Uh oh!

andyleiserson Apr 9, 2025

Uh oh!

ErichDonGubler Apr 9, 2025

Uh oh!

Uh oh!

ErichDonGubler commented Apr 3, 2025

Uh oh!

ErichDonGubler Apr 9, 2025

Uh oh!

andyleiserson commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

[msl-out] Fix ReadZeroSkipWrite bounds check mode for pointer arguments #7323

[msl-out] Fix ReadZeroSkipWrite bounds check mode for pointer arguments #7323

Uh oh!

Conversation

andyleiserson commented Mar 12, 2025

Uh oh!

andyleiserson commented Mar 14, 2025

Uh oh!

ErichDonGubler commented Apr 3, 2025

Uh oh!

ErichDonGubler left a comment

Choose a reason for hiding this comment

Uh oh!

ErichDonGubler Mar 14, 2025

Choose a reason for hiding this comment

Uh oh!

andyleiserson Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ErichDonGubler Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyleiserson Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

ErichDonGubler Apr 3, 2025

Choose a reason for hiding this comment

Uh oh!

andyleiserson Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

ErichDonGubler Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ErichDonGubler commented Apr 3, 2025

Uh oh!

ErichDonGubler Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

andyleiserson commented Apr 9, 2025

Uh oh!

Uh oh!

Uh oh!

[msl-out] Fix `ReadZeroSkipWrite` bounds check mode for pointer arguments #7323

[msl-out] Fix `ReadZeroSkipWrite` bounds check mode for pointer arguments #7323

ErichDonGubler Apr 3, 2025 •

edited

Loading