From 520539f8f9f1a48f067ed6fca94482ff3f183044 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Wed, 20 Oct 2021 17:42:38 +0000 Subject: [PATCH] Various validation error improvements A grab bag of validation error message improvements in the following files: - Adapter.cpp - BackendConnection.cpp - BindGroup.cpp - CommandBuffer.cpp - CommandBufferStateTracker.cpp - ComputePipeline.cpp - Device.cpp - Instance.cpp - Limits.cpp - Queue.cpp Bug: dawn:563 Change-Id: Ied9f660fc22302d3fd5af4796de32efec529ca05 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67001 Commit-Queue: Brandon Jones Reviewed-by: Austin Eng --- src/dawn_native/Adapter.cpp | 12 +++-- src/dawn_native/BackendConnection.cpp | 2 +- src/dawn_native/BindGroup.cpp | 47 +++++++++++-------- src/dawn_native/CommandBuffer.cpp | 4 +- src/dawn_native/CommandBufferStateTracker.cpp | 4 +- src/dawn_native/ComputePipeline.cpp | 2 +- src/dawn_native/Device.cpp | 14 ++---- src/dawn_native/Instance.cpp | 4 +- src/dawn_native/Limits.cpp | 21 +++++---- src/dawn_native/Queue.cpp | 22 ++++----- 10 files changed, 66 insertions(+), 66 deletions(-) diff --git a/src/dawn_native/Adapter.cpp b/src/dawn_native/Adapter.cpp index d83871b8e9..bdbfc2cdbf 100644 --- a/src/dawn_native/Adapter.cpp +++ b/src/dawn_native/Adapter.cpp @@ -144,8 +144,8 @@ namespace dawn_native { if (err.IsError()) { std::unique_ptr errorData = err.AcquireError(); - callback(WGPURequestDeviceStatus_Error, device, errorData->GetMessage().c_str(), - userdata); + callback(WGPURequestDeviceStatus_Error, device, + errorData->GetFormattedMessage().c_str(), userdata); return; } WGPURequestDeviceStatus status = @@ -166,9 +166,11 @@ namespace dawn_native { } if (descriptor != nullptr && descriptor->requiredLimits != nullptr) { - DAWN_TRY(ValidateLimits( - mUseTieredLimits ? ApplyLimitTiers(mLimits.v1) : mLimits.v1, - reinterpret_cast(descriptor->requiredLimits)->limits)); + DAWN_TRY_CONTEXT( + ValidateLimits( + mUseTieredLimits ? ApplyLimitTiers(mLimits.v1) : mLimits.v1, + reinterpret_cast(descriptor->requiredLimits)->limits), + "validating required limits"); DAWN_INVALID_IF(descriptor->requiredLimits->nextInChain != nullptr, "nextInChain is not nullptr."); diff --git a/src/dawn_native/BackendConnection.cpp b/src/dawn_native/BackendConnection.cpp index 09ef4ef76c..89379cb43d 100644 --- a/src/dawn_native/BackendConnection.cpp +++ b/src/dawn_native/BackendConnection.cpp @@ -30,7 +30,7 @@ namespace dawn_native { ResultOrError>> BackendConnection::DiscoverAdapters( const AdapterDiscoveryOptionsBase* options) { - return DAWN_VALIDATION_ERROR("DiscoverAdapters not implemented for this backend."); + return DAWN_FORMAT_VALIDATION_ERROR("DiscoverAdapters not implemented for this backend."); } } // namespace dawn_native diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 9d248ea3ca..361ca82618 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -36,10 +36,13 @@ namespace dawn_native { MaybeError ValidateBufferBinding(const DeviceBase* device, const BindGroupEntry& entry, const BindingInfo& bindingInfo) { - if (entry.buffer == nullptr || entry.sampler != nullptr || - entry.textureView != nullptr || entry.nextInChain != nullptr) { - return DAWN_VALIDATION_ERROR("Expected buffer binding"); - } + DAWN_INVALID_IF(entry.buffer == nullptr, "Binding entry buffer not set."); + + DAWN_INVALID_IF(entry.sampler != nullptr || entry.textureView != nullptr, + "Expected only buffer to be set for binding entry."); + + DAWN_INVALID_IF(entry.nextInChain != nullptr, "nextInChain must be nullptr."); + DAWN_TRY(device->ValidateObject(entry.buffer)); ASSERT(bindingInfo.bindingType == BindingInfoType::Buffer); @@ -116,10 +119,13 @@ namespace dawn_native { MaybeError ValidateTextureBinding(DeviceBase* device, const BindGroupEntry& entry, const BindingInfo& bindingInfo) { - if (entry.textureView == nullptr || entry.sampler != nullptr || - entry.buffer != nullptr || entry.nextInChain != nullptr) { - return DAWN_VALIDATION_ERROR("Expected texture binding"); - } + DAWN_INVALID_IF(entry.textureView == nullptr, "Binding entry textureView not set."); + + DAWN_INVALID_IF(entry.sampler != nullptr || entry.buffer != nullptr, + "Expected only textureView to be set for binding entry."); + + DAWN_INVALID_IF(entry.nextInChain != nullptr, "nextInChain must be nullptr."); + DAWN_TRY(device->ValidateObject(entry.textureView)); TextureViewBase* view = entry.textureView; @@ -189,10 +195,13 @@ namespace dawn_native { MaybeError ValidateSamplerBinding(const DeviceBase* device, const BindGroupEntry& entry, const BindingInfo& bindingInfo) { - if (entry.sampler == nullptr || entry.textureView != nullptr || - entry.buffer != nullptr || entry.nextInChain != nullptr) { - return DAWN_VALIDATION_ERROR("Expected sampler binding"); - } + DAWN_INVALID_IF(entry.sampler == nullptr, "Binding entry sampler not set."); + + DAWN_INVALID_IF(entry.textureView != nullptr || entry.buffer != nullptr, + "Expected only sampler to be set for binding entry."); + + DAWN_INVALID_IF(entry.nextInChain != nullptr, "nextInChain must be nullptr."); + DAWN_TRY(device->ValidateObject(entry.sampler)); ASSERT(bindingInfo.bindingType == BindingInfoType::Sampler); @@ -233,10 +242,12 @@ namespace dawn_native { const ExternalTextureBindingEntry* externalTextureBindingEntry = nullptr; FindInChain(entry.nextInChain, &externalTextureBindingEntry); - if (entry.sampler != nullptr || entry.textureView != nullptr || - entry.buffer != nullptr || externalTextureBindingEntry == nullptr) { - return DAWN_VALIDATION_ERROR("Expected external texture binding"); - } + DAWN_INVALID_IF(externalTextureBindingEntry == nullptr, + "Binding entry external texture not set."); + + DAWN_INVALID_IF( + entry.sampler != nullptr || entry.textureView != nullptr || entry.buffer != nullptr, + "Expected only external texture to be set for binding entry."); DAWN_TRY(ValidateSingleSType(externalTextureBindingEntry->nextInChain, wgpu::SType::ExternalTextureBindingEntry)); @@ -250,9 +261,7 @@ namespace dawn_native { MaybeError ValidateBindGroupDescriptor(DeviceBase* device, const BindGroupDescriptor* descriptor) { - if (descriptor->nextInChain != nullptr) { - return DAWN_VALIDATION_ERROR("nextInChain must be nullptr"); - } + DAWN_INVALID_IF(descriptor->nextInChain != nullptr, "nextInChain must be nullptr."); DAWN_TRY(device->ValidateObject(descriptor->layout)); diff --git a/src/dawn_native/CommandBuffer.cpp b/src/dawn_native/CommandBuffer.cpp index 5b07bb64a0..18fef0d952 100644 --- a/src/dawn_native/CommandBuffer.cpp +++ b/src/dawn_native/CommandBuffer.cpp @@ -59,9 +59,7 @@ namespace dawn_native { MaybeError CommandBufferBase::ValidateCanUseInSubmitNow() const { ASSERT(!IsError()); - if (mDestroyed) { - return DAWN_VALIDATION_ERROR("Command buffer reused in submit"); - } + DAWN_INVALID_IF(mDestroyed, "%s cannot be submitted more than once.", this); return {}; } diff --git a/src/dawn_native/CommandBufferStateTracker.cpp b/src/dawn_native/CommandBufferStateTracker.cpp index cff63f6eb2..5210936c59 100644 --- a/src/dawn_native/CommandBufferStateTracker.cpp +++ b/src/dawn_native/CommandBufferStateTracker.cpp @@ -246,7 +246,7 @@ namespace dawn_native { // because to have invalid aspects one of the above conditions must have failed earlier. // If this is reached, make sure lazy aspects and the error checks above are consistent. UNREACHABLE(); - return DAWN_VALIDATION_ERROR("Index buffer invalid"); + return DAWN_FORMAT_VALIDATION_ERROR("Index buffer is invalid."); } // TODO(dawn:563): Indicate which slots were not set. @@ -277,7 +277,7 @@ namespace dawn_native { // because to have invalid aspects one of the above conditions must have failed earlier. // If this is reached, make sure lazy aspects and the error checks above are consistent. UNREACHABLE(); - return DAWN_VALIDATION_ERROR("Bind groups invalid"); + return DAWN_FORMAT_VALIDATION_ERROR("Bind groups are invalid."); } DAWN_INVALID_IF(aspects[VALIDATION_ASPECT_PIPELINE], "No pipeline set."); diff --git a/src/dawn_native/ComputePipeline.cpp b/src/dawn_native/ComputePipeline.cpp index d79d4a066f..72addc4797 100644 --- a/src/dawn_native/ComputePipeline.cpp +++ b/src/dawn_native/ComputePipeline.cpp @@ -23,7 +23,7 @@ namespace dawn_native { MaybeError ValidateComputePipelineDescriptor(DeviceBase* device, const ComputePipelineDescriptor* descriptor) { if (descriptor->nextInChain != nullptr) { - return DAWN_VALIDATION_ERROR("nextInChain must be nullptr"); + return DAWN_FORMAT_VALIDATION_ERROR("nextInChain must be nullptr."); } if (descriptor->layout != nullptr) { diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index d5ae480426..84ad2d462e 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -507,10 +507,8 @@ namespace dawn_native { } MaybeError DeviceBase::ValidateIsAlive() const { - if (DAWN_LIKELY(mState == State::Alive)) { - return {}; - } - return DAWN_VALIDATION_ERROR("Device is lost"); + DAWN_INVALID_IF(mState != State::Alive, "%s is lost.", this); + return {}; } void DeviceBase::APILoseForTesting() { @@ -615,14 +613,10 @@ namespace dawn_native { ResultOrError DeviceBase::GetInternalFormat(wgpu::TextureFormat format) const { size_t index = ComputeFormatIndex(format); - if (index >= mFormatTable.size()) { - return DAWN_VALIDATION_ERROR("Unknown texture format"); - } + DAWN_INVALID_IF(index >= mFormatTable.size(), "Unknown texture format %s.", format); const Format* internalFormat = &mFormatTable[index]; - if (!internalFormat->isSupported) { - return DAWN_VALIDATION_ERROR("Unsupported texture format"); - } + DAWN_INVALID_IF(!internalFormat->isSupported, "Unsupported texture format %s.", format); return internalFormat; } diff --git a/src/dawn_native/Instance.cpp b/src/dawn_native/Instance.cpp index 936471e6c8..f2b08c7dc6 100644 --- a/src/dawn_native/Instance.cpp +++ b/src/dawn_native/Instance.cpp @@ -185,9 +185,7 @@ namespace dawn_native { } } - if (!foundBackend) { - return DAWN_VALIDATION_ERROR("Backend isn't present."); - } + DAWN_INVALID_IF(!foundBackend, "No matching backend found."); return {}; } diff --git a/src/dawn_native/Limits.cpp b/src/dawn_native/Limits.cpp index 5c55a55b25..949ce8331c 100644 --- a/src/dawn_native/Limits.cpp +++ b/src/dawn_native/Limits.cpp @@ -98,9 +98,9 @@ namespace dawn_native { template static MaybeError Validate(T supported, T required) { - if (IsBetter(required, supported)) { - return DAWN_VALIDATION_ERROR("requiredLimit lower than supported limit"); - } + DAWN_INVALID_IF(IsBetter(required, supported), + "Required limit (%u) is lower than the supported limit (%u).", + required, supported); return {}; } }; @@ -114,9 +114,9 @@ namespace dawn_native { template static MaybeError Validate(T supported, T required) { - if (IsBetter(required, supported)) { - return DAWN_VALIDATION_ERROR("requiredLimit greater than supported limit"); - } + DAWN_INVALID_IF(IsBetter(required, supported), + "Required limit (%u) is greater than the supported limit (%u).", + required, supported); return {}; } }; @@ -163,10 +163,11 @@ namespace dawn_native { } MaybeError ValidateLimits(const Limits& supportedLimits, const Limits& requiredLimits) { -#define X(Better, limitName, ...) \ - if (!IsLimitUndefined(requiredLimits.limitName)) { \ - DAWN_TRY(CheckLimit::Validate(supportedLimits.limitName, \ - requiredLimits.limitName)); \ +#define X(Better, limitName, ...) \ + if (!IsLimitUndefined(requiredLimits.limitName)) { \ + DAWN_TRY_CONTEXT(CheckLimit::Validate( \ + supportedLimits.limitName, requiredLimits.limitName), \ + "validating " #limitName); \ } LIMITS(X) #undef X diff --git a/src/dawn_native/Queue.cpp b/src/dawn_native/Queue.cpp index e6af98b202..644ce17548 100644 --- a/src/dawn_native/Queue.cpp +++ b/src/dawn_native/Queue.cpp @@ -434,9 +434,7 @@ namespace dawn_native { *status = WGPUQueueWorkDoneStatus_Error; DAWN_TRY(GetDevice()->ValidateObject(this)); - if (signalValue != 0) { - return DAWN_VALIDATION_ERROR("SignalValue must currently be 0."); - } + DAWN_INVALID_IF(signalValue != 0, "SignalValue (%u) is not 0.", signalValue); return {}; } @@ -451,17 +449,17 @@ namespace dawn_native { DAWN_TRY(ValidateImageCopyTexture(GetDevice(), *destination, *writeSize)); - if (dataLayout.offset > dataSize) { - return DAWN_VALIDATION_ERROR("Queue::WriteTexture out of range"); - } + DAWN_INVALID_IF(dataLayout.offset > dataSize, + "Data offset (%u) is greater than the data size (%u).", dataLayout.offset, + dataSize); - if (!(destination->texture->GetUsage() & wgpu::TextureUsage::CopyDst)) { - return DAWN_VALIDATION_ERROR("Texture needs the CopyDst usage bit"); - } + DAWN_INVALID_IF(!(destination->texture->GetUsage() & wgpu::TextureUsage::CopyDst), + "Usage (%s) of %s does not include %s.", destination->texture->GetUsage(), + destination->texture, wgpu::TextureUsage::CopyDst); - if (destination->texture->GetSampleCount() > 1) { - return DAWN_VALIDATION_ERROR("The sample count of textures must be 1"); - } + DAWN_INVALID_IF(destination->texture->GetSampleCount() > 1, + "Sample count (%u) of %s is not 1", destination->texture->GetSampleCount(), + destination->texture); DAWN_TRY(ValidateLinearToDepthStencilCopyRestrictions(*destination)); // We validate texture copy range before validating linear texture data,