From c19f27b80d97929193c3544cd75c3d6d15480bcb Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Thu, 21 Oct 2021 23:14:54 +0000 Subject: [PATCH] Revert "Improve validation errors for encoders" This reverts commit c7e6bb0d8d8f3bed3ca27ff822631c1671a4e5b2. 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 > Reviewed-by: Corentin Wallez # 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 Commit-Queue: Brandon Jones --- 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, 81 insertions(+), 88 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index c9510d0327..ab0857f62e 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/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(Command::PopDebugGroup); mDebugGroupStackSize--; diff --git a/src/dawn_native/ComputePassEncoder.cpp b/src/dawn_native/ComputePassEncoder.cpp index 1aa4845886..46277b13fc 100644 --- a/src/dawn_native/ComputePassEncoder.cpp +++ b/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()); - - 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); + DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), x)); + DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), y)); + DAWN_TRY(ValidatePerDimensionDispatchSizeLimit(GetDevice(), z)); } // 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( - GetDevice()->IsToggleEnabled(Toggle::DisallowUnsafeAPIs), - "DispatchIndirect is disallowed because it doesn't validate that the " - "dispatch size is valid yet."); + 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(indirectOffset % 4 != 0, - "Indirect offset (%u) is not a multiple of 4.", indirectOffset); + if (indirectOffset % 4 != 0) { + return DAWN_VALIDATION_ERROR("Indirect offset must be a multiple of 4"); + } - 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()); + if (indirectOffset >= indirectBuffer->GetSize() || + indirectOffset + kDispatchIndirectSize > indirectBuffer->GetSize()) { + return DAWN_VALIDATION_ERROR("Indirect offset out of bounds"); + } } // 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 7b14f5eddd..2724291234 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 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 ApiObjectBase* topLevelEncoder = mTopLevelEncoder; + const void* currentEncoder = mCurrentEncoder; + 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, - "Command buffer recording ended before %s was ended.", currentEncoder); + if (currentEncoder != topLevelEncoder) { + return DAWN_VALIDATION_ERROR("Command buffer recording ended mid-pass"); + } return {}; } diff --git a/src/dawn_native/EncodingContext.h b/src/dawn_native/EncodingContext.h index c395cf2d8f..b058e2bc61 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 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( - "Command cannot be recorded while %s is active.", mCurrentEncoder)); + HandleError(DAWN_VALIDATION_ERROR("Command cannot be recorded inside a pass")); } else { - HandleError(DAWN_FORMAT_VALIDATION_ERROR( - "Recording in an error or already ended %s.", encoder)); + HandleError(DAWN_VALIDATION_ERROR( + "Recording in an error or already ended pass encoder")); } return false; } @@ -86,7 +85,7 @@ namespace dawn_native { } template - 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 - 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); - MaybeError ExitRenderPass(const ApiObjectBase* passEncoder, + void EnterPass(const ObjectBase* 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; diff --git a/src/dawn_native/ProgrammablePassEncoder.cpp b/src/dawn_native/ProgrammablePassEncoder.cpp index 8cf1b820dd..c6e2e288ff 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp @@ -48,9 +48,9 @@ namespace dawn_native { } MaybeError ProgrammablePassEncoder::ValidateProgrammableEncoderEnd() const { - DAWN_INVALID_IF(mDebugGroupStackSize != 0, - "PushDebugGroup called %u time(s) without a corresponding PopDebugGroup.", - mDebugGroupStackSize); + if (mDebugGroupStackSize != 0) { + return DAWN_VALIDATION_ERROR("Each Push must be balanced by a corresponding Pop."); + } return {}; } @@ -75,9 +75,10 @@ namespace dawn_native { this, [&](CommandAllocator* allocator) -> MaybeError { if (IsValidationEnabled()) { - DAWN_INVALID_IF( - mDebugGroupStackSize == 0, - "PopDebugGroup called when no debug groups are currently pushed."); + if (mDebugGroupStackSize == 0) { + return DAWN_VALIDATION_ERROR( + "Pop must be balanced by a corresponding Push."); + } } allocator->Allocate(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, - "Bind group index (%u) exceeds the maximum (%u).", - static_cast(index), kMaxBindGroups); + if (index >= kMaxBindGroupsTyped) { + return DAWN_VALIDATION_ERROR("Setting bind group over the max"); + } ityp::span dynamicOffsets(dynamicOffsetsIn, BindingIndex(dynamicOffsetCountIn)); // Dynamic offsets count must match the number required by the layout perfectly. const BindGroupLayoutBase* layout = group->GetLayout(); - 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); + if (layout->GetDynamicBufferCount() != dynamicOffsets.size()) { + return DAWN_VALIDATION_ERROR("dynamicOffset count mismatch"); + } 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), - "Dynamic Offset[%u] (%u) is not %u byte aligned.", - static_cast(i), dynamicOffsets[i], requiredAlignment); + if (!IsAligned(dynamicOffsets[i], requiredAlignment)) { + return DAWN_VALIDATION_ERROR("Dynamic Buffer Offset need to be aligned"); + } 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( - (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); + 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"); + } } } diff --git a/src/dawn_native/RenderBundleEncoder.cpp b/src/dawn_native/RenderBundleEncoder.cpp index b21a0951ee..19f8c64862 100644 --- a/src/dawn_native/RenderBundleEncoder.cpp +++ b/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).", - descriptor)) { + if (GetDevice()->ConsumedError(FinishImpl(descriptor), &result)) { return RenderBundleBase::MakeError(GetDevice()); }