From f33afcdba6cfdb303375b544f13a9fd4d391d14a Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Thu, 28 Oct 2021 00:13:17 +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: I3807c30fb0de8e766fbc35b98ef9c059f9d441ee Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67603 Commit-Queue: Brandon Jones Reviewed-by: Austin Eng --- src/dawn_native/CommandEncoder.cpp | 3 +- src/dawn_native/ComputePassEncoder.cpp | 58 +++++++++-------- src/dawn_native/EncodingContext.cpp | 30 +++++---- src/dawn_native/EncodingContext.h | 31 +++++---- src/dawn_native/ProgrammablePassEncoder.cpp | 64 +++++++++++-------- src/dawn_native/ProgrammablePassEncoder.h | 2 + src/dawn_native/RenderBundleEncoder.cpp | 3 +- .../CommandBufferValidationTests.cpp | 54 ++++++++++++++++ 8 files changed, 164 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..9122d83750 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); @@ -130,6 +130,15 @@ namespace dawn_native { mComputePassUsages.push_back(std::move(usages)); } + void EncodingContext::EnsurePassExited(const ApiObjectBase* passEncoder) { + if (mCurrentEncoder != mTopLevelEncoder && mCurrentEncoder == passEncoder) { + // The current pass encoder is being deleted. Implicitly end the pass with an error. + mCurrentEncoder = mTopLevelEncoder; + HandleError(DAWN_FORMAT_VALIDATION_ERROR( + "Command buffer recording ended before %s was ended.", passEncoder)); + } + } + const RenderPassUsages& EncodingContext::GetRenderPassUsages() const { ASSERT(!mWereRenderPassUsagesAcquired); return mRenderPassUsages; @@ -161,12 +170,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 +185,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..0235b105e3 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,14 +112,18 @@ 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(); + // Called when a pass encoder is deleted. Provides an opportunity to clean up if it's the + // mCurrentEncoder. + void EnsurePassExited(const ApiObjectBase* passEncoder); + const RenderPassUsages& GetRenderPassUsages() const; const ComputePassUsages& GetComputePassUsages() const; RenderPassUsages AcquireRenderPassUsages(); @@ -138,12 +143,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..84356bcb3c 100644 --- a/src/dawn_native/ProgrammablePassEncoder.cpp +++ b/src/dawn_native/ProgrammablePassEncoder.cpp @@ -43,14 +43,21 @@ namespace dawn_native { mValidationEnabled(device->IsValidationEnabled()) { } + void ProgrammablePassEncoder::DeleteThis() { + // This must be called prior to the destructor because it may generate an error message + // which calls the virtual RenderPassEncoder->GetType() as part of it's formatting. + mEncodingContext->EnsurePassExited(this); + ApiObjectBase::DeleteThis(); + } + 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."); - } + DAWN_INVALID_IF(mDebugGroupStackSize != 0, + "PushDebugGroup called %u time(s) without a corresponding PopDebugGroup.", + mDebugGroupStackSize); return {}; } @@ -75,10 +82,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 +121,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 +159,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 +172,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/ProgrammablePassEncoder.h b/src/dawn_native/ProgrammablePassEncoder.h index f58a9090ac..9de57a18fa 100644 --- a/src/dawn_native/ProgrammablePassEncoder.h +++ b/src/dawn_native/ProgrammablePassEncoder.h @@ -40,6 +40,8 @@ namespace dawn_native { bool IsValidationEnabled() const; MaybeError ValidateProgrammableEncoderEnd() const; + virtual void DeleteThis() override; + // Compute and render passes do different things on SetBindGroup. These are helper functions // for the logic they have in common. MaybeError ValidateSetBindGroup(BindGroupIndex index, 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()); } diff --git a/src/tests/unittests/validation/CommandBufferValidationTests.cpp b/src/tests/unittests/validation/CommandBufferValidationTests.cpp index bf97c7cbe1..b6b5288b1b 100644 --- a/src/tests/unittests/validation/CommandBufferValidationTests.cpp +++ b/src/tests/unittests/validation/CommandBufferValidationTests.cpp @@ -206,6 +206,60 @@ TEST_F(CommandBufferValidationTest, CallsAfterAFailedFinish) { ASSERT_DEVICE_ERROR(encoder.CopyBufferToBuffer(copyBuffer, 0, copyBuffer, 0, 0)); } +// Test that passes which are de-referenced prior to ending still allow the correct errors to be +// produced. +TEST_F(CommandBufferValidationTest, PassDereferenced) { + DummyRenderPass dummyRenderPass(device); + + // Control case, command buffer ended after the pass is ended. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&dummyRenderPass); + pass.EndPass(); + encoder.Finish(); + } + + // Error case, no reference is kept to a render pass. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.BeginRenderPass(&dummyRenderPass); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + // Error case, no reference is kept to a compute pass. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.BeginComputePass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + // Error case, beginning a new pass after failing to end a de-referenced pass. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.BeginRenderPass(&dummyRenderPass); + wgpu::ComputePassEncoder pass = encoder.BeginComputePass(); + pass.EndPass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + // Error case, deleting the pass after finishing the commend encoder shouldn't generate an + // uncaptured error. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::ComputePassEncoder pass = encoder.BeginComputePass(); + ASSERT_DEVICE_ERROR(encoder.Finish()); + + pass = nullptr; + } + + // Valid case, command encoder is never finished so the de-referenced pass shouldn't generate an + // uncaptured error. + { + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.BeginComputePass(); + } +} + // Test that calling inject validation error produces an error. TEST_F(CommandBufferValidationTest, InjectValidationError) { wgpu::CommandEncoder encoder = device.CreateCommandEncoder();