commit 532be50b761b660e2e5e354bbffb5b020df9afef
author Jiawei Shao <jiawei.shao@intel.com> Mon Mar 09 00:45:18 2020 +0000
committer Commit Bot service account <commit-bot@chromium.org> Mon Mar 09 00:45:18 2020 +0000
tree beba8b0a7b01eb4099613235a4f534a92cf16b48
parent 7d20b4450182b8e25dfd315019623d7a746cc290

Check bindingType with shader stage when creating default pipeline layout

This patch intends to fix a crash issue when creating a rendering
pipeline with storage buffer declared in vertex shader and pipeline
layout is not set.

Without this patch, in PipelineLayoutBase::CreateDefault() the
bindingSlot.visibility is always set to Fragment and Compute when it is
a storage buffer, therefore a crash happens at the failure of the
assertion modules->IsCompatibleWithPipelineLayout() when the storage
buffer is actually declared in the vertex shader.

BUG=dawn:276
TEST=dawn_unittests

Change-Id: I56876a97d53ead5ed226dc1b9bbed1a77156b2b2
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/16564
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Jiawei Shao <jiawei.shao@intel.com>

src/dawn_native/BindGroupLayout.cpp

diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp
index 0ab3f1c..d16330a 100644
--- a/src/dawn_native/BindGroupLayout.cpp
+++ b/src/dawn_native/BindGroupLayout.cpp
@@ -23,6 +23,18 @@
 
 namespace dawn_native {
 
+    MaybeError ValidateBindingTypeWithShaderStageVisibility(
+        wgpu::BindingType bindingType,
+        wgpu::ShaderStage shaderStageVisibility) {
+        if (bindingType == wgpu::BindingType::StorageBuffer &&
+            (shaderStageVisibility & wgpu::ShaderStage::Vertex) != 0) {
+            return DAWN_VALIDATION_ERROR(
+                "storage buffer binding is not supported in vertex shader");
+        }
+
+        return {};
+    }
+
     MaybeError ValidateBindGroupLayoutDescriptor(DeviceBase*,
                                                  const BindGroupLayoutDescriptor* descriptor) {
         if (descriptor->nextInChain != nullptr) {
@@ -49,11 +61,8 @@
                 return DAWN_VALIDATION_ERROR("some binding index was specified more than once");
             }
 
-            if (binding.type == wgpu::BindingType::StorageBuffer &&
-                (binding.visibility & wgpu::ShaderStage::Vertex) != 0) {
-                return DAWN_VALIDATION_ERROR(
-                    "storage buffer binding is not supported in vertex shader");
-            }
+            DAWN_TRY(
+                ValidateBindingTypeWithShaderStageVisibility(binding.type, binding.visibility));
 
             switch (binding.type) {
                 case wgpu::BindingType::UniformBuffer:

src/dawn_native/BindGroupLayout.h

diff --git a/src/dawn_native/BindGroupLayout.h b/src/dawn_native/BindGroupLayout.h
index 4c0dd7a..02d8453 100644
--- a/src/dawn_native/BindGroupLayout.h
+++ b/src/dawn_native/BindGroupLayout.h
@@ -30,6 +30,10 @@
     MaybeError ValidateBindGroupLayoutDescriptor(DeviceBase*,
                                                  const BindGroupLayoutDescriptor* descriptor);
 
+    MaybeError ValidateBindingTypeWithShaderStageVisibility(
+        wgpu::BindingType bindingType,
+        wgpu::ShaderStage shaderStageVisibility);
+
     class BindGroupLayoutBase : public CachedObject {
       public:
         BindGroupLayoutBase(DeviceBase* device, const BindGroupLayoutDescriptor* descriptor);

src/dawn_native/PipelineLayout.cpp

diff --git a/src/dawn_native/PipelineLayout.cpp b/src/dawn_native/PipelineLayout.cpp
index a810f37..5cdf781 100644
--- a/src/dawn_native/PipelineLayout.cpp
+++ b/src/dawn_native/PipelineLayout.cpp
@@ -132,6 +132,9 @@
 
                     BindGroupLayoutBinding bindingSlot;
                     bindingSlot.binding = binding;
+
+                    DAWN_TRY(ValidateBindingTypeWithShaderStageVisibility(
+                        bindingInfo.type, StageBit(module->GetExecutionModel())));
                     if (bindingInfo.type == wgpu::BindingType::StorageBuffer) {
                         bindingSlot.visibility =
                             wgpu::ShaderStage::Fragment | wgpu::ShaderStage::Compute;

src/tests/unittests/validation/RenderPipelineValidationTests.cpp

diff --git a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp
index f8b3d42..9ffbbc9 100644
--- a/src/tests/unittests/validation/RenderPipelineValidationTests.cpp
+++ b/src/tests/unittests/validation/RenderPipelineValidationTests.cpp
@@ -456,3 +456,24 @@
         }
     }
 }
+
+// Test that declaring a storage buffer in the vertex shader without setting pipeline layout won't
+// cause crash.
+TEST_F(RenderPipelineValidationTest, StorageBufferInVertexShaderNoLayout) {
+    wgpu::ShaderModule vsModuleWithStorageBuffer =
+        utils::CreateShaderModule(device, utils::SingleShaderStage::Vertex, R"(
+        #version 450
+        #define kNumValues 100
+        layout(std430, set = 0, binding = 0) buffer Dst { uint dst[kNumValues]; };
+        void main() {
+            uint index = gl_VertexIndex;
+            dst[index] = 0x1234;
+            gl_Position = vec4(1.f, 0.f, 0.f, 1.f);
+        })");
+
+    utils::ComboRenderPipelineDescriptor descriptor(device);
+    descriptor.layout = nullptr;
+    descriptor.vertexStage.module = vsModuleWithStorageBuffer;
+    descriptor.cFragmentStage.module = fsModule;
+    ASSERT_DEVICE_ERROR(device.CreateRenderPipeline(&descriptor));
+}