From 613eee30c3bf47f0033d4969df601d47320bf26a Mon Sep 17 00:00:00 2001 From: Kai Ninomiya Date: Thu, 8 Jun 2017 18:25:14 -0700 Subject: [PATCH] Add validation for some null arguments and dangling render passes And add tests for both --- src/backend/common/CommandBuffer.cpp | 13 ++++ .../common/CommandBufferStateTracker.cpp | 8 +++ .../common/CommandBufferStateTracker.h | 1 + src/backend/common/Framebuffer.cpp | 5 ++ src/tests/CMakeLists.txt | 3 + .../CommandBufferValidationTests.cpp | 65 ++++++++++++++++++ .../validation/FramebufferValidationTests.cpp | 66 +++++++++++++++++++ .../validation/RenderPassValidationTests.cpp | 42 ++++++++++++ 8 files changed, 203 insertions(+) create mode 100644 src/tests/unittests/validation/CommandBufferValidationTests.cpp create mode 100644 src/tests/unittests/validation/FramebufferValidationTests.cpp create mode 100644 src/tests/unittests/validation/RenderPassValidationTests.cpp diff --git a/src/backend/common/CommandBuffer.cpp b/src/backend/common/CommandBuffer.cpp index df9940a0f5..925f8bb330 100644 --- a/src/backend/common/CommandBuffer.cpp +++ b/src/backend/common/CommandBuffer.cpp @@ -186,6 +186,15 @@ namespace backend { BeginRenderPassCmd* cmd = iterator.NextCommand(); auto* renderPass = cmd->renderPass.Get(); auto* framebuffer = cmd->framebuffer.Get(); + // TODO(kainino@chromium.org): null checks should not be necessary + if (renderPass == nullptr) { + HandleError("Render pass is invalid"); + return false; + } + if (framebuffer == nullptr) { + HandleError("Framebuffer is invalid"); + return false; + } if (!state->BeginRenderPass(renderPass, framebuffer)) { return false; } @@ -353,6 +362,10 @@ namespace backend { } } + if (!state->ValidateEndCommandBuffer()) { + return false; + } + return true; } diff --git a/src/backend/common/CommandBufferStateTracker.cpp b/src/backend/common/CommandBufferStateTracker.cpp index 7c02be9eb6..74ff8bd869 100644 --- a/src/backend/common/CommandBufferStateTracker.cpp +++ b/src/backend/common/CommandBufferStateTracker.cpp @@ -113,6 +113,14 @@ namespace backend { return RevalidateCanDraw(); } + bool CommandBufferStateTracker::ValidateEndCommandBuffer() const { + if (currentRenderPass != nullptr) { + builder->HandleError("Can't end command buffer with an active render pass"); + return false; + } + return true; + } + bool CommandBufferStateTracker::BeginSubpass() { if (currentRenderPass == nullptr) { builder->HandleError("Can't begin a subpass without an active render pass"); diff --git a/src/backend/common/CommandBufferStateTracker.h b/src/backend/common/CommandBufferStateTracker.h index 7fe4a23592..04f12b5959 100644 --- a/src/backend/common/CommandBufferStateTracker.h +++ b/src/backend/common/CommandBufferStateTracker.h @@ -34,6 +34,7 @@ namespace backend { bool ValidateCanDispatch(); bool ValidateCanDrawArrays(); bool ValidateCanDrawElements(); + bool ValidateEndCommandBuffer() const; // State-modifying methods bool BeginSubpass(); diff --git a/src/backend/common/Framebuffer.cpp b/src/backend/common/Framebuffer.cpp index 53b5a7b4a1..e6a24d375c 100644 --- a/src/backend/common/Framebuffer.cpp +++ b/src/backend/common/Framebuffer.cpp @@ -85,6 +85,11 @@ namespace backend { HandleError("Framebuffer render pass property set multiple times"); return; } + // TODO(kainino@chromium.org): null checks should not be necessary + if (renderPass == nullptr) { + HandleError("Render pass invalid"); + return; + } this->renderPass = renderPass; this->textureViews.resize(renderPass->GetAttachmentCount()); diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index 66f91bfe27..47e8eb5ff2 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -26,8 +26,11 @@ add_executable(nxt_unittests ${UNITTESTS_DIR}/ToBackendTests.cpp ${UNITTESTS_DIR}/WireTests.cpp ${VALIDATION_TESTS_DIR}/BufferValidationTests.cpp + ${VALIDATION_TESTS_DIR}/CommandBufferValidationTests.cpp ${VALIDATION_TESTS_DIR}/ComputeValidationTests.cpp ${VALIDATION_TESTS_DIR}/DepthStencilStateValidationTests.cpp + ${VALIDATION_TESTS_DIR}/FramebufferValidationTests.cpp + ${VALIDATION_TESTS_DIR}/RenderPassValidationTests.cpp ${VALIDATION_TESTS_DIR}/ValidationTest.cpp ${VALIDATION_TESTS_DIR}/ValidationTest.h ${TESTS_DIR}/UnittestsMain.cpp diff --git a/src/tests/unittests/validation/CommandBufferValidationTests.cpp b/src/tests/unittests/validation/CommandBufferValidationTests.cpp new file mode 100644 index 0000000000..f6feaf86c1 --- /dev/null +++ b/src/tests/unittests/validation/CommandBufferValidationTests.cpp @@ -0,0 +1,65 @@ +// Copyright 2017 The NXT Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "ValidationTest.h" + +class CommandBufferValidationTest : public ValidationTest { +}; + +// Test for an empty command buffer +TEST_F(CommandBufferValidationTest, Empty) { + AssertWillBeSuccess(device.CreateCommandBufferBuilder()) + .GetResult(); +} + +// Tests for null arguments to the command buffer builder +TEST_F(CommandBufferValidationTest, NullArguments) { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(0) + .GetResult(); + auto framebuffer = AssertWillBeSuccess(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetDimensions(100, 100) + .GetResult(); + + AssertWillBeError(device.CreateCommandBufferBuilder()) + .BeginRenderPass(renderpass, nullptr) + .EndRenderPass() + .GetResult(); + AssertWillBeError(device.CreateCommandBufferBuilder()) + .BeginRenderPass(nullptr, framebuffer) + .EndRenderPass() + .GetResult(); +} + +// Tests for basic render pass usage +TEST_F(CommandBufferValidationTest, RenderPass) { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetAttachmentCount(0) + .SetSubpassCount(1) + .GetResult(); + auto framebuffer = AssertWillBeSuccess(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetDimensions(100, 100) + .GetResult(); + + AssertWillBeSuccess(device.CreateCommandBufferBuilder()) + .BeginRenderPass(renderpass, framebuffer) + .EndRenderPass() + .GetResult(); + AssertWillBeError(device.CreateCommandBufferBuilder()) + .BeginRenderPass(renderpass, framebuffer) + .GetResult(); +} diff --git a/src/tests/unittests/validation/FramebufferValidationTests.cpp b/src/tests/unittests/validation/FramebufferValidationTests.cpp new file mode 100644 index 0000000000..027ca8e1b1 --- /dev/null +++ b/src/tests/unittests/validation/FramebufferValidationTests.cpp @@ -0,0 +1,66 @@ +// Copyright 2017 The NXT Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "ValidationTest.h" + +class FramebufferValidationTest : public ValidationTest { +}; + +// Test for an empty framebuffer builder +TEST_F(FramebufferValidationTest, Empty) { + auto framebuffer = AssertWillBeError(device.CreateFramebufferBuilder()) + .GetResult(); +} + +// Tests for null arguments to a framebuffer builder +TEST_F(FramebufferValidationTest, NullArguments) { + AssertWillBeError(device.CreateFramebufferBuilder()) + .SetRenderPass(nullptr) + .GetResult(); +} + +// Tests for passing error-valued arguments to a framebuffer builder +TEST_F(FramebufferValidationTest, ErrorValues) { + auto renderpass = AssertWillBeError(device.CreateRenderPassBuilder()) + .GetResult(); + AssertWillBeError(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .GetResult(); +} + +// Tests for basic framebuffer construction +TEST_F(FramebufferValidationTest, Basic) { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(0) + .GetResult(); + auto framebuffer = AssertWillBeSuccess(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetDimensions(100, 100) + .GetResult(); +} + +// Tests for framebuffer construction with an (empty) attachment +TEST_F(FramebufferValidationTest, BasicWithEmptyAttachment) { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .SetSubpassCount(1) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + auto framebuffer = AssertWillBeSuccess(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetDimensions(100, 100) + .GetResult(); +} diff --git a/src/tests/unittests/validation/RenderPassValidationTests.cpp b/src/tests/unittests/validation/RenderPassValidationTests.cpp new file mode 100644 index 0000000000..5d6b448536 --- /dev/null +++ b/src/tests/unittests/validation/RenderPassValidationTests.cpp @@ -0,0 +1,42 @@ +// Copyright 2017 The NXT Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "ValidationTest.h" + +class RenderPassValidationTest : public ValidationTest { +}; + +// Test for an empty render pass builder +TEST_F(RenderPassValidationTest, Empty) { + AssertWillBeError(device.CreateRenderPassBuilder()) + .GetResult(); +} + +// Test for a render pass with one subpass and no attachments +TEST_F(RenderPassValidationTest, OneSubpass) { + AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(0) + .GetResult(); +} + +// Test for a render pass with one subpass and one attachment +TEST_F(RenderPassValidationTest, OneSubpassOneAttachment) { + AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); +}