From 41badaedfb1e2efedec6be2498a257f21a9ecfb3 Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Fri, 6 Jun 2025 16:48:10 -0700 Subject: [PATCH] Update tests and changelog for deferred error reporting --- CHANGELOG.md | 6 ++ cts_runner/test.lst | 33 ++++++++-- .../tests/wgpu-gpu/bind_group_layout_dedup.rs | 14 ++--- tests/tests/wgpu-gpu/device.rs | 40 ++++++------- tests/tests/wgpu-gpu/encoder.rs | 60 ++++++++----------- xtask/src/cts.rs | 2 +- 6 files changed, 86 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97742ff9b..4b1a1cf6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,6 +139,12 @@ Bottom level categories: #### WebGPU - The type of the `size` parameter to `copy_buffer_to_buffer` has changed from `BufferAddress` to `impl Into>`. This achieves the spec-defined behavior of the value being optional, while still accepting existing calls without changes. By @andyleiserson in [#7659](https://github.com/gfx-rs/wgpu/pull/7659). +- To bring wgpu's error reporting into compliance with the WebGPU specification, the error type returned from some functions has changed, and some errors may be raised at a different time than they were previously. + - The error type returned by many methods on `CommandEncoder`, `RenderPassEncoder`, `ComputePassEncoder`, and `RenderBundleEncoder` has changed to `EncoderStateError` or `PassStateError`. These functions will return the `Ended` variant of these errors if called on an encoder that is no longer active. Reporting of all other errors is deferred until a call to `finish()`. + - Variants holding a `CommandEncoderError` in the error enums `ClearError`, `ComputePassErrorInner`, `QueryError`, and `RenderPassErrorInner` have been replaced with variants holding an `EncoderStateError`. + - The definition of `enum CommandEncoderError` has changed significantly, to reflect which errors can be raised by `CommandEncoder.finish()`. There are also some errors that no longer appear directly in `CommandEncoderError`, and instead appear nested within the `RenderPass` or `ComputePass` variants. + - `CopyError` has been removed. Errors that were previously a `CopyError` are now a `CommandEncoderError` returned by `finish()`. (The detailed reasons for copies to fail were and still are described by `TransferError`, which was previously a variant of `CopyError`, and is now a variant of `CommandEncoderError`). + #### HAL diff --git a/cts_runner/test.lst b/cts_runner/test.lst index 2d8e2553f..5089223cf 100644 --- a/cts_runner/test.lst +++ b/cts_runner/test.lst @@ -1,14 +1,37 @@ unittests:* webgpu:api,operation,command_buffer,basic:* -webgpu:api,operation,command_buffer,copyBufferToBuffer:single:newSig=false;* -// https://github.com/gfx-rs/wgpu/issues/7391 -//FAIL: webgpu:api,operation,command_buffer,copyBufferToBuffer:single:newSig=true;* -webgpu:api,operation,command_buffer,copyBufferToBuffer:state_transitions:* -webgpu:api,operation,command_buffer,copyBufferToBuffer:copy_order:* +webgpu:api,operation,command_buffer,copyBufferToBuffer:* webgpu:api,operation,compute,basic:memcpy:* //FAIL: webgpu:api,operation,compute,basic:large_dispatch:* webgpu:api,operation,compute_pipeline,overrides:* webgpu:api,operation,device,lost:* +webgpu:api,validation,encoding,cmds,clearBuffer:out_of_bounds,* +webgpu:api,validation,encoding,cmds,compute_pass:set_pipeline:* +webgpu:api,validation,encoding,cmds,compute_pass:dispatch_sizes:* +webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_with_invalid_or_destroyed_texture:* +webgpu:api,validation,encoding,cmds,copyTextureToTexture:texture,device_mismatch:* +webgpu:api,validation,encoding,cmds,copyTextureToTexture:mipmap_level:* +webgpu:api,validation,encoding,cmds,copyTextureToTexture:texture_usage:* +webgpu:api,validation,encoding,cmds,copyTextureToTexture:sample_count:* +//FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:multisampled_copy_restrictions:* +// texture_format_compatibility fails on Windows and Mac in CI. +// It passes locally on Mac. It is also a relatively long test. +//FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:texture_format_compatibility:* +//FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:depth_stencil_copy_restrictions:* +//FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_ranges:* +//FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_within_same_texture:* +//FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_aspects:* +// Fails on Windows, https://github.com/gfx-rs/wgpu/issues/7844. +//FAIL: webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_ranges_with_compressed_texture_formats:* +webgpu:api,validation,encoding,cmds,index_access:* +//FAIL: webgpu:api,validation,encoding,cmds,render,draw:* +webgpu:api,validation,encoding,encoder_state:* +webgpu:api,validation,encoding,encoder_open_state:non_pass_commands:* +webgpu:api,validation,encoding,encoder_open_state:render_pass_commands:* +//FAIL: webgpu:api,validation,encoding,encoder_open_state:render_bundle_commands:* +webgpu:api,validation,encoding,encoder_open_state:compute_pass_commands:* +webgpu:api,validation,encoding,beginComputePass:* +webgpu:api,validation,encoding,beginRenderPass:* webgpu:api,validation,queue,submit:command_buffer,device_mismatch:* webgpu:api,validation,queue,submit:command_buffer,duplicate_buffers:* webgpu:api,validation,queue,submit:command_buffer,submit_invalidates:* diff --git a/tests/tests/wgpu-gpu/bind_group_layout_dedup.rs b/tests/tests/wgpu-gpu/bind_group_layout_dedup.rs index b322b019b..b97b9cff1 100644 --- a/tests/tests/wgpu-gpu/bind_group_layout_dedup.rs +++ b/tests/tests/wgpu-gpu/bind_group_layout_dedup.rs @@ -316,11 +316,11 @@ fn separate_pipelines_have_incompatible_derived_bgls(ctx: TestingContext) { pass.set_bind_group(0, &bg2, &[]); pass.dispatch_workgroups(1, 1, 1); + drop(pass); + fail( &ctx.device, - || { - drop(pass); - }, + || encoder.finish(), Some("label at index 0 is not compatible with the corresponding bindgrouplayout"), ); } @@ -388,13 +388,13 @@ fn derived_bgls_incompatible_with_regular_bgls(ctx: TestingContext) { pass.set_bind_group(0, &bg, &[]); pass.dispatch_workgroups(1, 1, 1); + drop(pass); + fail( &ctx.device, - || { - drop(pass); - }, + || encoder.finish(), Some("label at index 0 is not compatible with the corresponding bindgrouplayout"), - ) + ); } #[gpu_test] diff --git a/tests/tests/wgpu-gpu/device.rs b/tests/tests/wgpu-gpu/device.rs index 71edda75c..53cdb731e 100644 --- a/tests/tests/wgpu-gpu/device.rs +++ b/tests/tests/wgpu-gpu/device.rs @@ -347,34 +347,34 @@ static DEVICE_DESTROY_THEN_MORE: GpuTestConfiguration = GpuTestConfiguration::ne ); // Creating a compute pass should fail. + let pass = encoder_for_compute_pass.begin_compute_pass(&wgpu::ComputePassDescriptor { + label: None, + timestamp_writes: None, + }); + drop(pass); fail( &ctx.device, - || { - encoder_for_compute_pass.begin_compute_pass(&wgpu::ComputePassDescriptor { - label: None, - timestamp_writes: None, - }); - }, + || encoder_for_compute_pass.finish(), Some("device with '' label is invalid"), ); // Creating a render pass should fail. + let pass = encoder_for_render_pass.begin_render_pass(&wgpu::RenderPassDescriptor { + label: None, + color_attachments: &[Some(wgpu::RenderPassColorAttachment { + ops: wgpu::Operations::default(), + resolve_target: None, + view: &target_view, + depth_slice: None, + })], + depth_stencil_attachment: None, + timestamp_writes: None, + occlusion_query_set: None, + }); + drop(pass); fail( &ctx.device, - || { - encoder_for_render_pass.begin_render_pass(&wgpu::RenderPassDescriptor { - label: None, - color_attachments: &[Some(wgpu::RenderPassColorAttachment { - ops: wgpu::Operations::default(), - resolve_target: None, - view: &target_view, - depth_slice: None, - })], - depth_stencil_attachment: None, - timestamp_writes: None, - occlusion_query_set: None, - }); - }, + || encoder_for_render_pass.finish(), Some("device with '' label is invalid"), ); diff --git a/tests/tests/wgpu-gpu/encoder.rs b/tests/tests/wgpu-gpu/encoder.rs index 1ee9ca082..aa9117313 100644 --- a/tests/tests/wgpu-gpu/encoder.rs +++ b/tests/tests/wgpu-gpu/encoder.rs @@ -18,7 +18,8 @@ static DROP_QUEUE_BEFORE_CREATING_COMMAND_ENCODER: GpuTestConfiguration = .parameters(TestParameters::default().expect_fail(FailureCase::always())) .run_sync(|ctx| { // Use the device after the queue is dropped. Currently this panics - // but it probably shouldn't + // but it probably shouldn't. + // TODO(https://github.com/gfx-rs/wgpu/issues/7781) revisit this let TestingContext { device, queue, .. } = ctx; drop(queue); let _encoder = @@ -62,33 +63,20 @@ static DROP_ENCODER_AFTER_ERROR: GpuTestConfiguration = GpuTestConfiguration::ne occlusion_query_set: None, }); - // Set a bad viewport on renderpass, triggering an error. - fail( - &ctx.device, - || { - renderpass.set_viewport(0.0, 0.0, -1.0, -1.0, 0.0, 1.0); - drop(renderpass); - }, - Some("less than zero"), - ); + // This viewport is invalid because it has negative size. + renderpass.set_viewport(0.0, 0.0, -1.0, -1.0, 0.0, 1.0); + drop(renderpass); - // This is the actual interesting error condition. We've created - // a CommandEncoder which errored out when processing a command. - // The encoder is still open! - drop(encoder); + fail(&ctx.device, || encoder.finish(), Some("less than zero")); }); #[gpu_test] static ENCODER_OPERATIONS_FAIL_WHILE_PASS_ALIVE: GpuTestConfiguration = GpuTestConfiguration::new() - .parameters( - TestParameters::default() - .features( - wgpu::Features::CLEAR_TEXTURE - | wgpu::Features::TIMESTAMP_QUERY - | wgpu::Features::TIMESTAMP_QUERY_INSIDE_ENCODERS, - ) - .expect_fail(FailureCase::always()), // temporary, until https://github.com/gfx-rs/wgpu/issues/7391 is completed - ) + .parameters(TestParameters::default().features( + wgpu::Features::CLEAR_TEXTURE + | wgpu::Features::TIMESTAMP_QUERY + | wgpu::Features::TIMESTAMP_QUERY_INSIDE_ENCODERS, + )) .run_sync(encoder_operations_fail_while_pass_alive); fn encoder_operations_fail_while_pass_alive(ctx: TestingContext) { @@ -303,6 +291,7 @@ fn encoder_operations_fail_while_pass_alive(ctx: TestingContext) { for &pass_type in [PassType::Compute, PassType::Render].iter() { for (op_name, op) in recording_ops.iter() { + // Test the case where the pass is not ended before calling finish() let mut encoder = ctx .device .create_command_encoder(&wgpu::CommandEncoderDescriptor::default()); @@ -312,26 +301,25 @@ fn encoder_operations_fail_while_pass_alive(ctx: TestingContext) { ctx.device.push_error_scope(wgpu::ErrorFilter::Validation); log::info!("Testing operation {op_name:?} on a locked command encoder while a {pass_type:?} pass is active"); - fail(&ctx.device, || op(&mut encoder), Some("encoder is locked")); + op(&mut encoder); - // Drop the pass - this also fails now since the encoder is invalid: - fail(&ctx.device, || drop(pass), Some("encoder is invalid")); - // Also, it's not possible to create a new pass on the encoder: - fail( - &ctx.device, - || encoder.begin_compute_pass(&wgpu::ComputePassDescriptor::default()), - Some("encoder is invalid"), - ); - } + fail(&ctx.device, || encoder.finish(), Some("encoder is locked")); - // Test encoder finishing separately since it consumes the encoder and doesn't fit above pattern. - { + drop(pass); + + // ...and the case where the pass is ended before calling finish() let mut encoder = ctx .device .create_command_encoder(&wgpu::CommandEncoderDescriptor::default()); + let pass = create_pass(&mut encoder, pass_type); + + log::info!("Testing operation {op_name:?} on a locked command encoder while a {pass_type:?} pass is active"); + op(&mut encoder); + + drop(pass); + fail(&ctx.device, || encoder.finish(), Some("encoder is locked")); - fail(&ctx.device, || drop(pass), Some("encoder is invalid")); } } } diff --git a/xtask/src/cts.rs b/xtask/src/cts.rs index 6ae9346f5..62461dc2d 100644 --- a/xtask/src/cts.rs +++ b/xtask/src/cts.rs @@ -34,7 +34,7 @@ pub fn run_cts(shell: Shell, mut args: Arguments) -> anyhow::Result<()> { tests.extend(shell.read_file(file)?.lines().filter_map(|line| { let trimmed = line.trim(); let is_comment = trimmed.starts_with("//") || trimmed.starts_with("#"); - (!is_comment).then(|| OsString::from(trimmed)) + (!trimmed.is_empty() && !is_comment).then(|| OsString::from(trimmed)) })) }