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

Add InvalidGroupIndex validation at create_shader_module #2775

Merged
merged 1 commit into from
Jun 16, 2022

Conversation

jinleili
Copy link
Contributor

Connections
#2526

Testing
Tested locally, now, if an invalid group index is used, the following validation message will be output:

Caused by:
    In Device::create_shader_module
    shader global ResourceBinding { group: 4, binding: 0 } uses a group index 4 that exceeds the max_bind_groups limit of 4.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

I think this error should be detected in wgpu_core::command::render::State::is_ready, not in create_shader_module. It looks like render bundle encoding doesn't detect this problem either.

The WebGPU spec says that these errors should be reported when a draw command is encoded, by the "Validate encoder bind groups" algorithm. Since setBindGroup refuses to set bind groups at indices greater than this.[[device]].[[limits]].maxBindGroups, the "Validate encoder bind groups" algorithm will fail if the module uses bind group numbers that are out of range.

It's important that wgpu follow the WebGPU specification carefully regarding when errors are reported. The WebGPU Conformance Test Suite will treat variant behaviors as failures - appropriately, because they mean that WebGPU behavior won't be portable across different environments.

@jinleili
Copy link
Contributor Author

If detect in is_ready,will need to repeat the detection code in render::State::is_ready and compute::State::is_ready.
Is it acceptable to move detection to wgpu_core::validation::Interface::check_stage ?

In addition, looking at google dawn is also done in the initialization phase of ShaderModule:
https://dawn.googlesource.com/dawn/+/refs/heads/main/src/dawn/native/ShaderModule.cpp#871

@jimblandy
Copy link
Member

I'm sorry - I've looked at the WebGPU spec more, and it looks to me like this is an unfinished area of the spec, so it's not important for us to comply exactly at this point. In particular, in the default pipeline layout algorithm, I noticed:

  • "ISSUE 35" says that the details of how a shader module's bindings are exposed are not yet settled.
  • Step 3.12.1 says, "Append entry to groupDescs[group]." There's no validation of group specified before this point, so this would be an out-of-bounds access if the resource's group attribute were out of range.

So my inference that the error was meant to be reported at draw call time isn't supported - it needs to be validated before that, at least at pipeline creation time.

Furthermore, "ISSUE 31" says that the rules for module validation are unsettled. So clearly this area is still being developed.

Doing the check at shader module creation time seems friendliest, so let's just go with that.

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

This seems like the right approach, until the spec gives us more specific guidance.

@jimblandy jimblandy merged commit f2c3d42 into gfx-rs:master Jun 16, 2022
@jinleili jinleili deleted the err_msg branch June 16, 2022 22:52
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.

2 participants