From 538f795e6cb147db01da89594835bb17e3fea2d4 Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Tue, 19 Oct 2021 16:14:51 +0000 Subject: [PATCH] Improve errors in BindGroupLayout, BindingInfo Updates all validation messages in BindGroupLayout.cpp and BindingInfo.cpp to give them better contextual information. Bug: dawn:563 Change-Id: I7166dce65c93d7c8ac4dd72555fff34c9202e041 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/66841 Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Commit-Queue: Brandon Jones --- src/dawn_native/BindGroupLayout.cpp | 106 +++++++++-------- src/dawn_native/BindingInfo.cpp | 169 +++++++++++++++++----------- src/dawn_native/BindingInfo.h | 5 + src/dawn_native/Device.cpp | 4 +- src/dawn_native/PerStage.cpp | 20 ++++ src/dawn_native/PerStage.h | 6 + src/dawn_native/Pipeline.cpp | 20 ---- src/dawn_native/PipelineLayout.cpp | 3 +- 8 files changed, 199 insertions(+), 134 deletions(-) diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp index f04769237b..c0097ccd0d 100644 --- a/src/dawn_native/BindGroupLayout.cpp +++ b/src/dawn_native/BindGroupLayout.cpp @@ -37,9 +37,9 @@ namespace dawn_native { DAWN_TRY_ASSIGN(format, device->GetInternalFormat(storageTextureFormat)); ASSERT(format != nullptr); - if (!format->supportsStorageUsage) { - return DAWN_VALIDATION_ERROR("Texture format does not support storage textures"); - } + DAWN_INVALID_IF(!format->supportsStorageUsage, + "Texture format (%s) does not support storage textures.", + storageTextureFormat); return {}; } @@ -48,8 +48,8 @@ namespace dawn_native { switch (dimension) { case wgpu::TextureViewDimension::Cube: case wgpu::TextureViewDimension::CubeArray: - return DAWN_VALIDATION_ERROR( - "Cube map and cube map texture views cannot be used as storage textures"); + return DAWN_FORMAT_VALIDATION_ERROR( + "%s texture views cannot be used as storage textures.", dimension); case wgpu::TextureViewDimension::e1D: case wgpu::TextureViewDimension::e2D: @@ -62,40 +62,25 @@ namespace dawn_native { } UNREACHABLE(); } - } // anonymous namespace - - MaybeError ValidateBindGroupLayoutDescriptor(DeviceBase* device, - const BindGroupLayoutDescriptor* descriptor, - bool allowInternalBinding) { - if (descriptor->nextInChain != nullptr) { - return DAWN_VALIDATION_ERROR("nextInChain must be nullptr"); - } - - std::set bindingsSet; - BindingCounts bindingCounts = {}; - for (uint32_t i = 0; i < descriptor->entryCount; ++i) { - const BindGroupLayoutEntry& entry = descriptor->entries[i]; - BindingNumber bindingNumber = BindingNumber(entry.binding); - - if (bindingsSet.count(bindingNumber) != 0) { - return DAWN_VALIDATION_ERROR("some binding index was specified more than once"); - } + MaybeError ValidateBindGroupLayoutEntry(DeviceBase* device, + const BindGroupLayoutEntry& entry, + bool allowInternalBinding) { DAWN_TRY(ValidateShaderStage(entry.visibility)); int bindingMemberCount = 0; + BindingInfoType bindingType; wgpu::ShaderStage allowedStages = kAllStages; if (entry.buffer.type != wgpu::BufferBindingType::Undefined) { bindingMemberCount++; + bindingType = BindingInfoType::Buffer; const BufferBindingLayout& buffer = entry.buffer; // The kInternalStorageBufferBinding is used internally and not a value // in wgpu::BufferBindingType. if (buffer.type == kInternalStorageBufferBinding) { - if (!allowInternalBinding) { - return DAWN_VALIDATION_ERROR("Internal binding types are disallowed"); - } + DAWN_INVALID_IF(!allowInternalBinding, "Internal binding types are disallowed"); } else { DAWN_TRY(ValidateBufferBindingType(buffer.type)); } @@ -107,22 +92,23 @@ namespace dawn_native { // Dynamic storage buffers aren't bounds checked properly in D3D12. Disallow them as // unsafe until the bounds checks are implemented. - if (device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs) && - buffer.hasDynamicOffset && - (buffer.type == wgpu::BufferBindingType::Storage || - buffer.type == kInternalStorageBufferBinding || - buffer.type == wgpu::BufferBindingType::ReadOnlyStorage)) { - return DAWN_VALIDATION_ERROR( - "Dynamic storage buffers are disallowed because they aren't secure yet. " - "See https://crbug.com/dawn/429"); - } + DAWN_INVALID_IF( + device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs) && + buffer.hasDynamicOffset && + (buffer.type == wgpu::BufferBindingType::Storage || + buffer.type == kInternalStorageBufferBinding || + buffer.type == wgpu::BufferBindingType::ReadOnlyStorage), + "Dynamic storage buffers are disallowed because they aren't secure yet. " + "See https://crbug.com/dawn/429"); } if (entry.sampler.type != wgpu::SamplerBindingType::Undefined) { bindingMemberCount++; + bindingType = BindingInfoType::Sampler; DAWN_TRY(ValidateSamplerBindingType(entry.sampler.type)); } if (entry.texture.sampleType != wgpu::TextureSampleType::Undefined) { bindingMemberCount++; + bindingType = BindingInfoType::Texture; const TextureBindingLayout& texture = entry.texture; DAWN_TRY(ValidateTextureSampleType(texture.sampleType)); @@ -133,12 +119,14 @@ namespace dawn_native { viewDimension = texture.viewDimension; } - if (texture.multisampled && viewDimension != wgpu::TextureViewDimension::e2D) { - return DAWN_VALIDATION_ERROR("Multisampled texture bindings must be 2D."); - } + DAWN_INVALID_IF( + texture.multisampled && viewDimension != wgpu::TextureViewDimension::e2D, + "View dimension (%s) for a multisampled texture bindings was not %s.", + viewDimension, wgpu::TextureViewDimension::e2D); } if (entry.storageTexture.access != wgpu::StorageTextureAccess::Undefined) { bindingMemberCount++; + bindingType = BindingInfoType::StorageTexture; const StorageTextureBindingLayout& storageTexture = entry.storageTexture; DAWN_TRY(ValidateStorageTextureAccess(storageTexture.access)); DAWN_TRY(ValidateStorageTextureFormat(device, storageTexture.format)); @@ -158,24 +146,48 @@ namespace dawn_native { FindInChain(entry.nextInChain, &externalTextureBindingLayout); if (externalTextureBindingLayout != nullptr) { bindingMemberCount++; + bindingType = BindingInfoType::ExternalTexture; } - if (bindingMemberCount != 1) { - return DAWN_VALIDATION_ERROR( - "Exactly one of buffer, sampler, texture, storageTexture, or externalTexture " - "must be set for each BindGroupLayoutEntry"); - } + DAWN_INVALID_IF(bindingMemberCount != 1, + "BindGroupLayoutEntry had more than one of buffer, sampler, texture, " + "storageTexture, or externalTexture set"); - if (!IsSubset(entry.visibility, allowedStages)) { - return DAWN_VALIDATION_ERROR("Binding type cannot be used with this visibility."); - } + DAWN_INVALID_IF( + !IsSubset(entry.visibility, allowedStages), + "%s bindings cannot be used with a visibility of %s. Only %s are allowed.", + bindingType, entry.visibility, allowedStages); + + return {}; + } + + } // anonymous namespace + + MaybeError ValidateBindGroupLayoutDescriptor(DeviceBase* device, + const BindGroupLayoutDescriptor* descriptor, + bool allowInternalBinding) { + DAWN_INVALID_IF(descriptor->nextInChain != nullptr, "nextInChain must be nullptr"); + + std::set bindingsSet; + BindingCounts bindingCounts = {}; + + for (uint32_t i = 0; i < descriptor->entryCount; ++i) { + const BindGroupLayoutEntry& entry = descriptor->entries[i]; + BindingNumber bindingNumber = BindingNumber(entry.binding); + + DAWN_INVALID_IF(bindingsSet.count(bindingNumber) != 0, + "On entries[%u]: binding index (%u) was specified by a previous entry.", + i, entry.binding); + + DAWN_TRY_CONTEXT(ValidateBindGroupLayoutEntry(device, entry, allowInternalBinding), + "validating entries[%u]", i); IncrementBindingCounts(&bindingCounts, entry); bindingsSet.insert(bindingNumber); } - DAWN_TRY(ValidateBindingCounts(bindingCounts)); + DAWN_TRY_CONTEXT(ValidateBindingCounts(bindingCounts), "validating binding counts"); return {}; } diff --git a/src/dawn_native/BindingInfo.cpp b/src/dawn_native/BindingInfo.cpp index 4b6516f5eb..aba5d0dde1 100644 --- a/src/dawn_native/BindingInfo.cpp +++ b/src/dawn_native/BindingInfo.cpp @@ -18,6 +18,32 @@ namespace dawn_native { + absl::FormatConvertResult AbslFormatConvert( + BindingInfoType value, + const absl::FormatConversionSpec& spec, + absl::FormatSink* s) { + switch (value) { + case BindingInfoType::Buffer: + s->Append("Buffer"); + break; + case BindingInfoType::Sampler: + s->Append("Sampler"); + break; + case BindingInfoType::Texture: + s->Append("Texture"); + break; + case BindingInfoType::StorageTexture: + s->Append("StorageTexture"); + break; + case BindingInfoType::ExternalTexture: + s->Append("ExternalTexture"); + break; + default: + UNREACHABLE(); + } + return {true}; + } + void IncrementBindingCounts(BindingCounts* bindingCounts, const BindGroupLayoutEntry& entry) { bindingCounts->totalCount += 1; @@ -96,84 +122,97 @@ namespace dawn_native { } MaybeError ValidateBindingCounts(const BindingCounts& bindingCounts) { - if (bindingCounts.dynamicUniformBufferCount > kMaxDynamicUniformBuffersPerPipelineLayout) { - return DAWN_VALIDATION_ERROR( - "The number of dynamic uniform buffers exceeds the maximum per-pipeline-layout " - "limit"); - } + DAWN_INVALID_IF( + bindingCounts.dynamicUniformBufferCount > kMaxDynamicUniformBuffersPerPipelineLayout, + "The number of dynamic uniform buffers (%u) exceeds the maximum per-pipeline-layout " + "limit (%u).", + bindingCounts.dynamicUniformBufferCount, kMaxDynamicUniformBuffersPerPipelineLayout); - if (bindingCounts.dynamicStorageBufferCount > kMaxDynamicStorageBuffersPerPipelineLayout) { - return DAWN_VALIDATION_ERROR( - "The number of dynamic storage buffers exceeds the maximum per-pipeline-layout " - "limit"); - } + DAWN_INVALID_IF( + bindingCounts.dynamicStorageBufferCount > kMaxDynamicStorageBuffersPerPipelineLayout, + "The number of dynamic storage buffers (%u) exceeds the maximum per-pipeline-layout " + "limit (%u).", + bindingCounts.dynamicStorageBufferCount, kMaxDynamicStorageBuffersPerPipelineLayout); for (SingleShaderStage stage : IterateStages(kAllStages)) { - if (bindingCounts.perStage[stage].sampledTextureCount > - kMaxSampledTexturesPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The number of sampled textures exceeds the maximum " - "per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].sampledTextureCount > + kMaxSampledTexturesPerShaderStage, + "The number of sampled textures (%u) in the %s stage exceeds the maximum " + "per-stage limit (%u).", + bindingCounts.perStage[stage].sampledTextureCount, stage, + kMaxSampledTexturesPerShaderStage); // The per-stage number of external textures is bound by the maximum sampled textures // per stage. - if (bindingCounts.perStage[stage].externalTextureCount > - kMaxSampledTexturesPerShaderStage / kSampledTexturesPerExternalTexture) { - return DAWN_VALIDATION_ERROR( - "The number of external textures exceeds the maximum " - "per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].externalTextureCount > + kMaxSampledTexturesPerShaderStage / kSampledTexturesPerExternalTexture, + "The number of external textures (%u) in the %s stage exceeds the maximum " + "per-stage limit (%u).", + bindingCounts.perStage[stage].externalTextureCount, stage, + kMaxSampledTexturesPerShaderStage / kSampledTexturesPerExternalTexture); - if (bindingCounts.perStage[stage].sampledTextureCount + - (bindingCounts.perStage[stage].externalTextureCount * - kSampledTexturesPerExternalTexture) > - kMaxSampledTexturesPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The combination of sampled textures and external textures exceeds the maximum " - "per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].sampledTextureCount + + (bindingCounts.perStage[stage].externalTextureCount * + kSampledTexturesPerExternalTexture) > + kMaxSampledTexturesPerShaderStage, + "The combination of sampled textures (%u) and external textures (%u) in the %s " + "stage exceeds the maximum per-stage limit (%u).", + bindingCounts.perStage[stage].sampledTextureCount, + bindingCounts.perStage[stage].externalTextureCount, stage, + kMaxSampledTexturesPerShaderStage); - if (bindingCounts.perStage[stage].samplerCount > kMaxSamplersPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The number of samplers exceeds the maximum per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].samplerCount > kMaxSamplersPerShaderStage, + "The number of samplers (%u) in the %s stage exceeds the maximum per-stage limit " + "(%u).", + bindingCounts.perStage[stage].samplerCount, stage, kMaxSamplersPerShaderStage); - if (bindingCounts.perStage[stage].samplerCount + - (bindingCounts.perStage[stage].externalTextureCount * - kSamplersPerExternalTexture) > - kMaxSamplersPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The combination of samplers and external textures exceeds the maximum " - "per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].samplerCount + + (bindingCounts.perStage[stage].externalTextureCount * + kSamplersPerExternalTexture) > + kMaxSamplersPerShaderStage, + "The combination of samplers (%u) and external textures (%u) in the %s stage " + "exceeds the maximum per-stage limit (%u).", + bindingCounts.perStage[stage].samplerCount, + bindingCounts.perStage[stage].externalTextureCount, stage, + kMaxSamplersPerShaderStage); - if (bindingCounts.perStage[stage].storageBufferCount > - kMaxStorageBuffersPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The number of storage buffers exceeds the maximum per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].storageBufferCount > kMaxStorageBuffersPerShaderStage, + "The number of storage buffers (%u) in the %s stage exceeds the maximum per-stage " + "limit (%u).", + bindingCounts.perStage[stage].storageBufferCount, stage, + kMaxStorageBuffersPerShaderStage); - if (bindingCounts.perStage[stage].storageTextureCount > - kMaxStorageTexturesPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The number of storage textures exceeds the maximum per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].storageTextureCount > + kMaxStorageTexturesPerShaderStage, + "The number of storage textures (%u) in the %s stage exceeds the maximum per-stage " + "limit (%u).", + bindingCounts.perStage[stage].storageTextureCount, stage, + kMaxStorageTexturesPerShaderStage); - if (bindingCounts.perStage[stage].uniformBufferCount > - kMaxUniformBuffersPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The number of uniform buffers exceeds the maximum per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].uniformBufferCount > kMaxUniformBuffersPerShaderStage, + "The number of uniform buffers (%u) in the %s stage exceeds the maximum per-stage " + "limit (%u).", + bindingCounts.perStage[stage].uniformBufferCount, stage, + kMaxUniformBuffersPerShaderStage); - if (bindingCounts.perStage[stage].uniformBufferCount + - (bindingCounts.perStage[stage].externalTextureCount * - kUniformsPerExternalTexture) > - kMaxUniformBuffersPerShaderStage) { - return DAWN_VALIDATION_ERROR( - "The combination of uniform buffers and external textures exceeds the maximum " - "per-stage limit."); - } + DAWN_INVALID_IF( + bindingCounts.perStage[stage].uniformBufferCount + + (bindingCounts.perStage[stage].externalTextureCount * + kUniformsPerExternalTexture) > + kMaxUniformBuffersPerShaderStage, + "The combination of uniform buffers (%u) and external textures (%u) in the %s " + "stage exceeds the maximum per-stage limit (%u).", + bindingCounts.perStage[stage].uniformBufferCount, + bindingCounts.perStage[stage].externalTextureCount, stage, + kMaxUniformBuffersPerShaderStage); } return {}; diff --git a/src/dawn_native/BindingInfo.h b/src/dawn_native/BindingInfo.h index 0d6cfc02ae..6662554090 100644 --- a/src/dawn_native/BindingInfo.h +++ b/src/dawn_native/BindingInfo.h @@ -50,6 +50,11 @@ namespace dawn_native { enum class BindingInfoType { Buffer, Sampler, Texture, StorageTexture, ExternalTexture }; + absl::FormatConvertResult AbslFormatConvert( + BindingInfoType value, + const absl::FormatConversionSpec& spec, + absl::FormatSink* s); + struct BindingInfo { BindingNumber binding; wgpu::ShaderStage visibility; diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 71e92e2441..0c948ded9a 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -1179,7 +1179,9 @@ namespace dawn_native { bool allowInternalBinding) { DAWN_TRY(ValidateIsAlive()); if (IsValidationEnabled()) { - DAWN_TRY(ValidateBindGroupLayoutDescriptor(this, descriptor, allowInternalBinding)); + DAWN_TRY_CONTEXT( + ValidateBindGroupLayoutDescriptor(this, descriptor, allowInternalBinding), + "validating %s", descriptor); } return GetOrCreateBindGroupLayout(descriptor); } diff --git a/src/dawn_native/PerStage.cpp b/src/dawn_native/PerStage.cpp index 198d99dbdc..469fd9fd6c 100644 --- a/src/dawn_native/PerStage.cpp +++ b/src/dawn_native/PerStage.cpp @@ -16,6 +16,26 @@ namespace dawn_native { + absl::FormatConvertResult AbslFormatConvert( + SingleShaderStage value, + const absl::FormatConversionSpec& spec, + absl::FormatSink* s) { + switch (value) { + case SingleShaderStage::Compute: + s->Append("Compute"); + break; + case SingleShaderStage::Vertex: + s->Append("Vertex"); + break; + case SingleShaderStage::Fragment: + s->Append("Fragment"); + break; + default: + UNREACHABLE(); + } + return {true}; + } + BitSetIterator IterateStages(wgpu::ShaderStage stages) { std::bitset bits(static_cast(stages)); return BitSetIterator(bits); diff --git a/src/dawn_native/PerStage.h b/src/dawn_native/PerStage.h index be9f4c459d..a67a08a36f 100644 --- a/src/dawn_native/PerStage.h +++ b/src/dawn_native/PerStage.h @@ -18,6 +18,7 @@ #include "common/Assert.h" #include "common/BitSetIterator.h" #include "common/Constants.h" +#include "dawn_native/Error.h" #include "dawn_native/dawn_platform.h" @@ -27,6 +28,11 @@ namespace dawn_native { enum class SingleShaderStage { Vertex, Fragment, Compute }; + absl::FormatConvertResult AbslFormatConvert( + SingleShaderStage value, + const absl::FormatConversionSpec& spec, + absl::FormatSink* s); + static_assert(static_cast(SingleShaderStage::Vertex) < kNumStages, ""); static_assert(static_cast(SingleShaderStage::Fragment) < kNumStages, ""); static_assert(static_cast(SingleShaderStage::Compute) < kNumStages, ""); diff --git a/src/dawn_native/Pipeline.cpp b/src/dawn_native/Pipeline.cpp index a1f24538e1..4fafe8b1e9 100644 --- a/src/dawn_native/Pipeline.cpp +++ b/src/dawn_native/Pipeline.cpp @@ -22,26 +22,6 @@ #include "dawn_native/ShaderModule.h" namespace dawn_native { - absl::FormatConvertResult AbslFormatConvert( - SingleShaderStage value, - const absl::FormatConversionSpec& spec, - absl::FormatSink* s) { - switch (value) { - case SingleShaderStage::Compute: - s->Append("Compute"); - break; - case SingleShaderStage::Vertex: - s->Append("Vertex"); - break; - case SingleShaderStage::Fragment: - s->Append("Fragment"); - break; - default: - UNREACHABLE(); - } - return {true}; - } - MaybeError ValidateProgrammableStage(DeviceBase* device, const ShaderModuleBase* module, const std::string& entryPoint, diff --git a/src/dawn_native/PipelineLayout.cpp b/src/dawn_native/PipelineLayout.cpp index 2f96eddf01..9b1b707b69 100644 --- a/src/dawn_native/PipelineLayout.cpp +++ b/src/dawn_native/PipelineLayout.cpp @@ -230,7 +230,8 @@ namespace dawn_native { desc.entryCount = entryVec.size(); if (device->IsValidationEnabled()) { - DAWN_TRY(ValidateBindGroupLayoutDescriptor(device, &desc)); + DAWN_TRY_CONTEXT(ValidateBindGroupLayoutDescriptor(device, &desc), "validating %s", + &desc); } return device->GetOrCreateBindGroupLayout(&desc, pipelineCompatibilityToken); };