Fix erroneous validation logic for BindGroupLayoutEntry

https://dawn-review.googlesource.com/c/dawn/+/34921 introduced a couple
of bugs. This CL fixes them and tests to ensure that the new validation
logic is working correctly.

BUG=dawn:527

Change-Id: Ief01dfda0b97a202bf12ff6aeb0e7a3b84b4b81c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/35700
Commit-Queue: Brandon Jones <bajones@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>

dawn.json

@@ -168,7 +168,7 @@
             {"name": "binding", "type": "uint32_t"},
             {"name": "visibility", "type": "shader stage"},
 
-            {"name": "type", "type": "binding type"},
+            {"name": "type", "type": "binding type", "default": "undefined"},
             {"name": "has dynamic offset", "type": "bool", "default": "false"},
             {"name": "min buffer binding size", "type": "uint64_t", "default": "0"},
             {"name": "view dimension", "type": "texture view dimension", "default": "undefined"},

src/dawn_native/BindGroupLayout.cpp

@@ -85,19 +85,19 @@ namespace dawn_native {
                 const BufferBindingLayout& buffer = entry.buffer;
                 DAWN_TRY(ValidateBufferBindingType(buffer.type));
 
-                // TODO(dawn:527): ReadOnlyStorage should have the same limits as Storage
-                // regarding use with Vertex shaders.
                 if (buffer.type == wgpu::BufferBindingType::Storage) {
                     allowedStages &= ~wgpu::ShaderStage::Vertex;
+                }
 
-                    // Dynamic storage buffers aren't bounds checked properly in D3D12. Disallow
+                // Dynamic storage buffers aren't bounds checked properly in D3D12. Disallow them as
-                    // them as unsafe until the bounds checks are implemented.
+                // unsafe until the bounds checks are implemented.
-                    if (device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs) &&
+                if (device->IsToggleEnabled(Toggle::DisallowUnsafeAPIs) &&
-                        buffer.hasDynamicOffset) {
+                    buffer.hasDynamicOffset &&
-                        return DAWN_VALIDATION_ERROR(
+                    (buffer.type == wgpu::BufferBindingType::Storage ||
-                            "Dynamic storage buffers are disallowed because they aren't secure "
+                     buffer.type == wgpu::BufferBindingType::ReadOnlyStorage)) {
-                            "yet. See https://crbug.com/dawn/429");
+                    return DAWN_VALIDATION_ERROR(
-                    }
+                        "Dynamic storage buffers are disallowed because they aren't secure yet. "
+                        "See https://crbug.com/dawn/429");
                 }
             }
             if (entry.sampler.type != wgpu::SamplerBindingType::Undefined) {
@@ -151,7 +151,7 @@ namespace dawn_native {
                 return DAWN_VALIDATION_ERROR(
                     "Only one of buffer, sampler, texture, or storageTexture may be set for each "
                     "BindGroupLayoutEntry");
-            } else if (bindingMemberCount > 1) {
+            } else if (bindingMemberCount == 1) {
                 if (entry.type != wgpu::BindingType::Undefined) {
                     return DAWN_VALIDATION_ERROR(
                         "BindGroupLayoutEntry type must be undefined if any of buffer, sampler, "

src/tests/end2end/DeprecatedAPITests.cpp

@@ -48,6 +48,63 @@ TEST_P(DeprecationTests, SetIndexBufferWithFormat) {
     pass.EndPass();
 }
 
+// Test that BindGroupLayoutEntry cannot have a type if buffer, sampler, texture, or storageTexture
+// are defined.
+TEST_P(DeprecationTests, BindGroupLayoutEntryTypeConflict) {
+    wgpu::BindGroupLayoutEntry binding;
+    binding.binding = 0;
+    binding.visibility = wgpu::ShaderStage::Vertex;
+
+    wgpu::BindGroupLayoutDescriptor descriptor;
+    descriptor.entryCount = 1;
+    descriptor.entries = &binding;
+
+    // Succeeds with only a type.
+    // Will soon emit a deprecation warning.
+    binding.type = wgpu::BindingType::UniformBuffer;
+    device.CreateBindGroupLayout(&descriptor);
+
+    binding.type = wgpu::BindingType::Undefined;
+
+    // Succeeds with only a buffer.type.
+    binding.buffer.type = wgpu::BufferBindingType::Uniform;
+    device.CreateBindGroupLayout(&descriptor);
+    // Fails when both type and a buffer.type are specified.
+    binding.type = wgpu::BindingType::UniformBuffer;
+    ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&descriptor));
+
+    binding.buffer.type = wgpu::BufferBindingType::Undefined;
+    binding.type = wgpu::BindingType::Undefined;
+
+    // Succeeds with only a sampler.type.
+    binding.sampler.type = wgpu::SamplerBindingType::Filtering;
+    device.CreateBindGroupLayout(&descriptor);
+    // Fails when both type and a sampler.type are specified.
+    binding.type = wgpu::BindingType::Sampler;
+    ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&descriptor));
+
+    binding.sampler.type = wgpu::SamplerBindingType::Undefined;
+    binding.type = wgpu::BindingType::Undefined;
+
+    // Succeeds with only a texture.sampleType.
+    binding.texture.sampleType = wgpu::TextureSampleType::Float;
+    device.CreateBindGroupLayout(&descriptor);
+    // Fails when both type and a texture.sampleType are specified.
+    binding.type = wgpu::BindingType::SampledTexture;
+    ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&descriptor));
+
+    binding.texture.sampleType = wgpu::TextureSampleType::Undefined;
+    binding.type = wgpu::BindingType::Undefined;
+
+    // Succeeds with only a storageTexture.access.
+    binding.storageTexture.access = wgpu::StorageTextureAccess::ReadOnly;
+    binding.storageTexture.format = wgpu::TextureFormat::RGBA8Unorm;
+    device.CreateBindGroupLayout(&descriptor);
+    // Fails when both type and a storageTexture.access are specified.
+    binding.type = wgpu::BindingType::ReadonlyStorageTexture;
+    ASSERT_DEVICE_ERROR(device.CreateBindGroupLayout(&descriptor));
+}
+
 DAWN_INSTANTIATE_TEST(DeprecationTests,
                       D3D12Backend(),
                       MetalBackend(),