Revert "Improve validation errors for encoders"

This reverts commit c7e6bb0d8d.

Reason for revert: clusterfuzz identified issue https://bugs.chromium.org/p/chromium/issues/detail?id=1262112

Original change's description:
> Improve validation errors for encoders
>
> Improves the validation messages in ComputePassEncoder.cpp,
> ProgrammablePassEncoder.cpp, RenderBundleEncoder.cpp, and
> EncodingContext.cpp/h to give them more contextual information.
>
> Bug: dawn:563
> Change-Id: I87c46c4bfda1375809fae93239029ea4e3b9c0a2
> Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67000
> Commit-Queue: Brandon Jones <bajones@chromium.org>
> Reviewed-by: Corentin Wallez <cwallez@chromium.org>

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: dawn:563
Change-Id: I259ccde1735c4201ff2736562cfe4689e9a22f62
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67321
Reviewed-by: Brandon Jones <bajones@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>

src/dawn_native/CommandEncoder.cpp

@@ -891,7 +891,8 @@ namespace dawn_native {
                 if (GetDevice()->IsValidationEnabled()) {
                     DAWN_INVALID_IF(
                         mDebugGroupStackSize == 0,
-                        "PopDebugGroup called when no debug groups are currently pushed.");
+                        "Every call to PopDebugGroup must be balanced by a corresponding call to "
+                        "PushDebugGroup.");
                 }
                 allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup);
                 mDebugGroupStackSize--;

src/dawn_native/ComputePassEncoder.cpp

@@ -26,6 +26,18 @@
 
 namespace dawn_native {
 
+    namespace {
+
+        MaybeError ValidatePerDimensionDispatchSizeLimit(const DeviceBase* device, uint32_t size) {
+            if (size > device->GetLimits().v1.maxComputeWorkgroupsPerDimension) {
+                return DAWN_VALIDATION_ERROR("Dispatch size exceeds defined limits");
+            }
+
+            return {};
+        }
+
+    }  // namespace
+
     ComputePassEncoder::ComputePassEncoder(DeviceBase* device,
                                            CommandEncoder* commandEncoder,
                                            EncodingContext* encodingContext)
@@ -73,24 +85,9 @@ namespace dawn_native {
             [&](CommandAllocator* allocator) -> MaybeError {
                 if (IsValidationEnabled()) {
                     DAWN_TRY(mCommandBufferState.ValidateCanDispatch());
-
+                    DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), x));
-                    uint32_t workgroupsPerDimension =
+                    DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), y));
-                        GetDevice()->GetLimits().v1.maxComputeWorkgroupsPerDimension;
+                    DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), z));
-
-                    DAWN_INVALID_IF(
-                        x > workgroupsPerDimension,
-                        "Dispatch size X (%u) exceeds max compute workgroups per dimension (%u).",
-                        x, workgroupsPerDimension);
-
-                    DAWN_INVALID_IF(
-                        y > workgroupsPerDimension,
-                        "Dispatch size Y (%u) exceeds max compute workgroups per dimension (%u).",
-                        y, workgroupsPerDimension);
-
-                    DAWN_INVALID_IF(
-                        z > workgroupsPerDimension,
-                        "Dispatch size Z (%u) exceeds max compute workgroups per dimension (%u).",
-                        z, workgroupsPerDimension);
                 }
 
                 // Record the synchronization scope for Dispatch, which is just the current
@@ -120,20 +117,21 @@ namespace dawn_native {
                     // Indexed dispatches need a compute-shader based validation to check that the
                     // dispatch sizes aren't too big. Disallow them as unsafe until the validation
                     // is implemented.
-                    DAWN_INVALID_IF(
+                    if (GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs)) {
-                        GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs),
+                        return DAWN_VALIDATION_ERROR(
                             "DispatchIndirect is disallowed because it doesn't validate that the "
-                        "dispatch size is valid yet.");
+                            "dispatch "
+                            "size is valid yet.");
+                    }
 
-                    DAWN_INVALID_IF(indirectOffset % 4 != 0,
+                    if (indirectOffset % 4 != 0) {
-                                    "Indirect offset (%u) is not a multiple of 4.", indirectOffset);
+                        return DAWN_VALIDATION_ERROR("Indirect offset must be a multiple of 4");
+                    }
 
