From 31680a7ec051e59e7d77a8ba463e1f0e5df4e797 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Mon, 7 Feb 2022 12:02:57 +0000 Subject: [PATCH] Add validation of the max binding number. Fixed: dawn:1283 Change-Id: I5efd0d5c92bd6c1a4cdfe91079a12a9f98ddfd61 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/79260 Reviewed-by: Brandon Jones Auto-Submit: Corentin Wallez Commit-Queue: Corentin Wallez --- src/dawn/common/Constants.h | 3 +++ src/dawn/native/BindGroupLayout.cpp | 3 +++ src/dawn/native/IntegerTypes.h | 3 ++- src/dawn/native/ShaderModule.cpp | 4 ++++ .../validation/BindGroupValidationTests.cpp | 14 +++++++---- .../ShaderModuleValidationTests.cpp | 23 +++++++++++++++++++ 6 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/dawn/common/Constants.h b/src/dawn/common/Constants.h index f5a521e6d2..74853a5d28 100644 --- a/src/dawn/common/Constants.h +++ b/src/dawn/common/Constants.h @@ -62,4 +62,7 @@ static constexpr uint8_t kSampledTexturesPerExternalTexture = 3u; static constexpr uint8_t kSamplersPerExternalTexture = 1u; static constexpr uint8_t kUniformsPerExternalTexture = 1u; +// A spec defined constant but that doesn't have a name. +static constexpr uint32_t kMaxBindingNumber = 65535; + #endif // COMMON_CONSTANTS_H_ diff --git a/src/dawn/native/BindGroupLayout.cpp b/src/dawn/native/BindGroupLayout.cpp index a523239720..08c618cb8d 100644 --- a/src/dawn/native/BindGroupLayout.cpp +++ b/src/dawn/native/BindGroupLayout.cpp @@ -167,6 +167,9 @@ namespace dawn::native { const BindGroupLayoutEntry& entry = descriptor->entries[i]; BindingNumber bindingNumber = BindingNumber(entry.binding); + DAWN_INVALID_IF(bindingNumber > kMaxBindingNumberTyped, + "Binding number (%u) exceeds the maximum binding number (%u).", + uint32_t(bindingNumber), uint32_t(kMaxBindingNumberTyped)); DAWN_INVALID_IF(bindingsSet.count(bindingNumber) != 0, "On entries[%u]: binding index (%u) was specified by a previous entry.", i, entry.binding); diff --git a/src/dawn/native/IntegerTypes.h b/src/dawn/native/IntegerTypes.h index 9849d2f1b2..fd4c2f14eb 100644 --- a/src/dawn/native/IntegerTypes.h +++ b/src/dawn/native/IntegerTypes.h @@ -23,13 +23,14 @@ namespace dawn::native { // Binding numbers in the shader and BindGroup/BindGroupLayoutDescriptors using BindingNumber = TypedInteger; + constexpr BindingNumber kMaxBindingNumberTyped = BindingNumber(kMaxBindingNumber); // Binding numbers get mapped to a packed range of indices using BindingIndex = TypedInteger; using BindGroupIndex = TypedInteger; - static constexpr BindGroupIndex kMaxBindGroupsTyped = BindGroupIndex(kMaxBindGroups); + constexpr BindGroupIndex kMaxBindGroupsTyped = BindGroupIndex(kMaxBindGroups); using ColorAttachmentIndex = TypedInteger; diff --git a/src/dawn/native/ShaderModule.cpp b/src/dawn/native/ShaderModule.cpp index f419713cb1..5e343fc106 100644 --- a/src/dawn/native/ShaderModule.cpp +++ b/src/dawn/native/ShaderModule.cpp @@ -832,6 +832,10 @@ namespace dawn::native { BindingNumber bindingNumber(resource.binding); BindGroupIndex bindGroupIndex(resource.bind_group); + DAWN_INVALID_IF(bindingNumber > kMaxBindingNumberTyped, + "Binding number (%u) exceeds the maximum binding number (%u).", + uint32_t(bindingNumber), uint32_t(kMaxBindingNumberTyped)); + const auto& [binding, inserted] = metadata->bindings[bindGroupIndex].emplace( bindingNumber, ShaderBindingInfo{}); DAWN_INVALID_IF( diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp index 28b7ce46cb..e9eb46ff7f 100644 --- a/src/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp @@ -925,11 +925,15 @@ TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutStorageBindingsInVertexShad } // Tests setting that bind group layout bindings numbers may be very large. -TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutEntryNumberLarge) { - // Checks that uint32_t max is valid. - utils::MakeBindGroupLayout(device, - {{std::numeric_limits::max(), wgpu::ShaderStage::Vertex, - wgpu::BufferBindingType::Uniform}}); +TEST_F(BindGroupLayoutValidationTest, BindGroupLayoutEntryMax) { + // Check that up to kMaxBindingNumber is valid. + utils::MakeBindGroupLayout( + device, {{kMaxBindingNumber, wgpu::ShaderStage::Vertex, wgpu::BufferBindingType::Uniform}}); + + // But after is an error. + ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout( + device, + {{kMaxBindingNumber + 1, wgpu::ShaderStage::Vertex, wgpu::BufferBindingType::Uniform}})); } // This test verifies that the BindGroupLayout bindings are correctly validated, even if the diff --git a/src/tests/unittests/validation/ShaderModuleValidationTests.cpp b/src/tests/unittests/validation/ShaderModuleValidationTests.cpp index 6ddc273797..72a373b431 100644 --- a/src/tests/unittests/validation/ShaderModuleValidationTests.cpp +++ b/src/tests/unittests/validation/ShaderModuleValidationTests.cpp @@ -512,3 +512,26 @@ struct Buf { buf.data[1] = c1; })")); } + +// Test that @binding must be less then kMaxBindingNumber +TEST_F(ShaderModuleValidationTest, MaxBindingNumber) { + static_assert(kMaxBindingNumber == 65535); + + // kMaxBindingNumber is valid. + utils::CreateShaderModule(device, R"( + @group(0) @binding(65535) var s : sampler; + @stage(fragment) fn main() -> @location(0) u32 { + _ = s; + return 0u; + } + )"); + + // kMaxBindingNumber + 1 is an error + ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, R"( + @group(0) @binding(65536) var s : sampler; + @stage(fragment) fn main() -> @location(0) u32 { + _ = s; + return 0u; + } + )")); +}