Check for more cases of unbalanced debug groups (#8048)

Fixes #3911

Author: andyleiserson

cts_runner/test.lst

@@ -25,6 +25,13 @@ webgpu:api,validation,encoding,cmds,clearBuffer:*
 webgpu:api,validation,encoding,cmds,compute_pass:set_pipeline:*
 webgpu:api,validation,encoding,cmds,compute_pass:dispatch_sizes:*
 webgpu:api,validation,encoding,cmds,copyTextureToTexture:*
+webgpu:api,validation,encoding,cmds,debug:debug_group_balanced:encoderType="non-pass"
+webgpu:api,validation,encoding,cmds,debug:debug_group_balanced:encoderType="compute%20pass"
+webgpu:api,validation,encoding,cmds,debug:debug_group_balanced:encoderType="render%20pass"
+//FAIL: webgpu:api,validation,encoding,cmds,debug:debug_group_balanced:encoderType="render%20bundle"
+// https://github.com/gfx-rs/wgpu/issues/8039
+webgpu:api,validation,encoding,cmds,debug:debug_group:*
+webgpu:api,validation,encoding,cmds,debug:debug_marker:*
 webgpu:api,validation,encoding,cmds,index_access:*
 //FAIL: webgpu:api,validation,encoding,cmds,render,draw:*
 webgpu:api,validation,encoding,cmds,render,draw:index_buffer_OOB:*
@@ -75,11 +82,7 @@ webgpu:api,validation,image_copy,layout_related:copy_end_overflows_u64:*
 // Fails with OOM in CI.
 fails-if(dx12) webgpu:api,validation,image_copy,layout_related:offset_alignment:*
 webgpu:api,validation,image_copy,texture_related:format:dimension="1d";*
-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:*
-//FAIL: webgpu:api,validation,queue,submit:command_buffer,invalid_submit_invalidates:*
-// https://github.com/gfx-rs/wgpu/issues/3911#issuecomment-2972995675
+webgpu:api,validation,queue,submit:command_buffer,*
 webgpu:api,validation,render_pass,render_pass_descriptor:attachments,*
 webgpu:api,validation,render_pass,render_pass_descriptor:resolveTarget,*
 webgpu:api,validation,texture,rg11b10ufloat_renderable:*
@@ -93,6 +96,7 @@ webgpu:api,operation,rendering,color_target_state:blend_constant,setting:*
 webgpu:api,operation,rendering,depth:*
 webgpu:api,operation,rendering,draw:*
 webgpu:api,operation,shader_module,compilation_info:*
+// Likely due to https://github.com/gfx-rs/wgpu/issues/7357.
 fails-if(metal) webgpu:api,operation,uncapturederror:iff_uncaptured:*
 //FAIL: webgpu:shader,execution,expression,call,builtin,select:*
 // - Fails with `const`/abstract int cases on all platforms because of <https://github.com/gfx-rs/wgpu/issues/4507>.

deno_webgpu/render_bundle.rs

@@ -20,6 +20,13 @@ use crate::buffer::GPUBuffer;
 use crate::texture::GPUTextureFormat;
 use crate::Instance;
 
+fn c_string_truncated_at_first_nul<T: Into<Vec<u8>>>(src: T) -> std::ffi::CString {
+    std::ffi::CString::new(src).unwrap_or_else(|err| {
+        let nul_pos = err.nul_position();
+        std::ffi::CString::new(err.into_vec().split_at(nul_pos).0).unwrap()
+    })
+}
+
 pub struct GPURenderBundleEncoder {
     pub instance: Instance,
     pub error_handler: super::error::ErrorHandler,
@@ -70,7 +77,7 @@ impl GPURenderBundleEncoder {
             .as_mut()
             .ok_or_else(|| JsErrorBox::generic("Encoder has already been finished"))?;
 
-        let label = std::ffi::CString::new(group_label).unwrap();
+        let label = c_string_truncated_at_first_nul(group_label);
         // SAFETY: the string the raw pointer points to lives longer than the below
         // function invocation.
         unsafe {
@@ -99,8 +106,7 @@ impl GPURenderBundleEncoder {
             .as_mut()
             .ok_or_else(|| JsErrorBox::generic("Encoder has already been finished"))?;
 
-        let label = std::ffi::CString::new(marker_label).unwrap();
-
+        let label = c_string_truncated_at_first_nul(marker_label);
         // SAFETY: the string the raw pointer points to lives longer than the below
         // function invocation.
         unsafe {

wgpu-core/src/command/compute.rs

@@ -8,7 +8,7 @@ use alloc::{borrow::Cow, boxed::Box, sync::Arc, vec::Vec};
 use core::{fmt, str};
 
 use crate::command::{
-    pass, CommandEncoder, EncoderStateError, PassStateError, TimestampWritesError,
+    pass, CommandEncoder, DebugGroupError, EncoderStateError, PassStateError, TimestampWritesError,
 };
 use crate::resource::DestroyedResourceError;
 use crate::{binding_model::BindError, resource::RawResourceAccess};
@@ -146,6 +146,8 @@ pub enum ComputePassErrorInner {
     #[error("Parent encoder is invalid")]
     InvalidParentEncoder,
     #[error(transparent)]
+    DebugGroupError(#[from] DebugGroupError),
+    #[error(transparent)]
     BindGroupIndexOutOfRange(#[from] pass::BindGroupIndexOutOfRange),
     #[error(transparent)]
     DestroyedResource(#[from] DestroyedResourceError),
@@ -162,8 +164,6 @@ pub enum ComputePassErrorInner {
     #[error(transparent)]
     MissingBufferUsage(#[from] MissingBufferUsageError),
     #[error(transparent)]
-    InvalidPopDebugGroup(#[from] pass::InvalidPopDebugGroup),
-    #[error(transparent)]
     Dispatch(#[from] DispatchError),
     #[error(transparent)]
     Bind(#[from] BindError),
@@ -226,6 +226,7 @@ impl WebGpuError for ComputePassError {
         let e: &dyn WebGpuError = match inner {
             ComputePassErrorInner::Device(e) => e,
             ComputePassErrorInner::EncoderState(e) => e,
+            ComputePassErrorInner::DebugGroupError(e) => e,
             ComputePassErrorInner::DestroyedResource(e) => e,
             ComputePassErrorInner::ResourceUsageCompatibility(e) => e,
             ComputePassErrorInner::MissingBufferUsage(e) => e,
@@ -238,7 +239,6 @@ impl WebGpuError for ComputePassError {
             ComputePassErrorInner::InvalidResource(e) => e,
             ComputePassErrorInner::TimestampWrites(e) => e,
             ComputePassErrorInner::InvalidValuesOffset(e) => e,
-            ComputePassErrorInner::InvalidPopDebugGroup(e) => e,
 
             ComputePassErrorInner::InvalidParentEncoder
             | ComputePassErrorInner::BindGroupIndexOutOfRange { .. }
@@ -734,6 +734,13 @@ impl Global {
                 }
             }
 
+            if state.general.debug_scope_depth > 0 {
+                Err(
+                    ComputePassErrorInner::DebugGroupError(DebugGroupError::MissingPop)
+                        .map_pass_err(pass_scope),
+                )?;
+            }
+
             unsafe {
                 state.general.raw_encoder.end_compute_pass();
             }

wgpu-core/src/command/mod.rs

@@ -284,6 +284,10 @@ impl CommandEncoderStatus {
             Self::Recording(mut inner) => {
                 if let Err(err) = inner.encoder.close_if_open() {
                     Self::Error(err.into())
+                } else if inner.debug_scope_depth > 0 {
+                    Self::Error(CommandEncoderError::DebugGroupError(
+                        DebugGroupError::MissingPop,
+                    ))
                 } else {
                     // Note: if we want to stop tracking the swapchain texture view,
                     // this is the place to do it.
@@ -648,6 +652,8 @@ pub struct CommandBufferMutable {
 
     indirect_draw_validation_resources: crate::indirect_validation::DrawResources,
 
+    debug_scope_depth: u32,
+
     #[cfg(feature = "trace")]
     pub(crate) commands: Option<Vec<TraceCommand>>,
 }
@@ -721,6 +727,7 @@ impl CommandEncoder {
                     temp_resources: Default::default(),
                     indirect_draw_validation_resources:
                         crate::indirect_validation::DrawResources::new(device.clone()),
+                    debug_scope_depth: 0,
                     #[cfg(feature = "trace")]
                     commands: if device.trace.lock().is_some() {
                         Some(Vec::new())
@@ -1041,6 +1048,8 @@ pub enum CommandEncoderError {
     #[error(transparent)]
     ResourceUsage(#[from] ResourceUsageCompatibilityError),
     #[error(transparent)]
+    DebugGroupError(#[from] DebugGroupError),
+    #[error(transparent)]
     MissingFeatures(#[from] MissingFeatures),
     #[error(transparent)]
     Transfer(#[from] TransferError),
@@ -1094,6 +1103,7 @@ impl WebGpuError for CommandEncoderError {
         let e: &dyn WebGpuError = match self {
             Self::Device(e) => e,
             Self::InvalidResource(e) => e,
+            Self::DebugGroupError(e) => e,
             Self::MissingFeatures(e) => e,
             Self::State(e) => e,
             Self::DestroyedResource(e) => e,
@@ -1110,6 +1120,23 @@ impl WebGpuError for CommandEncoderError {
     }
 }
 
+#[derive(Clone, Debug, Error)]
+#[non_exhaustive]
+pub enum DebugGroupError {
+    #[error("Cannot pop debug group, because number of pushed debug groups is zero")]
+    InvalidPop,
+    #[error("A debug group was not popped before the encoder was finished")]
+    MissingPop,
+}
+
+impl WebGpuError for DebugGroupError {
+    fn webgpu_error_type(&self) -> ErrorType {
+        match self {
+            Self::InvalidPop | Self::MissingPop => ErrorType::Validation,
+        }
+    }
+}
+
 #[derive(Clone, Debug, Error)]
 #[non_exhaustive]
 pub enum TimestampWritesError {
@@ -1175,6 +1202,8 @@ impl Global {
         let cmd_enc = hub.command_encoders.get(encoder_id);
         let mut cmd_buf_data = cmd_enc.data.lock();
         cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), CommandEncoderError> {
+            cmd_buf_data.debug_scope_depth += 1;
+
             #[cfg(feature = "trace")]
             if let Some(ref mut list) = cmd_buf_data.commands {
                 list.push(TraceCommand::PushDebugGroup(label.to_owned()));
@@ -1244,6 +1273,11 @@ impl Global {
         let cmd_enc = hub.command_encoders.get(encoder_id);
         let mut cmd_buf_data = cmd_enc.data.lock();
         cmd_buf_data.record_with(|cmd_buf_data| -> Result<(), CommandEncoderError> {
+            if cmd_buf_data.debug_scope_depth == 0 {
+                return Err(DebugGroupError::InvalidPop.into());
+            }
+            cmd_buf_data.debug_scope_depth -= 1;
+
             #[cfg(feature = "trace")]
             if let Some(ref mut list) = cmd_buf_data.commands {
                 list.push(TraceCommand::PopDebugGroup);

wgpu-core/src/command/pass.rs

@@ -3,7 +3,7 @@
 use crate::binding_model::{BindError, BindGroup, PushConstantUploadError};
 use crate::command::bind::Binder;
 use crate::command::memory_init::{CommandBufferTextureMemoryActions, SurfacesInDiscardState};
-use crate::command::{CommandEncoder, QueryResetMap, QueryUseError};
+use crate::command::{CommandEncoder, DebugGroupError, QueryResetMap, QueryUseError};
 use crate::device::{Device, DeviceError, MissingFeatures};
 use crate::init_tracker::BufferInitTrackerAction;
 use crate::pipeline::LateSizedBufferGroup;
@@ -42,16 +42,6 @@ impl WebGpuError for InvalidValuesOffset {
     }
 }
 
-#[derive(Clone, Debug, Error)]
-#[error("Cannot pop debug group, because number of pushed debug groups is zero")]
-pub struct InvalidPopDebugGroup;
-
-impl WebGpuError for InvalidPopDebugGroup {
-    fn webgpu_error_type(&self) -> ErrorType {
-        ErrorType::Validation
-    }
-}
-
 pub(crate) struct BaseState<'scope, 'snatch_guard, 'cmd_enc, 'raw_encoder> {
     pub(crate) device: &'cmd_enc Arc<Device>,
 
@@ -332,12 +322,12 @@ pub(crate) fn push_debug_group(state: &mut BaseState, string_data: &[u8], len: u
 
 pub(crate) fn pop_debug_group<E>(state: &mut BaseState) -> Result<(), E>
 where
-    E: From<InvalidPopDebugGroup>,
+    E: From<DebugGroupError>,
 {
     api_log!("Pass::pop_debug_group");
 
     if state.debug_scope_depth == 0 {
-        return Err(InvalidPopDebugGroup.into());
+        return Err(DebugGroupError::InvalidPop.into());
     }
     state.debug_scope_depth -= 1;
     if !state

wgpu-core/src/command/render.rs

@@ -11,8 +11,8 @@ use wgt::{
 
 use crate::command::{
     pass, pass_base, pass_try, validate_and_begin_occlusion_query,
-    validate_and_begin_pipeline_statistics_query, EncoderStateError, InnerCommandEncoder,
-    PassStateError, TimestampWritesError,
+    validate_and_begin_pipeline_statistics_query, DebugGroupError, EncoderStateError,
+    InnerCommandEncoder, PassStateError, TimestampWritesError,
 };
 use crate::pipeline::{RenderPipeline, VertexStep};
 use crate::resource::RawResourceAccess;
@@ -671,6 +671,8 @@ pub enum RenderPassErrorInner {
     EncoderState(#[from] EncoderStateError),
     #[error("Parent encoder is invalid")]
     InvalidParentEncoder,
+    #[error(transparent)]
+    DebugGroupError(#[from] DebugGroupError),
     #[error("The format of the {location} ({format:?}) is not resolvable")]
     UnsupportedResolveTargetFormat {
         location: AttachmentErrorLocation,
@@ -738,8 +740,6 @@ pub enum RenderPassErrorInner {
         count_buffer_size: u64,
     },
     #[error(transparent)]
-    InvalidPopDebugGroup(#[from] pass::InvalidPopDebugGroup),
-    #[error(transparent)]
     ResourceUsageCompatibility(#[from] ResourceUsageCompatibilityError),
     #[error("Render bundle has incompatible targets, {0}")]
     IncompatibleBundleTargets(#[from] RenderPassCompatibilityError),
@@ -842,6 +842,7 @@ impl WebGpuError for RenderPassError {
             RenderPassErrorInner::Device(e) => e,
             RenderPassErrorInner::ColorAttachment(e) => e,
             RenderPassErrorInner::EncoderState(e) => e,
+            RenderPassErrorInner::DebugGroupError(e) => e,
             RenderPassErrorInner::MissingFeatures(e) => e,
             RenderPassErrorInner::MissingDownlevelFlags(e) => e,
             RenderPassErrorInner::RenderCommand(e) => e,
@@ -854,7 +855,6 @@ impl WebGpuError for RenderPassError {
             RenderPassErrorInner::InvalidAttachment(e) => e,
             RenderPassErrorInner::TimestampWrites(e) => e,
             RenderPassErrorInner::InvalidValuesOffset(e) => e,
-            RenderPassErrorInner::InvalidPopDebugGroup(e) => e,
 
             RenderPassErrorInner::InvalidParentEncoder
             | RenderPassErrorInner::UnsupportedResolveTargetFormat { .. }
@@ -2220,6 +2220,13 @@ impl Global {
                     }
                 }
 
+                if state.general.debug_scope_depth > 0 {
+                    Err(
+                        RenderPassErrorInner::DebugGroupError(DebugGroupError::MissingPop)
+                            .map_pass_err(pass_scope),
+                    )?;
+                }
+
                 state
                     .info
                     .finish(