Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make wgpu_render_pass_set_bind_group clamp the number of dynamic offsets within limits. #4918

Closed
wants to merge 4 commits into from

Conversation

bradwerth
Copy link
Contributor

@bradwerth bradwerth commented Dec 21, 2023

This similarly updates wgpu_compute_pass_set_bind_group. Neither function throws an error when clamping. It's not clear what mechanism there is for error reporting at this entry point.

Connections
N/A

Description
User agents calling wgpu_compute_pass_set_bind_group directly can provide an arbitrary number of dynamic offsets without passing validation.

Testing
Untested. Any test using something like cpass.set_bind_group is stopped by validation.

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@bradwerth bradwerth requested a review from a team as a code owner December 21, 2023 18:54
@nical nical enabled auto-merge (squash) January 2, 2024 08:47
@@ -919,9 +919,17 @@ pub mod compute_ffi {
return;
}

// Clamp the number of dynamic offsets to the maximum value that the
Copy link
Member

Choose a reason for hiding this comment

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

For validation we should include the original length, so that in the body of the pass we can check to make sure the original_length == clamped length, and error if they don't match.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Briefly looking at the code around num_dynamic_offsets it looks to me like it would make more sense to just change its data type to be usize since every time we set it we cast it to u8 and every time we read it we cast it back to usize.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

For bookkeeping, going to request changes

@nical
Copy link
Contributor

nical commented Jan 9, 2024

Addressing this differently in #5026

@nical nical closed this Jan 9, 2024
auto-merge was automatically disabled January 9, 2024 13:31

Pull request was closed

@bradwerth bradwerth deleted the numDynamicOffsetsU8 branch January 16, 2024 22:09
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