diff --git a/CHANGELOG.md b/CHANGELOG.md index a9ccfb37f5c..a2d46149bc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -72,6 +72,7 @@ Bottom level categories: - `TextureFormat::is_srgb()` has been renamed to `TextureFormat::has_srgb_suffix()` to clarify its function. By @kpreid in [#9758](https://github.com/gfx-rs/wgpu/pull/9758). - Remove the never-constructed `CreateBlasError::InvalidAabbStride` variant. `create_blas` takes no stride, so it could never be produced; AABB stride is validated at build time as `BuildAccelerationStructureError::InvalidAabbStride`. By @mstampfli in [#9935](https://github.com/gfx-rs/wgpu/pull/9935). - Use `Arc`'d resources instead of IDs as the default resource type in `PipelineLayoutDescriptor`. By @sagudev in [#9985](https://github.com/gfx-rs/wgpu/pull/9985) +- `wgpu::Error::Validation::description` from `Device::create_shader_module` no longer include the shader source text and detailed compiler messages, per the WebGPU specification. These details remain accessible via `ShaderModule::get_compilation_info` or `Debug` formatting. By @beicause in [#10033](https://github.com/gfx-rs/wgpu/pull/10033). #### naga diff --git a/cts_runner/tests/integration.rs b/cts_runner/tests/integration.rs index bf094528e6a..bb3a012b405 100644 --- a/cts_runner/tests/integration.rs +++ b/cts_runner/tests/integration.rs @@ -58,27 +58,43 @@ fn exec_js_file(script_file: impl AsRef) -> Result<(), Error> { Ok(()) } -fn check_js_stderr(script: &str, expected: &str) -> Result<(), Error> { +fn check_js_stderr(script: &str, expected_stderr: &str) -> Result<(), Error> { + check_js_stdout(script, "", expected_stderr) +} + +fn check_js_stdout( + script: &str, + expected_stdout: &str, + expected_stderr: &str, +) -> Result<(), Error> { let mut tempfile = NamedTempFile::new().unwrap(); tempfile.write_all(script.as_bytes()).unwrap(); tempfile.flush().unwrap(); let output = exec_cts_runner(tempfile.path()); - if !output.stdout.is_empty() { + + let stdout_str = str::from_utf8(&output.stdout).unwrap(); + if expected_stdout.is_empty() && !output.stdout.is_empty() { + return Err(Error(format!( + "unexpected output on stdout: {:?}", + stdout_str, + ))); + } else if stdout_str != expected_stdout { return Err(Error(format!( - "unexpected output on stdout: {}", - str::from_utf8(&output.stdout).unwrap(), + "expected the following output on stdout:\n{:?}\n\nbut observed:\n{:?}", + expected_stdout, stdout_str, ))); } + let stderr_str = str::from_utf8(&output.stderr).unwrap(); - if expected.is_empty() && !stderr_str.is_empty() { + if expected_stderr.is_empty() && !stderr_str.is_empty() { return Err(Error(format!( - "unexpected output on stderr: {}", + "unexpected output on stderr: {:?}", stderr_str, ))); - } else if stderr_str != expected { + } else if stderr_str != expected_stderr { return Err(Error(format!( - "expected the following output on stderr:\n{}\n\nbut observed:\n{}", - expected, stderr_str, + "expected the following output on stderr:\n{:?}\n\nbut observed:\n{:?}", + expected_stderr, stderr_str, ))); } if !output.status.success() { @@ -143,13 +159,48 @@ fn uncaptured_error() -> Result<(), Error> { const device = await adapter.requestDevice(); device.createShaderModule({ code }) "#, - "cts_runner caught WebGPU error:\x20 -Shader '' parsing error: the type of `val` is expected to be `u32`, but got `{AbstractFloat}` - ┌─ wgsl:1:7 - │ -1 │ const val: u32 = 1.1; - │ ^^^ definition of `val`\n\n\n", + "cts_runner caught WebGPU error: Shader '' parsing error\n", + ) +} + +#[test] +fn shader_compilation_message() { + check_js_stdout( + r#" + const code = `const val: u32 = 1.1;`; + + const adapter = await navigator.gpu.requestAdapter(); + const device = await adapter.requestDevice(); + const shaderModule = device.createShaderModule({ code }); + console.log(await shaderModule.getCompilationInfo()); + + // Keep the event loop alive so the `uncapturederror` event is + // dispatched before the script exits. + await new Promise((r) => setTimeout(r, 100)); + "#, + concat!( + "GPUCompilationInfo {\n", + " messages: [\n", + " GPUCompilationMessage {\n", + " message: \x1b[32m\"\\n\"\x1b[39m +\n", + " \x1b[32m\"Shader '' parsing error: the type of `val` is expected to be `u32`, but got `{AbstractFloat}`\\n\"\x1b[39m +\n", + " \x1b[32m\" ┌─ wgsl:1:7\\n\"\x1b[39m +\n", + " \x1b[32m\" │\\n\"\x1b[39m +\n", + " \x1b[32m\"1 │ const val: u32 = 1.1;\\n\"\x1b[39m +\n", + " \x1b[32m\" │ ^^^ definition of `val`\\n\"\x1b[39m +\n", + " \x1b[32m\"\\n\"\x1b[39m,\n", + " type: \x1b[32m\"error\"\x1b[39m,\n", + " lineNum: \x1b[33m1\x1b[39m,\n", + " linePos: \x1b[33m7\x1b[39m,\n", + " offset: \x1b[33m6\x1b[39m,\n", + " length: \x1b[33m3\x1b[39m\n", + " }\n", + " ]\n", + "}\n", + ), + "cts_runner caught WebGPU error: Shader '' parsing error\n", ) + .unwrap(); } #[test] diff --git a/deno_webgpu/01_webgpu.js b/deno_webgpu/01_webgpu.js index ba97ecb9536..45cf0b49bc8 100644 --- a/deno_webgpu/01_webgpu.js +++ b/deno_webgpu/01_webgpu.js @@ -598,7 +598,8 @@ ObjectDefineProperty(GPUShaderModulePrototype, privateCustomInspect, { }, }); -ObjectDefineProperty(GPUCompilationInfo, privateCustomInspect, { +const GPUCompilationInfoPrototype = GPUCompilationInfo.prototype; +ObjectDefineProperty(GPUCompilationInfoPrototype, privateCustomInspect, { __proto__: null, value(inspect, inspectOptions) { return inspect( @@ -616,9 +617,9 @@ ObjectDefineProperty(GPUCompilationInfo, privateCustomInspect, { ); }, }); -const GPUCompilationInfoPrototype = GPUCompilationInfo.prototype; -ObjectDefineProperty(GPUCompilationMessage, privateCustomInspect, { +const GPUCompilationMessagePrototype = GPUCompilationMessage.prototype; +ObjectDefineProperty(GPUCompilationMessagePrototype, privateCustomInspect, { __proto__: null, value(inspect, inspectOptions) { return inspect( @@ -631,8 +632,8 @@ ObjectDefineProperty(GPUCompilationMessage, privateCustomInspect, { keys: [ "message", "type", - "line_num", - "line_pos", + "lineNum", + "linePos", "offset", "length", ], @@ -641,7 +642,6 @@ ObjectDefineProperty(GPUCompilationMessage, privateCustomInspect, { ); }, }); -const GPUCompilationMessagePrototype = GPUCompilationMessage.prototype; class GPUShaderStage { constructor() { diff --git a/deno_webgpu/error.rs b/deno_webgpu/error.rs index 9397fba7a42..c078c95c923 100644 --- a/deno_webgpu/error.rs +++ b/deno_webgpu/error.rs @@ -209,6 +209,17 @@ pub(crate) fn fmt_err(err: &(dyn std::error::Error + 'static)) -> String { let mut e = err.source(); while let Some(source) = e { + // Don't print detailed compiler messages when formatting error's description. + // + // per the WebGPU specification , + // the message of the validation error raised by `createShaderModule` should not include those details, + // since they are accessible via `getCompilationInfo()`. + match err.downcast_ref::() { + Some(CreateShaderModuleError::Parsing(_)) => break, + Some(CreateShaderModuleError::Validation(_)) => break, + _ => {} + }; + output.push_str(&format!(": {source}")); e = source.source(); } diff --git a/deno_webgpu/shader.rs b/deno_webgpu/shader.rs index d05a7b2dc02..ff77d9b6512 100644 --- a/deno_webgpu/shader.rs +++ b/deno_webgpu/shader.rs @@ -124,14 +124,14 @@ impl GPUCompilationMessage { impl GPUCompilationMessage { fn new(error: &pipeline::CreateShaderModuleError, source: &str) -> Self { - let message = error.to_string(); - - let loc = match error { - pipeline::CreateShaderModuleError::Parsing(e) => e.inner.location(source), + let (loc, message) = match error { + pipeline::CreateShaderModuleError::Parsing(e) => { + (e.inner.location(source), e.to_string()) + } pipeline::CreateShaderModuleError::Validation(e) => { - e.inner.location(source) + (e.inner.location(source), e.to_string()) } - _ => None, + _ => (None, error.to_string()), }; match loc { diff --git a/tests/tests/wgpu-gpu/dual_source_blending.rs b/tests/tests/wgpu-gpu/dual_source_blending.rs index 58ea75d5e15..621102ecc34 100644 --- a/tests/tests/wgpu-gpu/dual_source_blending.rs +++ b/tests/tests/wgpu-gpu/dual_source_blending.rs @@ -107,12 +107,29 @@ async fn dual_source_blending_disabled(ctx: TestingContext) { fail( &ctx.device, || { - let _ = ctx.device.create_shader_module(ShaderModuleDescriptor { + let module = ctx.device.create_shader_module(ShaderModuleDescriptor { label: Some("shader"), source: ShaderSource::Wgsl(FRAGMENT_SHADER_WITH_DUAL_SOURCE_BLENDING.into()), }); + let info = pollster::block_on(module.get_compilation_info()); + assert_eq!( + info.messages[0].message_type, + wgpu::CompilationMessageType::Error + ); + assert_eq!( + info.messages[0].location, + Some(SourceLocation { + line_number: 2, + line_position: 8, + offset: 8, + length: 20 + }) + ); + assert!(info.messages[0].message.contains( + "the `dual_source_blending` extension is not supported in the current environment" + )) }, - Some("the `dual_source_blending` extension is not supported in the current environment"), + Some("Shader 'shader' parsing error"), ); } diff --git a/tests/tests/wgpu-gpu/shader/compilation_messages/mod.rs b/tests/tests/wgpu-gpu/shader/compilation_messages/mod.rs index 5970a785766..df19f2bff91 100644 --- a/tests/tests/wgpu-gpu/shader/compilation_messages/mod.rs +++ b/tests/tests/wgpu-gpu/shader/compilation_messages/mod.rs @@ -35,7 +35,29 @@ static SHADER_COMPILE_ERROR: GpuTestConfiguration = GpuTestConfiguration::new() let sm = ctx .device .create_shader_module(include_wgsl!("error_shader.wgsl")); - assert!(pollster::block_on(scope.pop()).is_some()); + + let Some(wgpu::Error::Validation { + source, + description, + }) = scope.pop().await + else { + panic!("Expected validation error not found") + }; + // Description of validation error should not include detailed shader compilation message. + assert_eq!( + description, + "Validation Error\n\nCaused by:\n \ + In Device::create_shader_module, label = 'error_shader.wgsl'\n \ + Shader 'error_shader.wgsl' parsing error\n" + ); + let msg = source + .source() + .unwrap() + .source() + .unwrap() + .downcast_ref::>() + .unwrap() + .to_string(); let compilation_info = sm.get_compilation_info().await; let error_message = compilation_info @@ -43,6 +65,7 @@ static SHADER_COMPILE_ERROR: GpuTestConfiguration = GpuTestConfiguration::new() .iter() .find(|message| message.message_type == wgpu::CompilationMessageType::Error) .expect("Expected error message not found"); + assert_eq!(error_message.message, msg); let span = error_message.location.expect("Expected span not found"); assert_eq!( span.offset, 32, @@ -99,14 +122,32 @@ static ENABLE_EXTENSION_UNAVAILABLE: GpuTestConfiguration = GpuTestConfiguration fail( &ctx.device, || { - ctx.device + let module = ctx + .device .create_shader_module(wgpu::ShaderModuleDescriptor { label: Some("shader declaring enable extension"), source: wgpu::ShaderSource::Wgsl(std::borrow::Cow::Borrowed( ENABLE_EXTENSION_SHADER_SOURCE, )), + }); + let info = pollster::block_on(module.get_compilation_info()); + assert_eq!( + info.messages[0].message_type, + wgpu::CompilationMessageType::Error + ); + assert_eq!( + info.messages[0].location, + Some(wgpu::SourceLocation { + line_number: 2, + line_position: 12, + offset: 12, + length: 3 }) + ); + assert!(info.messages[0] + .message + .contains("the `f16` extension is not supported in the current environment")) }, - Some("the `f16` extension is not supported in the current environment"), + Some("Shader 'shader declaring enable extension' parsing error"), ); }); diff --git a/wgpu-core/src/pipeline.rs b/wgpu-core/src/pipeline.rs index 3de4c6032a4..b9f5b637ab6 100644 --- a/wgpu-core/src/pipeline.rs +++ b/wgpu-core/src/pipeline.rs @@ -182,19 +182,19 @@ impl ShaderModule { #[non_exhaustive] pub enum CreateShaderModuleError { #[cfg(feature = "wgsl")] - #[error(transparent)] + #[error("Shader '{label}' parsing error", label = _0.label.as_deref().unwrap_or_default())] Parsing(#[from] ShaderError), #[cfg(feature = "glsl")] - #[error(transparent)] + #[error("Shader '{label}' parsing error", label = _0.label.as_deref().unwrap_or_default())] ParsingGlsl(#[from] ShaderError), #[cfg(feature = "spirv")] - #[error(transparent)] + #[error("Shader '{label}' parsing error", label = _0.label.as_deref().unwrap_or_default())] ParsingSpirV(#[from] ShaderError), #[error("Failed to generate the backend-specific code")] Generation, #[error(transparent)] Device(#[from] DeviceError), - #[error(transparent)] + #[error("Shader '{label}' validation error", label = _0.label.as_deref().unwrap_or_default())] Validation(#[from] ShaderError>), #[error(transparent)] MissingFeatures(#[from] MissingFeatures), @@ -1165,3 +1165,45 @@ impl RenderPipeline { (bgl, error) } } + +#[cfg(feature = "wgsl")] +#[cfg(test)] +mod tests { + use alloc::{boxed::Box, string::ToString}; + use naga::error::ShaderError; + + use super::CreateShaderModuleError; + + // Test that validation error `Display` doesn't include + // the shader source text and detailed compiler messages, + // per the WebGPU specification , + #[test] + fn create_shader_module_error_message() { + let source: &str = "not valid wgsl"; + let error = CreateShaderModuleError::Parsing(ShaderError { + source: source.to_string(), + label: Some("my shader".to_string()), + inner: Box::new( + naga::front::wgsl::Frontend::new() + .parse(source) + .unwrap_err(), + ), + }); + + assert_eq!(error.to_string(), "Shader 'my shader' parsing error"); + + let source: &str = "fn main() -> f32 { let arr = array(1.0); return arr[-1]; }"; + let module = naga::front::wgsl::parse_str(source).unwrap(); + let mut validator = naga::valid::Validator::new( + naga::valid::ValidationFlags::all(), + naga::valid::Capabilities::empty(), + ); + let error = CreateShaderModuleError::Validation(ShaderError { + source: source.to_string(), + label: Some("my shader".to_string()), + inner: validator.validate(&module).unwrap_err(), + }); + + assert_eq!(error.to_string(), "Shader 'my shader' validation error"); + } +} diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 36d93d1135c..6e2a0c9d67e 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -266,16 +266,20 @@ impl ContextWgpuCore { self.0.generate_report() } + /// Handle the error and recursively format the error source into [`crate::Error::Validation::description`]. + /// + /// If `format_filter` returns `false`, the **next** error source will skip formatting. #[cold] #[track_caller] #[inline(never)] - fn handle_error_inner( + fn handle_error_inner_with_format_filter( &self, sink_mutex: &Mutex, error_type: ErrorType, source: ContextErrorSource, label: Label<'_>, fn_ident: &'static str, + format_filter: impl Fn(&(dyn Error + 'static)) -> bool, ) { let source: ErrorSource = Box::new(wgc::error::ContextError { fn_ident, @@ -284,7 +288,7 @@ impl ContextWgpuCore { }); let final_error_handling = { let mut sink = sink_mutex.lock(); - let description = || self.format_error(&*source); + let description = || self.format_error_with_filter(&*source, format_filter); let error = match error_type { ErrorType::Internal => { let description = description(); @@ -314,6 +318,27 @@ impl ContextWgpuCore { } } + #[cold] + #[track_caller] + #[inline(never)] + fn handle_error_inner( + &self, + sink_mutex: &Mutex, + error_type: ErrorType, + source: ContextErrorSource, + label: Label<'_>, + fn_ident: &'static str, + ) { + self.handle_error_inner_with_format_filter( + sink_mutex, + error_type, + source, + label, + fn_ident, + |_| true, + ); + } + #[inline] #[track_caller] fn handle_error( @@ -351,17 +376,34 @@ impl ContextWgpuCore { #[inline(never)] fn format_error(&self, err: &(dyn Error + 'static)) -> String { + self.format_error_with_filter(err, |_| true) + } + + #[inline(never)] + fn format_error_with_filter( + &self, + err: &(dyn Error + 'static), + filter: impl Fn(&(dyn Error + 'static)) -> bool, + ) -> String { let mut output = String::new(); let mut level = 1; - fn print_tree(output: &mut String, level: &mut usize, e: &(dyn Error + 'static)) { + fn print_tree( + output: &mut String, + level: &mut usize, + e: &(dyn Error + 'static), + filter: &impl Fn(&(dyn Error + 'static)) -> bool, + ) { let mut print = |e: &(dyn Error + 'static)| { use core::fmt::Write; writeln!(output, "{}{}", " ".repeat(*level * 2), e).unwrap(); + if !filter(e) { + return; + } if let Some(e) = e.source() { *level += 1; - print_tree(output, level, e); + print_tree(output, level, e, filter); *level -= 1; } }; @@ -374,7 +416,7 @@ impl ContextWgpuCore { } } - print_tree(&mut output, &mut level, err); + print_tree(&mut output, &mut level, err, &filter); format!("Validation Error\n\nCaused by:\n{output}") } @@ -1085,11 +1127,27 @@ impl dispatch::DeviceInterface for CoreDevice { .device_create_shader_module(self.id, &descriptor, source, None); let compilation_info = match error { Some(cause) => { - self.context.handle_error( + // Don't print detailed compiler messages when formatting error's description. + // + // per the WebGPU specification , + // the message of the validation error raised by `createShaderModule` should not include those details, + // since they are accessible via `getCompilationInfo()`. + self.context.handle_error_inner_with_format_filter( &self.error_sink, - cause.clone(), + cause.webgpu_error_type(), + Box::new(cause.clone()), desc.label, "Device::create_shader_module", + |e| match e.downcast_ref::() { + #[cfg(feature = "wgsl")] + Some(CreateShaderModuleError::Parsing(_)) => false, + #[cfg(feature = "glsl")] + Some(CreateShaderModuleError::ParsingGlsl(_)) => false, + #[cfg(feature = "spirv")] + Some(CreateShaderModuleError::ParsingSpirV(_)) => false, + Some(CreateShaderModuleError::Validation(_)) => false, + _ => true, + }, ); CompilationInfo::from(cause) }