Skip to content

Conversation

tadeuzagallo
Copy link
Member

@tadeuzagallo tadeuzagallo commented Nov 13, 2024

19c23eb

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

Reviewed by Mike Wyrzykowski.

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.

This previous landed in 286432@main, but there was an issue as we were trying to
generate buffer lengths for the variables that were already used for buffer lengths.
In order to fix that I kept the old the check that shader contains the variable, on
top of checking whether it's used.

* 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/286585@main

f450494

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 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 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ❌ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@tadeuzagallo tadeuzagallo self-assigned this Nov 13, 2024
@tadeuzagallo tadeuzagallo added the WebGPU For bugs in WebGPU label Nov 13, 2024
@tadeuzagallo tadeuzagallo force-pushed the eng/WebGPU-explicit-layouts-should-be-validated-for-compatibility-with-the-shader branch from 9425ee6 to f450494 Compare November 13, 2024 16:41
Copy link
Contributor

@mwyrzykowski mwyrzykowski left a comment

Choose a reason for hiding this comment

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

Thank you!

@tadeuzagallo tadeuzagallo added the merge-queue Applied to send a pull request to merge-queue label Nov 14, 2024
…the shader

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

Reviewed by Mike Wyrzykowski.

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.

This previous landed in 286432@main, but there was an issue as we were trying to
generate buffer lengths for the variables that were already used for buffer lengths.
In order to fix that I kept the old the check that shader contains the variable, on
top of checking whether it's used.

* 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/286585@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/WebGPU-explicit-layouts-should-be-validated-for-compatibility-with-the-shader branch from f450494 to 19c23eb Compare November 14, 2024 10:05
@webkit-commit-queue
Copy link
Collaborator

Committed 286585@main (19c23eb): https://commits.webkit.org/286585@main

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

@webkit-commit-queue webkit-commit-queue merged commit 19c23eb into WebKit:main Nov 14, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 14, 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.

4 participants