-                    DAWN_INVALID_IF(
+                    if (indirectOffset >= indirectBuffer->GetSize() ||
-                        indirectOffset >= indirectBuffer->GetSize() ||
+                        indirectOffset + kDispatchIndirectSize > indirectBuffer->GetSize()) {
-                            indirectOffset + kDispatchIndirectSize > indirectBuffer->GetSize(),
+                        return DAWN_VALIDATION_ERROR("Indirect offset out of bounds");
-                        "Indirect offset (%u) and dispatch size (%u) exceeds the indirect buffer "
+                    }
-                        "size (%u).",
-                        indirectOffset, kDispatchIndirectSize, indirectBuffer->GetSize());
                 }
 
                 // Record the synchronization scope for Dispatch, both the bindgroups and the

src/dawn_native/EncodingContext.cpp

@@ -24,7 +24,7 @@
 
 namespace dawn_native {
 
-    EncodingContext::EncodingContext(DeviceBase* device, const ApiObjectBase* initialEncoder)
+    EncodingContext::EncodingContext(DeviceBase* device, const ObjectBase* initialEncoder)
         : mDevice(device), mTopLevelEncoder(initialEncoder), mCurrentEncoder(initialEncoder) {
     }
 
@@ -87,7 +87,7 @@ namespace dawn_native {
         }
     }
 
