From c7e6bb0d8d8f3bed3ca27ff822631c1671a4e5b2 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Wed, 20 Oct 2021 17:13:48 +0000 Subject: [PATCH] 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 Reviewed-by: Corentin Wallez --- src/dawn_native/CommandEncoder.cpp | 3 +- src/dawn_native/ComputePassEncoder.cpp | 58 +++++++++++---------- src/dawn_native/EncodingContext.cpp | 21 ++++---- src/dawn_native/EncodingContext.h | 27 +++++----- src/dawn_native/ProgrammablePassEncoder.cpp | 57 +++++++++++--------- src/dawn_native/RenderBundleEncoder.cpp | 3 +- 6 files changed, 88 insertions(+), 81 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index ab0857f62e..c9510d0327 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -891,8 +891,7 @@ namespace dawn_native { if (GetDevice()->IsValidationEnabled()) { DAWN_INVALID_IF( mDebugGroupStackSize == 0, - "Every call to PopDebugGroup must be balanced by a corresponding call to " - "PushDebugGroup."); + "PopDebugGroup called when no debug groups are currently pushed."); } allocator->Allocate(Command::PopDebugGroup); mDebugGroupStackSize--; diff --git a/src/dawn_native/ComputePassEncoder.cpp b/src/dawn_native/ComputePassEncoder.cpp index 46277b13fc..1aa4845886 100644 --- a/src/dawn_native/ComputePassEncoder.cpp +++ b/src/dawn_native/ComputePassEncoder.cpp @@ -26,18 +26,6 @@ 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) @@ -85,9 +73,24 @@ namespace dawn_native { [&](CommandAllocator* allocator) -> MaybeError { if (IsValidationEnabled()) { DAWN_TRY(mCommandBufferState.ValidateCanDispatch()); - DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), x)); - DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), y)); - DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), z)); + + uint32_t workgroupsPerDimension = + GetDevice()->GetLimits().v1.maxComputeWorkgroupsPerDimension; + + 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 @@ -117,21 +120,20 @@ 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. - if (GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs)) { - return DAWN_VALIDATION_ERROR( - "DispatchIndirect is disallowed because it doesn't validate that the " - "dispatch " - "size is valid yet."); - } + DAWN_INVALID_IF( + GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs), + "DispatchIndirect is disallowed because it doesn't validate that the " + "dispatch size is valid yet."); - if (indirectOffset % 4 != 0) { - return DAWN_VALIDATION_ERROR("Indirect offset must be a multiple of 4"); - } + DAWN_INVALID_IF(indirectOffset % 4 != 0, + "Indirect offset (%u) is not a multiple of 4.", indirectOffset); - if (indirectOffset >= indirectBuffer->GetSize() || - indirectOffset + kDispatchIndirectSize > indirectBuffer->GetSize()) { - return DAWN_VALIDATION_ERROR("Indirect offset out of bounds"); - } + DAWN_INVALID_IF( + indirectOffset >= indirectBuffer->GetSize() || + indirectOffset + kDispatchIndirectSize > indirectBuffer->GetSize(), + "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 diff --git a/src/dawn_native/EncodingContext.cpp b/src/dawn_native/EncodingContext.cpp index 2724291234..7b14f5eddd 100644 --- a/src/dawn_native/EncodingContext.cpp +++ b/src/dawn_native/EncodingContext.cpp @@ -24,7 +24,7 @@ namespace dawn_native { - EncodingContext::EncodingContext(DeviceBase* device, const ObjectBase* initialEncoder) + EncodingContext::EncodingContext(DeviceBase* device, const ApiObjectBase* initialEncoder) : mDevice(device), mTopLevelEncoder(initialEncoder), mCurrentEncoder(initialEncoder) { } @@ -87,7 +87,7 @@ namespace dawn_native { } } - void EncodingContext::EnterPass(const ObjectBase* passEncoder) { + void EncodingContext::EnterPass(const ApiObjectBase* 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 ObjectBase* passEncoder, + MaybeError EncodingContext::ExitRenderPass(const ApiObjectBase* passEncoder, RenderPassResourceUsageTracker usageTracker, CommandEncoder* commandEncoder, IndirectDrawMetadata indirectDrawMetadata) { @@ -121,7 +121,7 @@ namespace dawn_native { return {}; } - void EncodingContext::ExitComputePass(const ObjectBase* passEncoder, + void EncodingContext::ExitComputePass(const ApiObjectBase* passEncoder, ComputePassResourceUsage usages) { ASSERT(mCurrentEncoder != mTopLevelEncoder); ASSERT(mCurrentEncoder == passEncoder); @@ -161,12 +161,10 @@ namespace dawn_native { } MaybeError EncodingContext::Finish() { - if (IsFinished()) { - return DAWN_VALIDATION_ERROR("Command encoding already finished"); - } + DAWN_INVALID_IF(IsFinished(), "Command encoding already finished."); - const void* currentEncoder = mCurrentEncoder; - const void* topLevelEncoder = mTopLevelEncoder; + const ApiObjectBase* currentEncoder = mCurrentEncoder; + const ApiObjectBase* 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 @@ -178,9 +176,8 @@ namespace dawn_native { if (mError != nullptr) { return std::move(mError); } - if (currentEncoder != topLevelEncoder) { - return DAWN_VALIDATION_ERROR("Command buffer recording ended mid-pass"); - } + DAWN_INVALID_IF(currentEncoder != topLevelEncoder, + "Command buffer recording ended before %s was ended.", currentEncoder); return {}; } diff --git a/src/dawn_native/EncodingContext.h b/src/dawn_native/EncodingContext.h index b058e2bc61..c395cf2d8f 100644 --- a/src/dawn_native/EncodingContext.h +++ b/src/dawn_native/EncodingContext.h @@ -28,13 +28,13 @@ namespace dawn_native { class CommandEncoder; class DeviceBase; - class ObjectBase; + class ApiObjectBase; // 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 ObjectBase* initialEncoder); + EncodingContext(DeviceBase* device, const ApiObjectBase* initialEncoder); ~EncodingContext(); CommandIterator AcquireCommands(); @@ -70,14 +70,15 @@ namespace dawn_native { return false; } - inline bool CheckCurrentEncoder(const ObjectBase* encoder) { + inline bool CheckCurrentEncoder(const ApiObjectBase* encoder) { if (DAWN_UNLIKELY(encoder != mCurrentEncoder)) { if (mCurrentEncoder != mTopLevelEncoder) { // The top level encoder was used when a pass encoder was current. - HandleError(DAWN_VALIDATION_ERROR("Command cannot be recorded inside a pass")); + HandleError(DAWN_FORMAT_VALIDATION_ERROR( + "Command cannot be recorded while %s is active.", mCurrentEncoder)); } else { - HandleError(DAWN_VALIDATION_ERROR( - "Recording in an error or already ended pass encoder")); + HandleError(DAWN_FORMAT_VALIDATION_ERROR( + "Recording in an error or already ended %s.", encoder)); } return false; } @@ -85,7 +86,7 @@ namespace dawn_native { } template - inline bool TryEncode(const ObjectBase* encoder, EncodeFunction&& encodeFunction) { + inline bool TryEncode(const ApiObjectBase* encoder, EncodeFunction&& encodeFunction) { if (!CheckCurrentEncoder(encoder)) { return false; } @@ -94,7 +95,7 @@ namespace dawn_native { } template - inline bool TryEncode(const ObjectBase* encoder, + inline bool TryEncode(const ApiObjectBase* encoder, EncodeFunction&& encodeFunction, const char* formatStr, const Args&... args) { @@ -111,12 +112,12 @@ namespace dawn_native { void WillBeginRenderPass(); // Functions to set current encoder state - void EnterPass(const ObjectBase* passEncoder); - MaybeError ExitRenderPass(const ObjectBase* passEncoder, + void EnterPass(const ApiObjectBase* passEncoder); + MaybeError ExitRenderPass(const ApiObjectBase* passEncoder, RenderPassResourceUsageTracker usageTracker, CommandEncoder* commandEncoder, IndirectDrawMetadata indirectDrawMetadata); - void ExitComputePass(const ObjectBase* passEncoder, ComputePassResourceUsage usages); + void ExitComputePass(const ApiObjectBase* passEncoder, ComputePassResourceUsage usages); MaybeError Finish(); const RenderPassUsages& GetRenderPassUsages() const; @@ -138,12 +139,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 ObjectBase* mTopLevelEncoder; + const ApiObjectBase* 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 ObjectBase* mCurrentEncoder; + const ApiObjectBase* mCurrentEncoder; RenderPassUsages mRenderPassUsages; bool mWereRenderPassUsagesAcquired = false; diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp index c6e2e288ff..8cf1b820dd 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp @@ -48,9 +48,9 @@ namespace dawn_native { } MaybeError ProgrammablePassEncoder::ValidateProgrammableEncoderEnd() const { - if (mDebugGroupStackSize != 0) { - return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop."); - } + DAWN_INVALID_IF(mDebugGroupStackSize != 0, + "PushDebugGroup called %u time(s) without a corresponding PopDebugGroup.", + mDebugGroupStackSize); return {}; } @@ -75,10 +75,9 @@ namespace dawn_native { this, [&](CommandAllocator* allocator) -> MaybeError { if (IsValidationEnabled()) { - if (mDebugGroupStackSize == 0) { - return DAWN_VALIDATION_ERROR( - "Pop must be balanced by a corresponding Push."); - } + DAWN_INVALID_IF( + mDebugGroupStackSize == 0, + "PopDebugGroup called when no debug groups are currently pushed."); } allocator->Allocate(Command::PopDebugGroup); mDebugGroupStackSize--; @@ -115,18 +114,21 @@ namespace dawn_native { const uint32_t* dynamicOffsetsIn) const { DAWN_TRY(GetDevice()->ValidateObject(group)); - if (index >= kMaxBindGroupsTyped) { - return DAWN_VALIDATION_ERROR("Setting bind group over the max"); - } + DAWN_INVALID_IF(index >= kMaxBindGroupsTyped, + "Bind group index (%u) exceeds the maximum (%u).", + static_cast(index), kMaxBindGroups); ityp::span dynamicOffsets(dynamicOffsetsIn, BindingIndex(dynamicOffsetCountIn)); // Dynamic offsets count must match the number required by the layout perfectly. const BindGroupLayoutBase* layout = group->GetLayout(); - if (layout->GetDynamicBufferCount() != dynamicOffsets.size()) { - return DAWN_VALIDATION_ERROR("dynamicOffset count mismatch"); - } + DAWN_INVALID_IF( + layout->GetDynamicBufferCount() != dynamicOffsets.size(), + "The number of dynamic offsets (%u) does not match the number of dynamic buffers (%u) " + "in %s.", + static_cast(dynamicOffsets.size()), + static_cast(layout->GetDynamicBufferCount()), layout); for (BindingIndex i{0}; i < dynamicOffsets.size(); ++i) { const BindingInfo& bindingInfo = layout->GetBindingInfo(i); @@ -150,9 +152,9 @@ namespace dawn_native { UNREACHABLE(); } - if (!IsAligned(dynamicOffsets[i], requiredAlignment)) { - return DAWN_VALIDATION_ERROR("Dynamic Buffer Offset need to be aligned"); - } + DAWN_INVALID_IF(!IsAligned(dynamicOffsets[i], requiredAlignment), + "Dynamic Offset[%u] (%u) is not %u byte aligned.", + static_cast(i), dynamicOffsets[i], requiredAlignment); BufferBinding bufferBinding = group->GetBindingAsBufferBinding(i); @@ -163,15 +165,20 @@ namespace dawn_native { if ((dynamicOffsets[i] > bufferBinding.buffer->GetSize() - bufferBinding.offset - bufferBinding.size)) { - if ((bufferBinding.buffer->GetSize() - bufferBinding.offset) == - bufferBinding.size) { - return DAWN_VALIDATION_ERROR( - "Dynamic offset out of bounds. The binding goes to the end of the " - "buffer even with a dynamic offset of 0. Did you forget to specify " - "the binding's size?"); - } else { - return DAWN_VALIDATION_ERROR("Dynamic offset out of bounds"); - } + DAWN_INVALID_IF( + (bufferBinding.buffer->GetSize() - bufferBinding.offset) == bufferBinding.size, + "Dynamic Offset[%u] (%u) is out of bounds of %s with a size of %u and a bound " + "range of (offset: %u, size: %u). The binding goes to the end of the buffer " + "even with a dynamic offset of 0. Did you forget to specify " + "the binding's size?", + static_cast(i), dynamicOffsets[i], bufferBinding.buffer, + bufferBinding.buffer->GetSize(), bufferBinding.offset, bufferBinding.size); + + 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(i), dynamicOffsets[i], bufferBinding.buffer, + bufferBinding.buffer->GetSize(), bufferBinding.offset, bufferBinding.size); } } diff --git a/src/dawn_native/RenderBundleEncoder.cpp b/src/dawn_native/RenderBundleEncoder.cpp index 19f8c64862..b21a0951ee 100644 --- a/src/dawn_native/RenderBundleEncoder.cpp +++ b/src/dawn_native/RenderBundleEncoder.cpp @@ -123,7 +123,8 @@ namespace dawn_native { RenderBundleBase* RenderBundleEncoder::APIFinish(const RenderBundleDescriptor* descriptor) { RenderBundleBase* result = nullptr; - if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result)) { + if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result, "calling Finish(%s).", + descriptor)) { return RenderBundleBase::MakeError(GetDevice()); }