Improve runtime minBindingSize validation message

The message now reports the group/number of the binding that failed
validation, as well as the buffer.

Bug: dawn:1604
Change-Id: Ib08a3eace5ec5ebaf9eecce01eb397b5c0681bd1
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/131843
Reviewed-by: Brandon Jones <bajones@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn/native/BindGroup.cpp

@@ -253,6 +253,16 @@ MaybeError ValidateExternalTextureBinding(
     return {};
 }
 
+template <typename F>
+void ForEachUnverifiedBufferBindingIndexImpl(const BindGroupLayoutBase* bgl, F&& f) {
+    uint32_t packedIndex = 0;
+    for (BindingIndex bindingIndex{0}; bindingIndex < bgl->GetBufferCount(); ++bindingIndex) {
+        if (bgl->GetBindingInfo(bindingIndex).buffer.minBindingSize == 0) {
+            f(bindingIndex, packedIndex++);
+        }
+    }
+}
+
 }  // anonymous namespace
 
 MaybeError ValidateBindGroupDescriptor(DeviceBase* device,
@@ -351,7 +361,7 @@ MaybeError ValidateBindGroupDescriptor(DeviceBase* device,
     ASSERT(bindingsSet.count() == descriptor->layout->GetUnexpandedBindingCount());
 
     return {};
-}  // anonymous namespace
+}
 
 // BindGroup
 
@@ -443,15 +453,11 @@ BindGroupBase::BindGroupBase(DeviceBase* device,
         }
     }
 
-    uint32_t packedIdx = 0;
+    ForEachUnverifiedBufferBindingIndexImpl(mLayout.Get(),
-    for (BindingIndex bindingIndex{0}; bindingIndex < descriptor->layout->GetBufferCount();
+                                            [&](BindingIndex bindingIndex, uint32_t packedIndex) {
-         ++bindingIndex) {
+                                                mBindingData.unverifiedBufferSizes[packedIndex] =
-        if (descriptor->layout->GetBindingInfo(bindingIndex).buffer.minBindingSize == 0) {
+                                                    mBindingData.bufferData[bindingIndex].size;
-            mBindingData.unverifiedBufferSizes[packedIdx] =
+                                            });
-                mBindingData.bufferData[bindingIndex].size;
-            ++packedIdx;
-        }
-    }
 
     GetObjectTrackingList()->Track(this);
 }
@@ -530,4 +536,9 @@ const std::vector<Ref<ExternalTextureBase>>& BindGroupBase::GetBoundExternalText
     return mBoundExternalTextures;
 }
 
+void BindGroupBase::ForEachUnverifiedBufferBindingIndex(
+    std::function<void(BindingIndex, uint32_t)> fn) const {
+    ForEachUnverifiedBufferBindingIndexImpl(mLayout.Get(), fn);
+}
+
 }  // namespace dawn::native

src/dawn/native/BindGroup.h

@@ -56,6 +56,8 @@ class BindGroupBase : public ApiObjectBase {
     const ityp::span<uint32_t, uint64_t>& GetUnverifiedBufferSizes() const;
     const std::vector<Ref<ExternalTextureBase>>& GetBoundExternalTextures() const;
 
+    void ForEachUnverifiedBufferBindingIndex(std::function<void(BindingIndex, uint32_t)> fn) const;
+
   protected:
     // To save memory, the size of a bind group is dynamically determined and the bind group is
     // placement-allocated into memory big enough to hold the bind group with its

src/dawn/native/CommandBufferStateTracker.cpp

@@ -14,6 +14,7 @@
 
 #include "dawn/native/CommandBufferStateTracker.h"
 
+#include <limits>
 #include <optional>
 #include <type_traits>
 #include <utility>
@@ -573,22 +574,40 @@ MaybeError CommandBufferStateTracker::CheckMissingAspects(ValidationAspects aspe
                 requiredBGL, mLastPipelineLayout, currentBGL, mBindgroups[i],
                 static_cast<uint32_t>(i));
 
-            // TODO(dawn:563): Report which buffer bindings are failing. This requires the ability
-            // to look up the binding index from the packed index.
             std::optional<uint32_t> packedIndex = FindFirstUndersizedBuffer(
                 mBindgroups[i]->GetUnverifiedBufferSizes(), (*mMinBufferSizes)[i]);
             if (packedIndex.has_value()) {
+                // Find the binding index for this packed index.
+                BindingIndex bindingIndex{std::numeric_limits<uint32_t>::max()};
+                mBindgroups[i]->ForEachUnverifiedBufferBindingIndex(
+                    [&](BindingIndex candidateBindingIndex, uint32_t candidatePackedIndex) {
+                        if (candidatePackedIndex == *packedIndex) {
+                            bindingIndex = candidateBindingIndex;
+                        }
+                    });
+                ASSERT(static_cast<uint32_t>(bindingIndex) != std::numeric_limits<uint32_t>::max());
+
+                const auto& bindingInfo = mBindgroups[i]->GetLayout()->GetBindingInfo(bindingIndex);
+                const BufferBinding& bufferBinding =
+                    mBindgroups[i]->GetBindingAsBufferBinding(bindingIndex);
+
+                BindingNumber bindingNumber = bindingInfo.binding;
+                const BufferBase* buffer = bufferBinding.buffer;
+
                 uint64_t bufferSize =
                     mBindgroups[i]->GetUnverifiedBufferSizes()[packedIndex.value()];
                 uint64_t minBufferSize = (*mMinBufferSizes)[i][packedIndex.value()];
+
                 return DAWN_VALIDATION_ERROR(
-                    "Binding sizes are too small for %s set at group index %u. A bound buffer "
+                    "%s bound with size %u at group %u, binding %u is too small. The pipeline (%s) "
-                    "contained %u bytes, but the current pipeline (%s) requires a buffer which is "
+                    "requires a buffer binding which is at least %u bytes.%s",
-                    "at least %u bytes. (Note that uniform buffer bindings must be a multiple of "
+                    buffer, bufferSize, static_cast<uint32_t>(i),
-                    "16 bytes, and as a result may be larger than the associated data in the "
+                    static_cast<uint32_t>(bindingNumber), mLastPipeline, minBufferSize,
-                    "shader source.)",
+                    (bindingInfo.buffer.type == wgpu::BufferBindingType::Uniform
-                    mBindgroups[i], static_cast<uint32_t>(i), bufferSize, mLastPipeline,
+                         ? " This binding is a uniform buffer binding. It is padded to a multiple "
-                    minBufferSize);
+                           "of 16 bytes, and as a result may be larger than the associated data in "
+                           "the shader source."
+                         : ""));
             }
         }