-    void EncodingContext::EnterPass(const ApiObjectBase* passEncoder) {
+    void EncodingContext::EnterPass(const ObjectBase* passEncoder) {
         // Assert we're at the top level.
         ASSERT(mCurrentEncoder == mTopLevelEncoder);
         ASSERT(passEncoder != nullptr);
@@ -95,7 +95,7 @@ namespace dawn_native {
         mCurrentEncoder = passEncoder;
     }
 
-    MaybeError EncodingContext::ExitRenderPass(const ApiObjectBase* passEncoder,
+    MaybeError EncodingContext::ExitRenderPass(const ObjectBase* passEncoder,
                                                RenderPassResourceUsageTracker usageTracker,
                                                CommandEncoder* commandEncoder,
                                                IndirectDrawMetadata indirectDrawMetadata) {
@@ -121,7 +121,7 @@ namespace dawn_native {
         return {};
     }
 
-    void EncodingContext::ExitComputePass(const ApiObjectBase* passEncoder,
+    void EncodingContext::ExitComputePass(const ObjectBase* passEncoder,
                                           ComputePassResourceUsage usages) {
         ASSERT(mCurrentEncoder != mTopLevelEncoder);
         ASSERT(mCurrentEncoder == passEncoder);
@@ -161,10 +161,12 @@ namespace dawn_native {
     }
 
     MaybeError EncodingContext::Finish() {
-        DAWN_INVALID_IF(IsFinished(), "Command encoding already finished.");
+        if (IsFinished()) {
+            return DAWN_VALIDATION_ERROR("Command encoding already finished");
+        }
 
-        const ApiObjectBase* currentEncoder = mCurrentEncoder;
+        const void* currentEncoder = mCurrentEncoder;
-        const ApiObjectBase* topLevelEncoder = mTopLevelEncoder;
+        const void* topLevelEncoder = mTopLevelEncoder;
 
         // Even if finish validation fails, it is now invalid to call any encoding commands,
         // so we clear the encoders. Note: mTopLevelEncoder == nullptr is used as a flag for
@@ -176,8 +178,9 @@ namespace dawn_native {
         if (mError != nullptr) {
             return std::move(mError);
         }
-        DAWN_INVALID_IF(currentEncoder != topLevelEncoder,
+        if (currentEncoder != topLevelEncoder) {
-                        "Command buffer recording ended before %s was ended.", currentEncoder);
+            return DAWN_VALIDATION_ERROR("Command buffer recording ended mid-pass");
+        }
         return {};
     }
 

src/dawn_native/EncodingContext.h

@@ -28,13 +28,13 @@ namespace dawn_native {
 
     class CommandEncoder;
     class DeviceBase;
-    class ApiObjectBase;
+    class ObjectBase;
 
     // Base class for allocating/iterating commands.
     // It performs error tracking as well as encoding state for render/compute passes.
     class EncodingContext {
       public:
-        EncodingContext(DeviceBase* device, const ApiObjectBase* initialEncoder);
+        EncodingContext(DeviceBase* device, const ObjectBase* initialEncoder);
         ~EncodingContext();
 
         CommandIterator AcquireCommands();
@@ -70,15 +70,14 @@ namespace dawn_native {
             return false;
         }
 
-        inline bool CheckCurrentEncoder(const ApiObjectBase* encoder) {
+        inline bool CheckCurrentEncoder(const ObjectBase* encoder) {
             if (DAWN_UNLIKELY(encoder != mCurrentEncoder)) {
                 if (mCurrentEncoder != mTopLevelEncoder) {
                     // The top level encoder was used when a pass encoder was current.
-                    HandleError(DAWN_FORMAT_VALIDATION_ERROR(
+                    HandleError(DAWN_VALIDATION_ERROR("Command cannot be recorded inside a pass"));
-                        "Command cannot be recorded while %s is active.", mCurrentEncoder));
                 } else {
-                    HandleError(DAWN_FORMAT_VALIDATION_ERROR(
+                    HandleError(DAWN_VALIDATION_ERROR(
-                        "Recording in an error or already ended %s.", encoder));
+                        "Recording in an error or already ended pass encoder"));
                 }
                 return false;
             }
@@ -86,7 +85,7 @@ namespace dawn_native {
         }
 
         template <typename EncodeFunction>
-        inline bool TryEncode(const ApiObjectBase* encoder, EncodeFunction&& encodeFunction) {
+        inline bool TryEncode(const ObjectBase* encoder, EncodeFunction&& encodeFunction) {
             if (!CheckCurrentEncoder(encoder)) {
                 return false;
             }
@@ -95,7 +94,7 @@ namespace dawn_native {
         }
 
         template <typename EncodeFunction, typename... Args>
-        inline bool TryEncode(const ApiObjectBase* encoder,
+        inline bool TryEncode(const ObjectBase* encoder,
                               EncodeFunction&& encodeFunction,
                               const char* formatStr,
                               const Args&... args) {
@@ -112,12 +111,12 @@ namespace dawn_native {
         void WillBeginRenderPass();
 
         // Functions to set current encoder state
-        void EnterPass(const ApiObjectBase* passEncoder);
+        void EnterPass(const ObjectBase* passEncoder);
-        MaybeError ExitRenderPass(const ApiObjectBase* passEncoder,
+        MaybeError ExitRenderPass(const ObjectBase* passEncoder,
                                   RenderPassResourceUsageTracker usageTracker,
                                   CommandEncoder* commandEncoder,
                                   IndirectDrawMetadata indirectDrawMetadata);
-        void ExitComputePass(const ApiObjectBase* passEncoder, ComputePassResourceUsage usages);
+        void ExitComputePass(const ObjectBase* passEncoder, ComputePassResourceUsage usages);
         MaybeError Finish();
 
         const RenderPassUsages& GetRenderPassUsages() const;
@@ -139,12 +138,12 @@ namespace dawn_native {
         // There can only be two levels of encoders. Top-level and render/compute pass.
         // The top level encoder is the encoder the EncodingContext is created with.
         // It doubles as flag to check if encoding has been Finished.
-        const ApiObjectBase* mTopLevelEncoder;
+        const ObjectBase* mTopLevelEncoder;
         // The current encoder must be the same as the encoder provided to TryEncode,
         // otherwise an error is produced. It may be nullptr if the EncodingContext is an error.
         // The current encoder changes with Enter/ExitPass which should be called by
         // CommandEncoder::Begin/EndPass.
-        const ApiObjectBase* mCurrentEncoder;
+        const ObjectBase* mCurrentEncoder;
 
         RenderPassUsages mRenderPassUsages;
         bool mWereRenderPassUsagesAcquired = false;

src/dawn_native/ProgrammablePassEncoder.cpp

@@ -48,9 +48,9 @@ namespace dawn_native {
     }
 
     MaybeError ProgrammablePassEncoder::ValidateProgrammableEncoderEnd() const {
-        DAWN_INVALID_IF(mDebugGroupStackSize != 0,
+        if (mDebugGroupStackSize != 0) {
-                        "PushDebugGroup called %u time(s) without a corresponding PopDebugGroup.",
+            return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop.");
-                        mDebugGroupStackSize);
+        }
         return {};
     }
 
@@ -75,9 +75,10 @@ namespace dawn_native {
             this,
             [&](CommandAllocator* allocator) -> MaybeError {
                 if (IsValidationEnabled()) {
-                    DAWN_INVALID_IF(
+                    if (mDebugGroupStackSize == 0) {
-                        mDebugGroupStackSize == 0,
+                        return DAWN_VALIDATION_ERROR(
-                        "PopDebugGroup called when no debug groups are currently pushed.");
+                            "Pop must be balanced by a corresponding Push.");
+                    }
                 }
                 allocator->Allocate<PopDebugGroupCmd>(Command::PopDebugGroup);
                 mDebugGroupStackSize--;
@@ -114,21 +115,18 @@ namespace dawn_native {
         const uint32_t* dynamicOffsetsIn) const {
         DAWN_TRY(GetDevice()->ValidateObject(group));
 
-        DAWN_INVALID_IF(index >= kMaxBindGroupsTyped,
+        if (index >= kMaxBindGroupsTyped) {
-                        "Bind group index (%u) exceeds the maximum (%u).",
+            return DAWN_VALIDATION_ERROR("Setting bind group over the max");
-                        static_cast<uint32_t>(index), kMaxBindGroups);
+        }
 
         ityp::span<BindingIndex, const uint32_t> dynamicOffsets(dynamicOffsetsIn,
                                                                 BindingIndex(dynamicOffsetCountIn));
 
         // Dynamic offsets count must match the number required by the layout perfectly.
         const BindGroupLayoutBase* layout = group->GetLayout();
-        DAWN_INVALID_IF(
+        if (layout->GetDynamicBufferCount() != dynamicOffsets.size()) {
-            layout->GetDynamicBufferCount() != dynamicOffsets.size(),
+            return DAWN_VALIDATION_ERROR("dynamicOffset count mismatch");
-            "The number of dynamic offsets (%u) does not match the number of dynamic buffers (%u) "
+        }
-            "in %s.",
-            static_cast<uint32_t>(dynamicOffsets.size()),
-            static_cast<uint32_t>(layout->GetDynamicBufferCount()), layout);
 
         for (BindingIndex i{0}; i < dynamicOffsets.size(); ++i) {
             const BindingInfo& bindingInfo = layout->GetBindingInfo(i);
@@ -152,9 +150,9 @@ namespace dawn_native {
                     UNREACHABLE();
             }
 
-            DAWN_INVALID_IF(!IsAligned(dynamicOffsets[i], requiredAlignment),
+            if (!IsAligned(dynamicOffsets[i], requiredAlignment)) {
-                            "Dynamic Offset[%u] (%u) is not %u byte aligned.",
+                return DAWN_VALIDATION_ERROR("Dynamic Buffer Offset need to be aligned");
-                            static_cast<uint32_t>(i), dynamicOffsets[i], requiredAlignment);
+            }
 
             BufferBinding bufferBinding = group->GetBindingAsBufferBinding(i);
 
@@ -165,20 +163,15 @@ namespace dawn_native {
 
             if ((dynamicOffsets[i] >
                  bufferBinding.buffer->GetSize() - bufferBinding.offset - bufferBinding.size)) {
-                DAWN_INVALID_IF(
+                if ((bufferBinding.buffer->GetSize() - bufferBinding.offset) ==
-                    (bufferBinding.buffer->GetSize() - bufferBinding.offset) == bufferBinding.size,
+                    bufferBinding.size) {
-                    "Dynamic Offset[%u] (%u) is out of bounds of %s with a size of %u and a bound "
+                    return DAWN_VALIDATION_ERROR(
-                    "range of (offset: %u, size: %u). The binding goes to the end of the buffer "
+                        "Dynamic offset out of bounds. The binding goes to the end of the "
-                    "even with a dynamic offset of 0. Did you forget to specify "
+                        "buffer even with a dynamic offset of 0. Did you forget to specify "
-                    "the binding's size?",
+                        "the binding's size?");
-                    static_cast<uint32_t>(i), dynamicOffsets[i], bufferBinding.buffer,
+                } else {
-                    bufferBinding.buffer->GetSize(), bufferBinding.offset, bufferBinding.size);
+                    return DAWN_VALIDATION_ERROR("Dynamic offset out of bounds");
-
+                }
-                return DAWN_FORMAT_VALIDATION_ERROR(
-                    "Dynamic Offset[%u] (%u) is out of bounds of "
-                    "%s with a size of %u and a bound range of (offset: %u, size: %u).",
-                    static_cast<uint32_t>(i), dynamicOffsets[i], bufferBinding.buffer,
-                    bufferBinding.buffer->GetSize(), bufferBinding.offset, bufferBinding.size);
             }
         }
 

src/dawn_native/RenderBundleEncoder.cpp

@@ -123,8 +123,7 @@ namespace dawn_native {
     RenderBundleBase* RenderBundleEncoder::APIFinish(const RenderBundleDescriptor* descriptor) {
         RenderBundleBase* result = nullptr;
 
-        if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result, "calling Finish(%s).",
+        if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result)) {
-                                       descriptor)) {
             return RenderBundleBase::MakeError(GetDevice());
         }