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

Ensure safety of indirect dispatch #5714

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented May 17, 2024

Ensure safety of indirect dispatch by injecting a compute shader that validates the content of the indirect buffer.

Part of #2431.

Depends on #6318.

@teoxoy teoxoy requested a review from a team as a code owner May 17, 2024 15:38
@teoxoy

This comment was marked as resolved.

@cwfitzgerald

This comment was marked as resolved.

@cwfitzgerald cwfitzgerald self-requested a review May 18, 2024 02:26
@teoxoy

This comment was marked as resolved.

jimblandy

This comment was marked as resolved.

@daxpedda

This comment was marked as resolved.

@teoxoy

This comment was marked as resolved.

@teoxoy teoxoy force-pushed the validate-indirect-compute branch from 018b23b to a5bebb0 Compare May 21, 2024 12:14
@teoxoy

This comment was marked as resolved.

@teoxoy teoxoy requested a review from jimblandy May 21, 2024 12:20
@teoxoy teoxoy force-pushed the validate-indirect-compute branch 4 times, most recently from ac3f089 to 36281af Compare May 21, 2024 14:35
@teoxoy

This comment was marked as resolved.

@teoxoy

This comment was marked as resolved.

@teoxoy

This comment was marked as resolved.

@teoxoy

This comment was marked as resolved.

@teoxoy teoxoy force-pushed the validate-indirect-compute branch from b57350e to e0bd41a Compare May 27, 2024 14:25
@teoxoy

This comment was marked as resolved.

@cwfitzgerald

This comment was marked as resolved.

jimblandy

This comment was marked as resolved.

@teoxoy

This comment was marked as resolved.

cwfitzgerald

This comment was marked as resolved.

@teoxoy teoxoy force-pushed the validate-indirect-compute branch 3 times, most recently from ab03bc6 to abeb863 Compare June 11, 2024 19:32
@teoxoy teoxoy requested a review from jimblandy June 11, 2024 20:47
wgpu-core/src/indirect_validation.rs Show resolved Hide resolved
tests/tests/dispatch_workgroups_indirect.rs Outdated Show resolved Hide resolved
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.

See Atomic issue and other small nits

EDIT from @ErichDonGubler: added a link for atomic issue

tests/tests/dispatch_workgroups_indirect.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/bind.rs Outdated Show resolved Hide resolved
wgpu-core/src/indirect_validation.rs Outdated Show resolved Hide resolved
by injecting a compute shader that validates the content of the indirect buffer
@teoxoy
Copy link
Member Author

teoxoy commented Oct 14, 2024

I don't know why github it not letting me "rebase and merge":

image

Via the CLI is also not working.

gh pr merge 5714 -r -R gfx-rs/wgpu
GraphQL: This branch can't be rebased (mergePullRequest)

@teoxoy
Copy link
Member Author

teoxoy commented Oct 14, 2024

I will just squash and merge, losing 6619f3a is not the end of the world.

@teoxoy teoxoy merged commit 7f708ed into gfx-rs:trunk Oct 14, 2024
27 checks passed
@teoxoy teoxoy deleted the validate-indirect-compute branch October 14, 2024 13:02
@schell
Copy link
Contributor

schell commented Oct 15, 2024

I'm tracking trunk in my project and just hit a panic in these changes:

thread 'test::unlit_textured_cube_material' panicked at /Users/schell/.cargo/git/checkouts/wgpu-53e70f8674b08dd4/59f56e0/wgpu-core/src/indirect_validation.rs:288:58:
called `Option::unwrap()` on a `None` value

I'm guessing it's this binding binding_size on line 276 in the file mentioned:

        let binding_size = calculate_src_buffer_binding_size(buffer_size, limits);

being used on line 288:

                size: Some(NonZeroU64::new(binding_size).unwrap()),

...so for some reason in my case binding_size is 0. Maybe the buffer passed is 0? 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants