dawn_native: Do debug marker validation at encoding time.

This also adds missing coverage for push/pop debug group in render
bundles. The RenderBundleEncoder didn't validate itself on Finish, so
add a regression test for that too.

The overarching goal with this CL is to do validation at encoding time
which will help produce SyncScopeResourceUsage in the frontend for
dispatch() calls so that they can be reused by the backends.

Bug: dawn:635
Change-Id: Ie5a2d987fda3854b3145ba4b7a34994ea605e820
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/38842
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Auto-Submit: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>

src/dawn_native/CommandEncoder.cpp

@@ -790,7 +790,13 @@ namespace dawn_native {
 
     void CommandEncoder::PopDebugGroup() {
         mEncodingContext.TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
+            if (GetDevice()->IsValidationEnabled()) {
+                if (mDebugGroupStackSize == 0) {
+                    return DAWN_VALIDATION_ERROR("Pop must be balanced by a corresponding Push.");
+                }
+            }
             allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup);
+            mDebugGroupStackSize--;
 
             return {};
         });
@@ -805,6 +811,8 @@ namespace dawn_native {
             char* label = allocator->AllocateData<char>(cmd->length + 1);
             memcpy(label, groupLabel, cmd->length + 1);
 
+            mDebugGroupStackSize++;
+
             return {};
         });
     }
@@ -890,7 +898,9 @@ namespace dawn_native {
             DAWN_TRY(ValidatePassResourceUsage(passUsage));
         }
 
-        uint64_t debugGroupStackSize = 0;
+        if (mDebugGroupStackSize != 0) {
+            return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop.");
+        }
 
         commands->Reset();
         Command type;
@@ -936,15 +946,12 @@ namespace dawn_native {
 
                 case Command::PopDebugGroup: {
                     commands->NextCommand<PopDebugGroupCmd>();
-                    DAWN_TRY(ValidateCanPopDebugGroup(debugGroupStackSize));
-                    debugGroupStackSize--;
                     break;
                 }
 
                 case Command::PushDebugGroup: {
                     const PushDebugGroupCmd* cmd = commands->NextCommand<PushDebugGroupCmd>();
                     commands->NextData<char>(cmd->length + 1);
-                    debugGroupStackSize++;
                     break;
                 }
 
@@ -962,8 +969,6 @@ namespace dawn_native {
             }
         }
 
-        DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize));
-
         return {};
     }
 

src/dawn_native/CommandEncoder.h

@@ -84,6 +84,8 @@ namespace dawn_native {
         std::set<TextureBase*> mTopLevelTextures;
         std::set<QuerySetBase*> mUsedQuerySets;
         QueryAvailabilityMap mQueryAvailabilityMap;
+
+        uint64_t mDebugGroupStackSize = 0;
     };
 
 }  // namespace dawn_native

src/dawn_native/CommandValidation.cpp

