Skip to content

Conversation

mwyrzykowski
Copy link
Contributor

@mwyrzykowski mwyrzykowski commented Nov 6, 2024

646adca

[WebGPU] explicit layouts should be validated for compatibility with the shader
https://bugs.webkit.org/show_bug.cgi?id=282710
rdar://138847403

Reviewed by Dan Glastonbury.

There is a bug in our validation code that checks the compatibility between the
variables used by the shader and the bind group provided via the API, and it happens
in two steps:
- first, we start by traversing the API-provided bind group we check if the shader
  contains a variable for that group/binding pair. Here we incorrectly check against
  all the variables in the shader, but we should only consider the variables that
  are actually used, since it's valid to have multiple variables with the same
  binding, so long as they are not used by the same entry point.
- the second part of the code is actually correct, where we only validate the used
  variables against the bind group. However, since we incorrectly selected an unused
  variable to be serialized in the previous step, that variable is never, allowing
  a type mismatch between the generated shader and the bind group.

* LayoutTests/fast/webgpu/nocrash/fuzz-282710-expected.txt: Added.
* LayoutTests/fast/webgpu/nocrash/fuzz-282710.html: Added.
* Source/WebGPU/WGSL/GlobalVariableRewriter.cpp:
(WGSL::RewriteGlobalVariables::insertStructs):

Canonical link: https://commits.webkit.org/286432@main

8ba74e8

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ⏳ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
⏳ 🛠 ios-sim ✅ 🛠 mac-AS-debug loading-orange 🧪 wpe-wk2 loading 🧪 win-tests
✅ 🧪 webkitperl ⏳ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 🛠 wpe-cairo
⏳ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 loading-orange 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@mwyrzykowski mwyrzykowski self-assigned this Nov 6, 2024
@mwyrzykowski mwyrzykowski added the WebGPU For bugs in WebGPU label Nov 6, 2024
Copy link
Contributor

@djg djg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@tadeuzagallo tadeuzagallo force-pushed the eng/WebGPU-explicit-layouts-should-be-validated-for-compatibility-with-the-shader branch from 4f9613a to 8ba74e8 Compare November 8, 2024 17:51
@tadeuzagallo tadeuzagallo self-assigned this Nov 8, 2024
@mwyrzykowski mwyrzykowski added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Nov 8, 2024
@tadeuzagallo tadeuzagallo added safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Nov 11, 2024
@mwyrzykowski mwyrzykowski added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Nov 11, 2024
…the shader

https://bugs.webkit.org/show_bug.cgi?id=282710
rdar://138847403

Reviewed by Dan Glastonbury.

There is a bug in our validation code that checks the compatibility between the
variables used by the shader and the bind group provided via the API, and it happens
in two steps:
- first, we start by traversing the API-provided bind group we check if the shader
  contains a variable for that group/binding pair. Here we incorrectly check against
  all the variables in the shader, but we should only consider the variables that
  are actually used, since it's valid to have multiple variables with the same
  binding, so long as they are not used by the same entry point.
- the second part of the code is actually correct, where we only validate the used
  variables against the bind group. However, since we incorrectly selected an unused
  variable to be serialized in the previous step, that variable is never, allowing
  a type mismatch between the generated shader and the bind group.

* LayoutTests/fast/webgpu/nocrash/fuzz-282710-expected.txt: Added.
* LayoutTests/fast/webgpu/nocrash/fuzz-282710.html: Added.
* Source/WebGPU/WGSL/GlobalVariableRewriter.cpp:
(WGSL::RewriteGlobalVariables::insertStructs):

Canonical link: https://commits.webkit.org/286432@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/WebGPU-explicit-layouts-should-be-validated-for-compatibility-with-the-shader branch from 8ba74e8 to 646adca Compare November 11, 2024 16:10
@webkit-commit-queue
Copy link
Collaborator

Committed 286432@main (646adca): https://commits.webkit.org/286432@main

Reviewed commits have been landed. Closing PR #36284 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 646adca into WebKit:main Nov 11, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WebGPU For bugs in WebGPU

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants