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 <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>
diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp
index f047692..c0097cc 100644
--- a/src/dawn_native/BindGroupLayout.cpp
+++ b/src/dawn_native/BindGroupLayout.cpp
@@ -37,9 +37,9 @@
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 @@
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 @@
}
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<BindingNumber> 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 @@
// 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 @@
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 @@
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<BindingNumber> 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 4b6516f..aba5d0d 100644
--- a/src/dawn_native/BindingInfo.cpp
+++ b/src/dawn_native/BindingInfo.cpp
@@ -18,6 +18,32 @@
namespace dawn_native {
+ absl::FormatConvertResult<absl::FormatConversionCharSet::kString> 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 @@
}
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 0d6cfc0..6662554 100644
--- a/src/dawn_native/BindingInfo.h
+++ b/src/dawn_native/BindingInfo.h
@@ -50,6 +50,11 @@
enum class BindingInfoType { Buffer, Sampler, Texture, StorageTexture, ExternalTexture };
+ absl::FormatConvertResult<absl::FormatConversionCharSet::kString> 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 71e92e2..0c948de 100644
--- a/src/dawn_native/Device.cpp
+++ b/src/dawn_native/Device.cpp
@@ -1179,7 +1179,9 @@
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 198d99d..469fd9f 100644
--- a/src/dawn_native/PerStage.cpp
+++ b/src/dawn_native/PerStage.cpp
@@ -16,6 +16,26 @@
namespace dawn_native {
+ absl::FormatConvertResult<absl::FormatConversionCharSet::kString> 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<kNumStages, SingleShaderStage> IterateStages(wgpu::ShaderStage stages) {
std::bitset<kNumStages> bits(static_cast<uint32_t>(stages));
return BitSetIterator<kNumStages, SingleShaderStage>(bits);
diff --git a/src/dawn_native/PerStage.h b/src/dawn_native/PerStage.h
index be9f4c4..a67a08a 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 @@
enum class SingleShaderStage { Vertex, Fragment, Compute };
+ absl::FormatConvertResult<absl::FormatConversionCharSet::kString> AbslFormatConvert(
+ SingleShaderStage value,
+ const absl::FormatConversionSpec& spec,
+ absl::FormatSink* s);
+
static_assert(static_cast<uint32_t>(SingleShaderStage::Vertex) < kNumStages, "");
static_assert(static_cast<uint32_t>(SingleShaderStage::Fragment) < kNumStages, "");
static_assert(static_cast<uint32_t>(SingleShaderStage::Compute) < kNumStages, "");
diff --git a/src/dawn_native/Pipeline.cpp b/src/dawn_native/Pipeline.cpp
index a1f2453..4fafe8b 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<absl::FormatConversionCharSet::kString> 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 2f96edd..9b1b707 100644
--- a/src/dawn_native/PipelineLayout.cpp
+++ b/src/dawn_native/PipelineLayout.cpp
@@ -230,7 +230,8 @@
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);
};