@@ -33,7 +33,6 @@ namespace dawn_native {
                                                       Command type,
                                                       CommandBufferStateTracker* commandBufferState,
                                                       const AttachmentState* attachmentState,
-                                                      uint64_t* debugGroupStackSize,
                                                       const char* disallowedMessage) {
             switch (type) {
                 case Command::Draw: {
@@ -68,15 +67,12 @@ namespace dawn_native {
 
                 case Command::PopDebugGroup: {
                     commands->NextCommand<PopDebugGroupCmd>();
-                    DAWN_TRY(ValidateCanPopDebugGroup(*debugGroupStackSize));
-                    *debugGroupStackSize -= 1;
                     break;
                 }
 
                 case Command::PushDebugGroup: {
                     PushDebugGroupCmd* cmd = commands->NextCommand<PushDebugGroupCmd>();
                     commands->NextData<char>(cmd->length + 1);
-                    *debugGroupStackSize += 1;
                     break;
                 }
 
@@ -122,40 +118,21 @@ namespace dawn_native {
 
     }  // namespace
 
-    MaybeError ValidateCanPopDebugGroup(uint64_t debugGroupStackSize) {
-        if (debugGroupStackSize == 0) {
-            return DAWN_VALIDATION_ERROR("Pop must be balanced by a corresponding Push.");
-        }
-        return {};
-    }
-
-    MaybeError ValidateFinalDebugGroupStackSize(uint64_t debugGroupStackSize) {
-        if (debugGroupStackSize != 0) {
-            return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop.");
-        }
-        return {};
-    }
-
     MaybeError ValidateRenderBundle(CommandIterator* commands,
                                     const AttachmentState* attachmentState) {
         CommandBufferStateTracker commandBufferState;
-        uint64_t debugGroupStackSize = 0;
-
         Command type;
         while (commands->NextCommandId(&type)) {
             DAWN_TRY(ValidateRenderBundleCommand(commands, type, &commandBufferState,
-                                                 attachmentState, &debugGroupStackSize,
+                                                 attachmentState,
                                                  "Command disallowed inside a render bundle"));
         }
 
-        DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize));
         return {};
     }
 
     MaybeError ValidateRenderPass(CommandIterator* commands, const BeginRenderPassCmd* renderPass) {
         CommandBufferStateTracker commandBufferState;
-        uint64_t debugGroupStackSize = 0;
-
         Command type;
         while (commands->NextCommandId(&type)) {
             switch (type) {
@@ -171,7 +148,6 @@ namespace dawn_native {
 
                 case Command::EndRenderPass: {
                     commands->NextCommand<EndRenderPassCmd>();
-                    DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize));
                     return {};
                 }
 
@@ -222,7 +198,7 @@ namespace dawn_native {
                 default:
                     DAWN_TRY(ValidateRenderBundleCommand(
                         commands, type, &commandBufferState, renderPass->attachmentState.Get(),
-                        &debugGroupStackSize, "Command disallowed inside a render pass"));
+                        "Command disallowed inside a render pass"));
             }
         }
 
@@ -232,14 +208,11 @@ namespace dawn_native {
 
     MaybeError ValidateComputePass(CommandIterator* commands) {
         CommandBufferStateTracker commandBufferState;
-        uint64_t debugGroupStackSize = 0;
-
         Command type;
         while (commands->NextCommandId(&type)) {
             switch (type) {
                 case Command::EndComputePass: {
                     commands->NextCommand<EndComputePassCmd>();
-                    DAWN_TRY(ValidateFinalDebugGroupStackSize(debugGroupStackSize));
                     return {};
                 }
 
@@ -263,15 +236,12 @@ namespace dawn_native {
 
                 case Command::PopDebugGroup: {
                     commands->NextCommand<PopDebugGroupCmd>();
-                    DAWN_TRY(ValidateCanPopDebugGroup(debugGroupStackSize));
-                    debugGroupStackSize--;
                     break;
                 }
 
                 case Command::PushDebugGroup: {
                     PushDebugGroupCmd* cmd = commands->NextCommand<PushDebugGroupCmd>();
                     commands->NextData<char>(cmd->length + 1);
-                    debugGroupStackSize++;
                     break;
                 }
 

src/dawn_native/CommandValidation.h

@@ -29,9 +29,6 @@ namespace dawn_native {
     struct PassResourceUsage;
     struct TexelBlockInfo;
 
-    MaybeError ValidateCanPopDebugGroup(uint64_t debugGroupStackSize);
-    MaybeError ValidateFinalDebugGroupStackSize(uint64_t debugGroupStackSize);
-
     MaybeError ValidateRenderBundle(CommandIterator* commands,
                                     const AttachmentState* attachmentState);
     MaybeError ValidateRenderPass(CommandIterator* commands, const BeginRenderPassCmd* renderPass);

src/dawn_native/ComputePassEncoder.cpp

@@ -47,6 +47,10 @@ namespace dawn_native {
 
     void ComputePassEncoder::EndPass() {
         if (mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
+                if (IsValidationEnabled()) {
+                    DAWN_TRY(ValidateProgrammableEncoderEnd());
+                }
+
                 allocator->Allocate<EndComputePassCmd>(Command::EndComputePass);
 
                 return {};

src/dawn_native/ProgrammablePassEncoder.cpp

@@ -97,13 +97,20 @@ namespace dawn_native {
         : ObjectBase(device, errorTag),
           mEncodingContext(encodingContext),
           mUsageTracker(passType),
-          mValidationEnabled(false) {
+          mValidationEnabled(device->IsValidationEnabled()) {
     }
 
     bool ProgrammablePassEncoder::IsValidationEnabled() const {
         return mValidationEnabled;
     }
 
+    MaybeError ProgrammablePassEncoder::ValidateProgrammableEncoderEnd() const {
+        if (mDebugGroupStackSize != 0) {
+            return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop.");
+        }
+        return {};
+    }
+
     void ProgrammablePassEncoder::InsertDebugMarker(const char* groupLabel) {
         mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
             InsertDebugMarkerCmd* cmd =
@@ -119,7 +126,13 @@ namespace dawn_native {
 
     void ProgrammablePassEncoder::PopDebugGroup() {
         mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
+            if (IsValidationEnabled()) {
+                if (mDebugGroupStackSize == 0) {
+                    return DAWN_VALIDATION_ERROR("Pop must be balanced by a corresponding Push.");
+                }
+            }
             allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup);
+            mDebugGroupStackSize--;
 
             return {};
         });
@@ -134,6 +147,8 @@ namespace dawn_native {
             char* label = allocator->AllocateData<char>(cmd->length + 1);
             memcpy(label, groupLabel, cmd->length + 1);
 
+            mDebugGroupStackSize++;
+
             return {};
         });
     }

src/dawn_native/ProgrammablePassEncoder.h

@@ -45,6 +45,7 @@ namespace dawn_native {
 
       protected:
         bool IsValidationEnabled() const;
+        MaybeError ValidateProgrammableEncoderEnd() const;
 
         // Construct an "error" programmable pass encoder.
         ProgrammablePassEncoder(DeviceBase* device,
@@ -55,6 +56,8 @@ namespace dawn_native {
         EncodingContext* mEncodingContext = nullptr;
         PassResourceUsageTracker mUsageTracker;
 
+        uint64_t mDebugGroupStackSize = 0;
+
       private:
         const bool mValidationEnabled;
     };

src/dawn_native/RenderBundleEncoder.cpp

@@ -103,19 +103,29 @@ namespace dawn_native {
     }
 
     RenderBundleBase* RenderBundleEncoder::Finish(const RenderBundleDescriptor* descriptor) {
-        PassResourceUsage usages = mUsageTracker.AcquireResourceUsage();
+        RenderBundleBase* result = nullptr;
 
-        DeviceBase* device = GetDevice();
+        if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result)) {
+            return RenderBundleBase::MakeError(GetDevice());
+        }
+
+        return result;
+    }
+
+    ResultOrError<RenderBundleBase*> RenderBundleEncoder::FinishImpl(
+        const RenderBundleDescriptor* descriptor) {
         // Even if mBundleEncodingContext.Finish() validation fails, calling it will mutate the
         // internal state of the encoding context. Subsequent calls to encode commands will generate
         // errors.
-        if (device->ConsumedError(mBundleEncodingContext.Finish()) ||
+        DAWN_TRY(mBundleEncodingContext.Finish());
-            (device->IsValidationEnabled() &&
+
-             device->ConsumedError(ValidateFinish(mBundleEncodingContext.GetIterator(), usages)))) {
+        PassResourceUsage usages = mUsageTracker.AcquireResourceUsage();
-            return RenderBundleBase::MakeError(device);
+        if (IsValidationEnabled()) {
+            DAWN_TRY(GetDevice()->ValidateObject(this));
+            DAWN_TRY(ValidateProgrammableEncoderEnd());
+            DAWN_TRY(ValidateFinish(mBundleEncodingContext.GetIterator(), usages));
         }
 
-        ASSERT(!IsError());
         return new RenderBundleBase(this, descriptor, mAttachmentState.Get(), std::move(usages));
     }
 

src/dawn_native/RenderBundleEncoder.h

@@ -42,6 +42,7 @@ namespace dawn_native {
       private:
         RenderBundleEncoder(DeviceBase* device, ErrorTag errorTag);
 
+        ResultOrError<RenderBundleBase*> FinishImpl(const RenderBundleDescriptor* descriptor);
         MaybeError ValidateFinish(CommandIterator* commands, const PassResourceUsage& usages) const;
 
         EncodingContext mBundleEncodingContext;

src/dawn_native/RenderPassEncoder.cpp

@@ -94,15 +94,15 @@ namespace dawn_native {
 
     void RenderPassEncoder::EndPass() {
         if (mEncodingContext->TryEncode(this, [&](CommandAllocator* allocator) -> MaybeError {
-                allocator->Allocate<EndRenderPassCmd>(Command::EndRenderPass);
-
                 if (IsValidationEnabled()) {
+                    DAWN_TRY(ValidateProgrammableEncoderEnd());
                     if (mOcclusionQueryActive) {
                         return DAWN_VALIDATION_ERROR(
                             "The occlusion query must be ended before endPass.");
                     }
                 }
 
+                allocator->Allocate<EndRenderPassCmd>(Command::EndRenderPass);
                 return {};
             })) {
             mEncodingContext->ExitPass(this, mUsageTracker.AcquireResourceUsage());

src/tests/unittests/validation/DebugMarkerValidationTests.cpp

@@ -14,6 +14,7 @@
 
 #include "tests/unittests/validation/ValidationTest.h"
 
+#include "utils/ComboRenderBundleEncoderDescriptor.h"
 #include "utils/WGPUHelpers.h"
 
 class DebugMarkerValidationTest : public ValidationTest {};
@@ -70,6 +71,52 @@ TEST_F(DebugMarkerValidationTest, RenderUnbalancedPop) {
     ASSERT_DEVICE_ERROR(encoder.Finish());
 }
 
+// Correct usage of debug markers should succeed in render bundle.
+TEST_F(DebugMarkerValidationTest, RenderBundleSuccess) {
+    utils::ComboRenderBundleEncoderDescriptor desc;
+    desc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
+    desc.colorFormatsCount = 1;
+
+    wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&desc);
+    encoder.PushDebugGroup("Event Start");
+    encoder.PushDebugGroup("Event Start");
+    encoder.InsertDebugMarker("Marker");
+    encoder.PopDebugGroup();
+    encoder.PopDebugGroup();
+
+    encoder.Finish();
+}
+
+// A PushDebugGroup call without a following PopDebugGroup produces an error in render bundle.
+TEST_F(DebugMarkerValidationTest, RenderBundleUnbalancedPush) {
+    utils::ComboRenderBundleEncoderDescriptor desc;
+    desc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
+    desc.colorFormatsCount = 1;
+
+    wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&desc);
+    encoder.PushDebugGroup("Event Start");
+    encoder.PushDebugGroup("Event Start");
+    encoder.InsertDebugMarker("Marker");
+    encoder.PopDebugGroup();
+
+    ASSERT_DEVICE_ERROR(encoder.Finish());
+}
+
+// A PopDebugGroup call without a preceding PushDebugGroup produces an error in render bundle.
+TEST_F(DebugMarkerValidationTest, RenderBundleUnbalancedPop) {
+    utils::ComboRenderBundleEncoderDescriptor desc;
+    desc.cColorFormats[0] = wgpu::TextureFormat::RGBA8Unorm;
+    desc.colorFormatsCount = 1;
+
+    wgpu::RenderBundleEncoder encoder = device.CreateRenderBundleEncoder(&desc);
+    encoder.PushDebugGroup("Event Start");
+    encoder.InsertDebugMarker("Marker");
+    encoder.PopDebugGroup();
+    encoder.PopDebugGroup();
+
+    ASSERT_DEVICE_ERROR(encoder.Finish());
+}
+
 // Correct usage of debug markers should succeed in compute pass.
 TEST_F(DebugMarkerValidationTest, ComputeSuccess) {
     wgpu::CommandEncoder encoder = device.CreateCommandEncoder();

src/tests/unittests/validation/RenderBundleValidationTests.cpp

@@ -141,6 +141,28 @@ TEST_F(RenderBundleValidationTest, Empty) {
     commandEncoder.Finish();
 }
 
+// Test that an empty error bundle encoder produces an error bundle.
+// This is a regression test for error render bundle encoders containing no commands would
+// produce non-error render bundles.
+TEST_F(RenderBundleValidationTest, EmptyErrorEncoderProducesErrorBundle) {
+    DummyRenderPass renderPass(device);
+
+    utils::ComboRenderBundleEncoderDescriptor desc = {};
+    // Having 0 attachments is invalid!
+    desc.colorFormatsCount = 0;
+
+    wgpu::RenderBundleEncoder renderBundleEncoder;
+    ASSERT_DEVICE_ERROR(renderBundleEncoder = device.CreateRenderBundleEncoder(&desc));
+    wgpu::RenderBundle renderBundle;
+    ASSERT_DEVICE_ERROR(renderBundle = renderBundleEncoder.Finish());
+
+    wgpu::CommandEncoder commandEncoder = device.CreateCommandEncoder();
+    wgpu::RenderPassEncoder pass = commandEncoder.BeginRenderPass(&renderPass);
+    pass.ExecuteBundles(1, &renderBundle);
+    pass.EndPass();
+    ASSERT_DEVICE_ERROR(commandEncoder.Finish());
+}
+
 // Test executing zero render bundles.
 TEST_F(RenderBundleValidationTest, ZeroBundles) {
     DummyRenderPass renderPass(device);