Skip to content

fix(naga): Reject direct access to atomic variables#9262

Open
ecoricemon wants to merge 2 commits intogfx-rs:trunkfrom
ecoricemon:reject-direct-atomic
Open

fix(naga): Reject direct access to atomic variables#9262
ecoricemon wants to merge 2 commits intogfx-rs:trunkfrom
ecoricemon:reject-direct-atomic

Conversation

@ecoricemon
Copy link
Contributor

@ecoricemon ecoricemon commented Mar 19, 2026

Connections
Fixes issue #5474
A follow-up PR for #9126

Description
This PR raises a WGSL front-end error in apply_load_rule when the load rule is applied to a reference whose pointee type is atomic.

  • Test changes
    The existing invalid_functions test for return_atomic was slightly adjusted. The original test used a bare atomic return type; since the new front-end error fires during module lowering before the validator runs, the test was updated to use a struct wrapping an atomic so that NonConstructibleReturnType is still exercised through the validator as originally intended.

  • Affected CTS
    webgpu:shader,validation,expression,binary,add_sub_mul:* : 901 -> 904 (100%)
    webgpu:shader,validation,expression,binary,bitwise_shift:invalid_types:* : 26 -> 28 (100%)
    webgpu:shader,validation,expression,call,builtin,abs:* : 56 -> 57 (100%)
    webgpu:shader,validation,expression,call,builtin,countLeadingZeros:* : 47 -> 48 (100%)
    webgpu:shader,validation,expression,call,builtin,countOneBits:* : 47 -> 48 (100%)
    webgpu:shader,validation,expression,call,builtin,countTrailingZeros:* : 47 -> 48 (100%)
    webgpu:shader,validation,expression,call,builtin,firstLeadingBit:* : 47 -> 48 (100%)
    webgpu:shader,validation,expression,call,builtin,firstTrailingBit:* : 47 -> 48 (100%)
    webgpu:shader,validation,expression,call,builtin,reverseBits:* : 47 -> 48 (100%)
    webgpu:shader,validation,expression,call,builtin,select:* : 842 -> 843 (100%)
    webgpu:shader,validation,expression,matrix,* : 1863 -> 1881 (100%)
    webgpu:shader,validation,expression,unary,* : 302 -> 304 (100%)
    webgpu:shader,validation,statement,increment_decrement:* : 218 -> 222 (100%)
    webgpu:shader,validation,statement,phony:* : 58 -> 62 (100%)
    webgpu:shader,validation,statement,switch:* : 87 -> 88 (100%)
    webgpu:shader,validation,expression,binary,and_or_xor:* : 821 -> 824 (99.76%)
    webgpu:shader,validation,expression,binary,comparison:* : 468 -> 474 (99.58%)
    webgpu:shader,validation,expression,call,builtin,clamp:* : 161 -> 162 (71.68%)
    webgpu:shader,validation,expression,call,builtin,extractBits:* : 29 -> 33 (62.26%)
    webgpu:shader,validation,expression,call,builtin,insertBits:* : 85 -> 89 (76.07%)
    webgpu:shader,validation,types,* : 1450 -> 1453 (95.22%)

Uncertainty
I'm not fully confident this is the right approach. Please review. If the approach looks correct, I will identify any affected CTS tests and update them accordingly.

Testing
Existing tests

Squash or Rebase?
Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. 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.

@ecoricemon ecoricemon marked this pull request as ready for review March 19, 2026 13:55
@cwfitzgerald
Copy link
Member

I think this also needs to be validated against in the validator otherwise spirv and glsl will still have this problem. It may still be useful for error message purposes to keep the validation in the front end

@ecoricemon
Copy link
Contributor Author

Thank you for the feedback. It sounds like the best path forward is to introduce a new expression variant, Expression::AtomicLoad, and then verify its pointee type during the validation stage. I’ll investigate this approach and update the PR accordingly.

@ecoricemon ecoricemon marked this pull request as draft March 19, 2026 22:57
@cwfitzgerald
Copy link
Member

Oh I see, this is actually just a wgsl front end issue, an atomic load is expressed as an Expression::Load on an atomic type, so a non-atomic load is actually not even expressible in the IR as is.

So this PR is fine as is, sorry for the misunderstanding!

@ecoricemon
Copy link
Contributor Author

ecoricemon commented Mar 19, 2026

No problem at all! I'll update affected CTS and the PR shortly. Please let me know if you spot any other potential issues with this approach.

@ecoricemon ecoricemon force-pushed the reject-direct-atomic branch from 459e8f3 to 8fa745f Compare March 20, 2026 11:54
@ecoricemon ecoricemon marked this pull request as ready for review March 20, 2026 12:24
@andyleiserson
Copy link
Contributor

I ran the naga benchmark (cargo bench --bench wgpu-benchmark -- naga::front) just in case this affects performance, but any difference is within the noise.

@teoxoy
Copy link
Member

teoxoy commented Mar 20, 2026

This looks ok but I think we should really be moving atomic load/store into new variants of AtomicFunction. Should be done separately as this is a refactor that will touch lots of places.

@ecoricemon ecoricemon force-pushed the reject-direct-atomic branch from 8fa745f to 462e3a4 Compare March 20, 2026 22:33
@ecoricemon
Copy link
Contributor Author

Thank you for the comments. I've resolved the conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants