Deferred error reporting for debug commands (gfx-rs#7789)

Author: andyleiserson

deno_webgpu/error.rs

@@ -19,6 +19,7 @@ use wgpu_core::command::CommandEncoderError;
 use wgpu_core::command::ComputePassError;
 use wgpu_core::command::CopyError;
 use wgpu_core::command::CreateRenderBundleError;
+use wgpu_core::command::EncoderStateError;
 use wgpu_core::command::QueryError;
 use wgpu_core::command::RenderBundleError;
 use wgpu_core::command::RenderPassError;
@@ -198,6 +199,12 @@ fn fmt_err(err: &(dyn std::error::Error + 'static)) -> String {
     output
 }
 
+impl From<EncoderStateError> for GPUError {
+    fn from(err: EncoderStateError) -> Self {
+        GPUError::Validation(fmt_err(&err))
+    }
+}
+
 impl From<CreateBufferError> for GPUError {
     fn from(err: CreateBufferError) -> Self {
         match err {

tests/tests/wgpu-gpu/encoder.rs

@@ -80,11 +80,15 @@ static DROP_ENCODER_AFTER_ERROR: GpuTestConfiguration = GpuTestConfiguration::ne
 
 #[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,
-    ))
+    .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
+    )
     .run_sync(encoder_operations_fail_while_pass_alive);
 
 fn encoder_operations_fail_while_pass_alive(ctx: TestingContext) {

wgpu-core/src/command/mod.rs

@@ -114,6 +114,53 @@ impl CommandEncoderStatus {
         }
     }
 
+    /// Record commands using the supplied closure.
+    ///
+    /// If the encoder is in the [`Self::Recording`] state, calls the closure to
+    /// record commands. If the closure returns an error, stores that error in
+    /// the encoder for later reporting when `finish()` is called. Returns
+    /// `Ok(())` even if the closure returned an error.
+    ///
+    /// If the encoder is not in the [`Self::Recording`] state, the closure will
+    /// not be called and nothing will be recorded. The encoder will be
+    /// invalidated (if it is not already). If the error is a [validation error
+    /// that should be raised immediately][ves], returns it in `Err`, otherwise,
+    /// returns `Ok(())`.
+    ///
+    /// [ves]: https://www.w3.org/TR/webgpu/#abstract-opdef-validate-the-encoder-state
+    fn record_with<
+        F: FnOnce(&mut CommandBufferMutable) -> Result<(), E>,
+        E: Clone + Into<CommandEncoderError>,
+    >(
+        &mut self,
+        f: F,
+    ) -> Result<(), EncoderStateError> {
+        let err = match self.record() {
+            Ok(guard) => {
+                guard.record(f);
+                return Ok(());
+            }
+            Err(err) => err,
+        };
+        match err {
+            err @ EncoderStateError::Locked => {
+                // Invalidate the encoder and do not record anything, but do not
+                // return an immediate validation error.
+                self.invalidate(err);
+                Ok(())
+            }
+            err @ EncoderStateError::Ended => {
+                // Invalidate the encoder, do not record anything, and return an
+                // immediate validation error.
+                Err(self.invalidate(err))
+            }
+            // Encoder is already invalid. Do not record anything, but do not
+            // return an immediate validation error.
+            EncoderStateError::Invalid => Ok(()),
+            EncoderStateError::Unlocked | EncoderStateError::Submitted => unreachable!(),
+        }
+    }
+
     #[cfg(feature = "trace")]
     fn get_inner(&mut self) -> &mut CommandBufferMutable {
         match self {
@@ -230,6 +277,21 @@ impl<'a> RecordingGuard<'a> {
     pub(crate) fn mark_successful(self) {
         mem::forget(self)
     }
+
+    fn record<
+        F: FnOnce(&mut CommandBufferMutable) -> Result<(), E>,
+        E: Clone + Into<CommandEncoderError>,
+    >(
+        mut self,
+        f: F,
+    ) {
+        match f(&mut self) {
+            Ok(()) => self.mark_successful(),
+            Err(err) => {
+                self.inner.invalidate(err);
+            }
+        }
+    }
 }
 
 impl<'a> Drop for RecordingGuard<'a> {
@@ -859,104 +921,98 @@ impl Global {
         &self,
         encoder_id: id::CommandEncoderId,
         label: &str,
-    ) -> Result<(), CommandEncoderError> {
+    ) -> Result<(), EncoderStateError> {
         profiling::scope!("CommandEncoder::push_debug_group");
         api_log!("CommandEncoder::push_debug_group {label}");
 
         let hub = &self.hub;
 
         let cmd_buf = hub.command_buffers.get(encoder_id.into_command_buffer_id());
         let mut cmd_buf_data = cmd_buf.data.lock();
-        let mut cmd_buf_data_guard = cmd_buf_data.record()?;
-        let cmd_buf_data = &mut *cmd_buf_data_guard;
-
-        #[cfg(feature = "trace")]
-        if let Some(ref mut list) = cmd_buf_data.commands {
-            list.push(TraceCommand::PushDebugGroup(label.to_owned()));
-        }
+        cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), CommandEncoderError> {
+            #[cfg(feature = "trace")]
+            if let Some(ref mut list) = cmd_buf_data.commands {
+                list.push(TraceCommand::PushDebugGroup(label.to_owned()));
+            }
 
-        let cmd_buf_raw = cmd_buf_data.encoder.open()?;
-        if !cmd_buf
-            .device
-            .instance_flags
-            .contains(wgt::InstanceFlags::DISCARD_HAL_LABELS)
-        {
-            unsafe {
-                cmd_buf_raw.begin_debug_marker(label);
+            let cmd_buf_raw = cmd_buf_data.encoder.open()?;
+            if !cmd_buf
+                .device
+                .instance_flags
+                .contains(wgt::InstanceFlags::DISCARD_HAL_LABELS)
+            {
+                unsafe {
+                    cmd_buf_raw.begin_debug_marker(label);
+                }
             }
-        }
 
-        cmd_buf_data_guard.mark_successful();
-        Ok(())
+            Ok(())
+        })
     }
 
     pub fn command_encoder_insert_debug_marker(
         &self,
         encoder_id: id::CommandEncoderId,
         label: &str,
-    ) -> Result<(), CommandEncoderError> {
+    ) -> Result<(), EncoderStateError> {
         profiling::scope!("CommandEncoder::insert_debug_marker");
         api_log!("CommandEncoder::insert_debug_marker {label}");
 
         let hub = &self.hub;
 
         let cmd_buf = hub.command_buffers.get(encoder_id.into_command_buffer_id());
         let mut cmd_buf_data = cmd_buf.data.lock();
-        let mut cmd_buf_data_guard = cmd_buf_data.record()?;
-        let cmd_buf_data = &mut *cmd_buf_data_guard;
-
-        #[cfg(feature = "trace")]
-        if let Some(ref mut list) = cmd_buf_data.commands {
-            list.push(TraceCommand::InsertDebugMarker(label.to_owned()));
-        }
+        cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), CommandEncoderError> {
+            #[cfg(feature = "trace")]
+            if let Some(ref mut list) = cmd_buf_data.commands {
+                list.push(TraceCommand::InsertDebugMarker(label.to_owned()));
+            }
 
-        if !cmd_buf
-            .device
-            .instance_flags
-            .contains(wgt::InstanceFlags::DISCARD_HAL_LABELS)
-        {
-            let cmd_buf_raw = cmd_buf_data.encoder.open()?;
-            unsafe {
-                cmd_buf_raw.insert_debug_marker(label);
+            if !cmd_buf
+                .device
+                .instance_flags
+                .contains(wgt::InstanceFlags::DISCARD_HAL_LABELS)
+            {
+                let cmd_buf_raw = cmd_buf_data.encoder.open()?;
+                unsafe {
+                    cmd_buf_raw.insert_debug_marker(label);
+                }
             }
-        }
 
-        cmd_buf_data_guard.mark_successful();
-        Ok(())
+            Ok(())
+        })
     }
 
     pub fn command_encoder_pop_debug_group(
         &self,
         encoder_id: id::CommandEncoderId,
-    ) -> Result<(), CommandEncoderError> {
+    ) -> Result<(), EncoderStateError> {
         profiling::scope!("CommandEncoder::pop_debug_marker");
         api_log!("CommandEncoder::pop_debug_group");
 
         let hub = &self.hub;
 
         let cmd_buf = hub.command_buffers.get(encoder_id.into_command_buffer_id());
         let mut cmd_buf_data = cmd_buf.data.lock();
-        let mut cmd_buf_data_guard = cmd_buf_data.record()?;
-        let cmd_buf_data = &mut *cmd_buf_data_guard;
-
-        #[cfg(feature = "trace")]
-        if let Some(ref mut list) = cmd_buf_data.commands {
-            list.push(TraceCommand::PopDebugGroup);
-        }
+        cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), CommandEncoderError> {
+            #[cfg(feature = "trace")]
+            if let Some(ref mut list) = cmd_buf_data.commands {
+                list.push(TraceCommand::PopDebugGroup);
+            }
 
-        let cmd_buf_raw = cmd_buf_data.encoder.open()?;
-        if !cmd_buf
-            .device
-            .instance_flags
-            .contains(wgt::InstanceFlags::DISCARD_HAL_LABELS)
-        {
-            unsafe {
-                cmd_buf_raw.end_debug_marker();
+            let cmd_buf_raw = cmd_buf_data.encoder.open()?;
+            if !cmd_buf
+                .device
+                .instance_flags
+                .contains(wgt::InstanceFlags::DISCARD_HAL_LABELS)
+            {
+                unsafe {
+                    cmd_buf_raw.end_debug_marker();
+                }
             }
-        }
 
-        cmd_buf_data_guard.mark_successful();
-        Ok(())
+            Ok(())
+        })
     }
 
     fn validate_pass_timestamp_writes(