fix: don't depend on BG{,L} layout entry order in HAL #5421
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ErichDonGubler
force-pushed
the
create-bgl-resource-ordering
branch
from
e7d2b8e to
dea6a96
Compare
ErichDonGubler
added
area: correctness
ErichDonGubler
force-pushed
the
create-bgl-resource-ordering
branch
2 times, most recently
from
0adbd36 to
a709356
Compare
ErichDonGubler
changed the title
ErichDonGubler
force-pushed
the
create-bgl-resource-ordering
branch
from
44946ae to
c0aeb31
Compare
This isn't guaranteed by `wgpu-core`; we should try to match by binding slot index instead.
This isn't guaranteed by `wgpu-core`; we should try to match by binding slot index instead.
This isn't guaranteed by `wgpu-core`; we should try to match by binding slot index instead.
ErichDonGubler
force-pushed
the
create-bgl-resource-ordering
branch
from
c0aeb31 to
38eb30e
Compare
ErichDonGubler
added
the
PR: needs back-porting
ErichDonGubler
commented
Apr 1, 2024
jimblandy
approved these changes
Apr 1, 2024
There was a problem hiding this comment.
We need a followup bug for the quadratic behavior, but other than that, this looks fantastic. Thank you so much for writing a test.
Edit from @ErichDonGubler: Added in #5473.
cwfitzgerald
removed
the
PR: needs back-porting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
Description
Multiple backends for
wgpu-halcurrently make assumptions about the ordering of descriptor entries provided for bind groups and bind group layouts that are invalid. In particular, it was assumed that the element order of resource entries stored inVecs in bind group layouts were the same for bind group resources entries stored inVecs. This caused binding slot order to become out-of-sync when actually creating bind groups from descriptors and bind group layouts with code likedesc.entries.iter().zip(desc.layout.entries.iter()), bypassing essential validation for, i.e., type matching in resource entries, and usage masks for buffers and textures. At best, this caused crashes because of different numbers of types of resources being used (as observed in Firefox's bug 1877461, comment 3). At worst, this caused resources of (of the same and different) types to be bound in the wrong places. We in Firefox noticed because of crashes and putting graphics drivers into invalid states. I'm sure there are other potential types of damage that are even worse. 😬Testing
The test
wgpu_test::different_bgl_order_bw_shader_and_apihas been added, which exercises the previously crashing case where more than one resource type was being used in a bind group layout. This doesn't cover cases of resources of the same type having their order mixed up, but it should be sufficient to prevent a significant regression.Checklist
cargo fmt.cargo clippy. If applicable, add:--target wasm32-unknown-unknown--target wasm32-unknown-emscriptencargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.