Shader validation enum#17824
Conversation
|
Does this effectively expose this functionality to the user? |
alice-i-cecile
left a comment
There was a problem hiding this comment.
Thanks for tackling this :) Definitely the right direction! I've highlighted a few small cleanup items, some docs to write, and then we can bubble up the unsafe to the end user who's actually calling these things.
Shouldn't be too bad, and feel free to ask questions or for advice on the tricky bits.
|
not sure if I got the |
|
Thanks! |
| // | ||
| // This function passes binary data to the backend as-is and can potentially result in a | ||
| // driver crash or bogus behavior. No attempt is made to ensure that data is valid SPIR-V. | ||
| unsafe { self.create_shader_module(desc) }, |
There was a problem hiding this comment.
having an unsafe function to create unchecked shaders, and having a safe function that also creates unchecked shaders in an unsafe block if you pass in spirv feels wrong (and the comment on the function is misleading).
i think we should probably panic for spirv here.
| // SAFETY: | ||
| // | ||
| // creates a shader module with user-customizable runtime checks which allows shaders to | ||
| // perform operations which can lead to undefined behavior like indexing out of bounds, | ||
| // thus it's the caller responsibility to pass a shader which doesn't perform any of this | ||
| // operations. |
There was a problem hiding this comment.
i'd copy the short safety comment below, not much use in copying the long version from the top of the function.
alice-i-cecile
left a comment
There was a problem hiding this comment.
A couple more nits and additions to the docs, but this is nearly ready :) Thanks!
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
|
Thank you! |
| /// | ||
| /// See [`ValidateShader`](bevy_render::render_resource::ValidateShader) for more information on the tradeoffs involved with shader validation. | ||
| #[inline] | ||
| pub fn create_and_validate_shader_module( |
There was a problem hiding this comment.
Maybe I'm missing something, but this looks like it just ends up calling .create_shader_module() and that function doesn't do any validation.
There was a problem hiding this comment.
This calls create_shader_module in the wgpu device. In that implementation runtime checks are enabled
https://docs.rs/wgpu/latest/src/wgpu/api/device.rs.html#80-85
pub fn create_shader_module(&self, desc: ShaderModuleDescriptor<'_>) -> ShaderModule {
let module = self
.inner
.create_shader_module(desc, wgt::ShaderRuntimeChecks::checked());
ShaderModule { inner: module }
}
There was a problem hiding this comment.
Ah, I thought it was calling the bevy one, that makes way more sense.
| match &desc.source { | ||
| wgpu::ShaderSource::SpirV(_source) | ||
| if self | ||
| .features() | ||
| .contains(wgpu::Features::SPIRV_SHADER_PASSTHROUGH) => | ||
| panic!("no safety checks are performed for spirv shaders. use `create_shader_module` instead"), | ||
| _ => self.device.create_shader_module(desc), | ||
| } |
There was a problem hiding this comment.
what happens if we don't have SPIRV_SHADER_PASSTHROUGH here?
There was a problem hiding this comment.
If we don't have SPRIV_SHADER_PASSTHROGH the match guard will be false. The _ branch will be executed I believe
There was a problem hiding this comment.
ok. i'm not sure it has much value here; it will presumably result in failure either way, but a different failure depending on the enabled features. can we just remove it so we always panic on this path?
There was a problem hiding this comment.
I have removed the match guard
Objective
Make checked vs unchecked shaders configurable
Fixes #17786
Solution
Added
ValidateShadersenum toShaderand addedcreate_and_validate_shader_moduletoRenderDeviceTesting
I tested the shader examples locally and they all worked. I'd like to write a few tests to verify but am unsure how to start.