Report errors when CommandEncoder.finish is called

CHANGELOG.md

@@ -126,6 +126,12 @@ Bottom level categories:
 #### WebGPU
 
 - The type of the `size` parameter to `copy_buffer_to_buffer` has changed from `BufferAddress` to `impl Into<Option<BufferAddress>>`. 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
 

cts_runner/test.lst

@@ -1,14 +1,46 @@
 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:buffer_state:bufferState="valid"
+//FAIL: webgpu:api,validation,encoding,cmds,clearBuffer:buffer_state:bufferState="invalid"
+// https://github.com/gfx-rs/wgpu/issues/7796 (Mac only)
+webgpu:api,validation,encoding,cmds,clearBuffer:buffer_state:bufferState="destroyed"
+webgpu:api,validation,encoding,cmds,clearBuffer:buffer,device_mismatch:*
+webgpu:api,validation,encoding,cmds,clearBuffer:default_args:*
+webgpu:api,validation,encoding,cmds,clearBuffer:buffer_usage:*
+webgpu:api,validation,encoding,cmds,clearBuffer:size_alignment:*
+webgpu:api,validation,encoding,cmds,clearBuffer:offset_alignment:*
+webgpu:api,validation,encoding,cmds,clearBuffer:overflow:*
+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:*
+webgpu:api,validation,encoding,cmds,copyTextureToTexture:copy_aspects:*
+//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:*

deno_webgpu/error.rs

@@ -19,6 +19,7 @@ use wgpu_core::command::CommandEncoderError;
 use wgpu_core::command::ComputePassError;
 use wgpu_core::command::CreateRenderBundleError;
 use wgpu_core::command::EncoderStateError;
+use wgpu_core::command::PassStateError;
 use wgpu_core::command::QueryError;
 use wgpu_core::command::RenderBundleError;
 use wgpu_core::command::RenderPassError;
@@ -204,6 +205,12 @@ impl From<EncoderStateError> for GPUError {
     }
 }
 
+impl From<PassStateError> for GPUError {
+    fn from(err: PassStateError) -> Self {
+        GPUError::Validation(fmt_err(&err))
+    }
+}
+
 impl From<CreateBufferError> for GPUError {
     fn from(err: CreateBufferError) -> Self {
         match err {

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]

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"),
         );
 

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"));
         }
     }
 }

wgpu-core/src/command/bundle.rs

@@ -85,6 +85,7 @@ use alloc::{
     vec::Vec,
 };
 use core::{
+    convert::Infallible,
     num::{NonZeroU32, NonZeroU64},
     ops::Range,
 };
@@ -153,7 +154,7 @@ pub struct RenderBundleEncoderDescriptor<'a> {
 #[derive(Debug)]
 #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
 pub struct RenderBundleEncoder {
-    base: BasePass<RenderCommand>,
+    base: BasePass<RenderCommand, Infallible>,
     parent_id: id::DeviceId,
     pub(crate) context: RenderPassContext,
     pub(crate) is_depth_read_only: bool,
@@ -170,7 +171,7 @@ impl RenderBundleEncoder {
     pub fn new(
         desc: &RenderBundleEncoderDescriptor,
         parent_id: id::DeviceId,
-        base: Option<BasePass<RenderCommand>>,
+        base: Option<BasePass<RenderCommand, Infallible>>,
     ) -> Result<Self, CreateRenderBundleError> {
         let (is_depth_read_only, is_stencil_read_only) = match desc.depth_stencil {
             Some(ds) => {
@@ -248,7 +249,7 @@ impl RenderBundleEncoder {
     }
 
     #[cfg(feature = "trace")]
-    pub(crate) fn to_base_pass(&self) -> BasePass<RenderCommand> {
+    pub(crate) fn to_base_pass(&self) -> BasePass<RenderCommand, Infallible> {
         self.base.clone()
     }
 
@@ -466,6 +467,7 @@ impl RenderBundleEncoder {
         let render_bundle = RenderBundle {
             base: BasePass {
                 label: desc.label.as_deref().map(str::to_owned),
+                error: None,
                 commands,
                 dynamic_offsets: flat_dynamic_offsets,
                 string_data: self.base.string_data,
@@ -863,7 +865,7 @@ pub type RenderBundleDescriptor<'a> = wgt::RenderBundleDescriptor<Label<'a>>;
 pub struct RenderBundle {
     // Normalized command stream. It can be executed verbatim,
     // without re-binding anything on the pipeline change.
-    base: BasePass<ArcRenderCommand>,
+    base: BasePass<ArcRenderCommand, Infallible>,
     pub(super) is_depth_read_only: bool,
     pub(super) is_stencil_read_only: bool,
     pub(crate) device: Arc<Device>,