Reject user locations and return types for compute shaders - #10026
Reject user locations and return types for compute shaders#10026ErichDonGubler wants to merge 3 commits into
Conversation
ErichDonGubler
added
area: correctness
| naga::valid::ValidationError::EntryPoint { | ||
| stage: naga::ShaderStage::Compute, | ||
| source: naga::valid::EntryPointError::Argument( | ||
| 0, | ||
| naga::valid::VaryingError::InvalidAttributeInStage( | ||
| "location", | ||
| naga::ShaderStage::Compute | ||
| ), | ||
| ), | ||
| .. | ||
| }, |
There was a problem hiding this comment.
thought: It'd be nice to have errors that actually point to the offending front-end span. I have some WGSL front-end changes that can do this, but it would be redundant with the validation added here, and I'm concerned that duplication might cause more problems than it would solve for us.
There was a problem hiding this comment.
I thought we had a bug for this, but couldn't immediately find it in 30 seconds of searching.
There have been some times that I've added redundant errors in the front end to give diagnostics, but I think that's mostly when it was directly adjacent to things I had to do in the front end anyways.
Even if getting good diagnostics from validation is hard, it seems better to invest time in that than in duplicating things in the front-end.
For entrypoint errors specifically, there's hopefully not too much uncertainty about where in the code the problem occurs.
480950c to
20142d0
Compare
andyleiserson
left a comment
andyleiserson
left a comment
There was a problem hiding this comment.
Nice!
Looks like you could also enable the CTS test webgpu:shader,validation,shader_io,locations:stage_inout:*.
| } | ||
| Ok(()) | ||
| }; | ||
| reject_location_binding(fa.binding.as_ref())?; |
There was a problem hiding this comment.
It might make sense to put this in validate_impl? That is more directly for validating bindings, and I think is already called once per binding, so the check could be simplified to something like this in the section for Binding::Location:
if self.stage == crate::ShaderStage::Compute {
return Err(VaryingError::InvalidAttributeInStage(...));
}| if ep.stage == crate::ShaderStage::Mesh { | ||
| return Err(EntryPointError::UnexpectedMeshShaderEntryResult.with_span()); | ||
| match ep.stage { | ||
| crate::ShaderStage::Vertex => { |
There was a problem hiding this comment.
nit: Since we're rewriting the lines anyways, maybe change to nt?
| naga::valid::ValidationError::EntryPoint { | ||
| stage: naga::ShaderStage::Compute, | ||
| source: naga::valid::EntryPointError::Argument( | ||
| 0, | ||
| naga::valid::VaryingError::InvalidAttributeInStage( | ||
| "location", | ||
| naga::ShaderStage::Compute | ||
| ), | ||
| ), | ||
| .. | ||
| }, |
There was a problem hiding this comment.
I thought we had a bug for this, but couldn't immediately find it in 30 seconds of searching.
There have been some times that I've added redundant errors in the front end to give diagnostics, but I think that's mostly when it was directly adjacent to things I had to do in the front end anyways.
Even if getting good diagnostics from validation is hard, it seems better to invest time in that than in duplicating things in the front-end.
For entrypoint errors specifically, there's hopefully not too much uncertainty about where in the code the problem occurs.
| #### DX12 | ||
|
|
||
| - Make sure padding bytes are 0 in the destination buffer after a `copy_texture_to_buffer` when `UnrestrictedBufferTextureCopyPitchSupported` is not available. By @teoxoy in [#10005](https://github.com/gfx-rs/wgpu/pull/10005). | ||
| - Reject result types in compute shaders. By @ErichDonGubler in [#10026](https://github.com/gfx-rs/wgpu/pull/10026). |
There was a problem hiding this comment.
| - Reject result types in compute shaders. By @ErichDonGubler in [#10026](https://github.com/gfx-rs/wgpu/pull/10026). | |
| - Reject return types on compute shader entrypoints. By @ErichDonGubler in [#10026](https://github.com/gfx-rs/wgpu/pull/10026). |
I gave UnexpectedComputeShaderEntryResult a pass since EntryResult was already used by other variants, but I think matching the spec language "return type" is clearer.
| ctx.validate(ep, fa.ty, fa.binding.as_ref()) | ||
| .map_err_inner(|e| EntryPointError::Argument(index as u32, e).with_span())?; | ||
| match ep.stage { | ||
| nt::ShaderStage::Compute => { |
There was a problem hiding this comment.
This should also apply to mesh & task shaders, plus maybe some ray tracing stages.
3 participants
Connections
Testing
Test coverage has been added to the
wgsl_errorstest group.Squash or Rebase?
Rebase, please.
Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.