From 910143d622e9bc3b3237b0ad2018574e49b6fadc Mon Sep 17 00:00:00 2001 From: Andy Leiserson Date: Tue, 5 Aug 2025 07:52:50 -0700 Subject: [PATCH] Check for more cases of unbalanced debug groups (#8048) Fixes #3911 --- cts_runner/test.lst | 14 ++++++++----- deno_webgpu/render_bundle.rs | 12 ++++++++--- wgpu-core/src/command/compute.rs | 15 ++++++++++---- wgpu-core/src/command/mod.rs | 34 ++++++++++++++++++++++++++++++++ wgpu-core/src/command/pass.rs | 16 +++------------ wgpu-core/src/command/render.rs | 17 +++++++++++----- 6 files changed, 78 insertions(+), 30 deletions(-) diff --git a/cts_runner/test.lst b/cts_runner/test.lst index 727530d29..26c1db582 100644 --- a/cts_runner/test.lst +++ b/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 . diff --git a/deno_webgpu/render_bundle.rs b/deno_webgpu/render_bundle.rs index de6dec878..da31929d8 100644 --- a/deno_webgpu/render_bundle.rs +++ b/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>>(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 { diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index 3d26d60ea..0d8aaf27e 100644 --- a/wgpu-core/src/command/compute.rs +++ b/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(); } diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index 3abcf8307..2467784d5 100644 --- a/wgpu-core/src/command/mod.rs +++ b/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>, } @@ -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); diff --git a/wgpu-core/src/command/pass.rs b/wgpu-core/src/command/pass.rs index 146711ced..916f99118 100644 --- a/wgpu-core/src/command/pass.rs +++ b/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, @@ -332,12 +322,12 @@ pub(crate) fn push_debug_group(state: &mut BaseState, string_data: &[u8], len: u pub(crate) fn pop_debug_group(state: &mut BaseState) -> Result<(), E> where - E: From, + E: From, { 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 diff --git a/wgpu-core/src/command/render.rs b/wgpu-core/src/command/render.rs index 12d5bc86d..abc8163c6 100644 --- a/wgpu-core/src/command/render.rs +++ b/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(