From 67a11308f5716f7bb647f217d4691b76f436849d Mon Sep 17 00:00:00 2001 From: Pursche Date: Sat, 25 Apr 2026 15:17:37 +0200 Subject: [PATCH] Add validation for unbound descriptor sets Add validation for unbound descriptor sets Fix bug where DynamicArray would not free --- Source/Base/Base/Container/DynamicArray.h | 7 +-- .../Vulkan/Backend/CommandListHandlerVK.cpp | 19 +++++++ .../Vulkan/Backend/CommandListHandlerVK.h | 5 ++ .../Vulkan/Backend/PipelineHandlerVK.cpp | 32 +++++++++++ .../Vulkan/Backend/PipelineHandlerVK.h | 4 ++ .../Renderer/Renderers/Vulkan/RendererVK.cpp | 57 +++++++++++++++++++ .../Renderer/Renderers/Vulkan/RendererVK.h | 3 + 7 files changed, 122 insertions(+), 5 deletions(-) diff --git a/Source/Base/Base/Container/DynamicArray.h b/Source/Base/Base/Container/DynamicArray.h index b18ca8f3..15b4f0d5 100644 --- a/Source/Base/Base/Container/DynamicArray.h +++ b/Source/Base/Base/Container/DynamicArray.h @@ -39,12 +39,9 @@ class DynamicArray { assert(_count > index); - for (int i = index; index < _count; i++) + for (size_t i = index; i + 1 < _count; i++) { - if (i + 1 < _count) - { - _data[i] = _data[i + 1]; - } + _data[i] = _data[i + 1]; } _count--; diff --git a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.cpp b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.cpp index f6119df4..72e3d6ae 100644 --- a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.cpp +++ b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.cpp @@ -31,6 +31,7 @@ namespace Renderer i8 renderPassOpenCount = 0; i8 pipelineOpenCount = 0; + u8 unboundDescriptorSetsMask = 0; // bits set = descriptor set slots required by the bound pipeline that haven't been bound on this command list yet QueueType queueType = QueueType::Graphics; }; @@ -321,6 +322,24 @@ namespace Renderer commandList.pipelineOpenCount = count; } + u8 CommandListHandlerVK::GetUnboundDescriptorSets(CommandListID id) + { + CommandListHandlerVKData& data = static_cast(*_data); + + CommandList& commandList = data.commandLists[static_cast(id)]; + + return commandList.unboundDescriptorSetsMask; + } + + void CommandListHandlerVK::SetUnboundDescriptorSets(CommandListID id, u8 mask) + { + CommandListHandlerVKData& data = static_cast(*_data); + + CommandList& commandList = data.commandLists[static_cast(id)]; + + commandList.unboundDescriptorSetsMask = mask; + } + tracy::VkCtxManualScope*& CommandListHandlerVK::GetTracyScope(CommandListID id) { CommandListHandlerVKData& data = static_cast(*_data); diff --git a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.h b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.h index 4a61fc5b..62ba2583 100644 --- a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.h +++ b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.h @@ -56,6 +56,11 @@ namespace Renderer i8 GetPipelineOpenCount(CommandListID id); void SetPipelineOpenCount(CommandListID id, i8 count); + // Bitmask of descriptor set slots the currently bound pipeline statically uses but which haven't been bound on this command list yet. + // Initialized in BeginPipeline from the pipeline's used-set mask, cleared bit-by-bit by BindDescriptorSet, validated by Draw / Dispatch, reset on EndPipeline. + u8 GetUnboundDescriptorSets(CommandListID id); + void SetUnboundDescriptorSets(CommandListID id, u8 mask); + tracy::VkCtxManualScope*& GetTracyScope(CommandListID id); VkFence GetCurrentFence(); diff --git a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.cpp b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.cpp index 826ac781..9b0cf35f 100644 --- a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.cpp +++ b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.cpp @@ -43,6 +43,7 @@ namespace Renderer std::unordered_set setsUsed; std::unordered_map usedBindingsPerSlot; + u8 usedDescriptorSetMask = 0; // bit `slot` set for each statically-used descriptor set slot (including DEBUG) }; struct ComputePipelineCacheDesc @@ -65,6 +66,7 @@ namespace Renderer std::unordered_set setsUsed; std::unordered_map usedBindingsPerSlot; + u8 usedDescriptorSetMask = 0; // bit `slot` set for each statically-used descriptor set slot (including DEBUG) }; struct PipelineHandlerVKData : IPipelineHandlerVKData @@ -282,6 +284,18 @@ namespace Renderer return static_cast(data.computePipelines[static_cast(id)].setsUsed.contains(setNumber)); } + u8 PipelineHandlerVK::GetUsedDescriptorSetMask(GraphicsPipelineID id) + { + PipelineHandlerVKData& data = static_cast(*_data); + return data.graphicsPipelines[static_cast(id)].usedDescriptorSetMask; + } + + u8 PipelineHandlerVK::GetUsedDescriptorSetMask(ComputePipelineID id) + { + PipelineHandlerVKData& data = static_cast(*_data); + return data.computePipelines[static_cast(id)].usedDescriptorSetMask; + } + const PersistentBitSet* PipelineHandlerVK::GetUsedBindings(GraphicsPipelineID id, u32 slot) { PipelineHandlerVKData& data = static_cast(*_data); @@ -476,6 +490,15 @@ namespace Renderer bindInfoPushConstants.insert(bindInfoPushConstants.end(), bindReflection.pushConstants.begin(), bindReflection.pushConstants.end()); } + // Build the used-set bitmask from reflection. DEBUG is included: if a shader actively uses the DEBUG set we want + // the draw validator to flag a missing bind. The asymmetric "binding DEBUG that the pipeline doesn't use" case + // is still tolerated because BindDescriptorSet silently early-returns for DEBUG when the pipeline doesn't use it. + pipeline.usedDescriptorSetMask = 0; + for (u32 slot : pipeline.setsUsed) + { + pipeline.usedDescriptorSetMask |= static_cast(1u << slot); + } + // -- Create Descriptor Set Layout from reflected SPIR-V -- for (BindInfo& bindInfo : bindInfos) { @@ -822,6 +845,15 @@ namespace Renderer pipeline.usedBindingsPerSlot[bindInfo.set].Set(bindInfo.binding); } + // Build the used-set bitmask from reflection. DEBUG is included: if a shader actively uses the DEBUG set we want + // the draw validator to flag a missing bind. The asymmetric "binding DEBUG that the pipeline doesn't use" case + // is still tolerated because BindDescriptorSet silently early-returns for DEBUG when the pipeline doesn't use it. + pipeline.usedDescriptorSetMask = 0; + for (u32 slot : pipeline.setsUsed) + { + pipeline.usedDescriptorSetMask |= static_cast(1u << slot); + } + std::vector bindInfos; std::vector bindInfoPushConstants; const BindReflection& bindReflection = _shaderHandler->GetFullBindReflection(desc.computeShader); diff --git a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.h b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.h index c105786f..fec84bff 100644 --- a/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.h +++ b/Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.h @@ -74,6 +74,10 @@ namespace Renderer bool UsesDescriptorSet(GraphicsPipelineID id, u32 setNumber); bool UsesDescriptorSet(ComputePipelineID id, u32 setNumber); + // Returns a bitmask where bit `slot` is set for each descriptor set slot the pipeline statically uses (including DEBUG). + u8 GetUsedDescriptorSetMask(GraphicsPipelineID id); + u8 GetUsedDescriptorSetMask(ComputePipelineID id); + const PersistentBitSet* GetUsedBindings(GraphicsPipelineID id, u32 slot); const PersistentBitSet* GetUsedBindings(ComputePipelineID id, u32 slot); diff --git a/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.cpp b/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.cpp index 9c699450..e75967aa 100644 --- a/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.cpp +++ b/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.cpp @@ -601,6 +601,39 @@ namespace Renderer _device->TransitionImageLayout(commandBuffer, image, range.aspectMask, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL, 1, 1); } + namespace + { + const char* DescriptorSetSlotToString(u32 slot) + { + switch (slot) + { + case DescriptorSetSlot::DEBUG: return "DEBUG"; + case DescriptorSetSlot::GLOBAL: return "GLOBAL"; + case DescriptorSetSlot::LIGHT: return "LIGHT"; + case DescriptorSetSlot::TERRAIN: return "TERRAIN"; + case DescriptorSetSlot::MODEL: return "MODEL"; + case DescriptorSetSlot::PER_PASS: return "PER_PASS"; + case DescriptorSetSlot::PER_DRAW: return "PER_DRAW"; + default: return "UNKNOWN"; + } + } + } + + void RendererVK::ValidateBoundDescriptorSets(CommandListID commandListID, const char* opName) + { + u8 mask = _commandListHandler->GetUnboundDescriptorSets(commandListID); + if (mask == 0) + return; + + for (u32 slot = 0; slot < 8; slot++) + { + if ((mask & (1u << slot)) == 0) + continue; + + NC_LOG_CRITICAL("{} called with descriptor set slot {} ({}) statically used by the bound pipeline but never bound on this command list", opName, slot, DescriptorSetSlotToString(slot)); + } + } + void RendererVK::Draw(CommandListID commandListID, u32 numVertices, u32 numInstances, u32 vertexOffset, u32 instanceOffset) { VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID); @@ -609,6 +642,7 @@ namespace Renderer { NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!"); } + ValidateBoundDescriptorSets(commandListID, "vkCmdDraw"); vkCmdDraw(commandBuffer, numVertices, numInstances, vertexOffset, instanceOffset); } @@ -621,6 +655,7 @@ namespace Renderer { NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!"); } + ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndirect"); VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer); @@ -635,6 +670,7 @@ namespace Renderer { NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!"); } + ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndirectCount"); VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer); VkBuffer vkDrawCountBuffer = _bufferHandler->GetBuffer(drawCountBuffer); @@ -650,6 +686,7 @@ namespace Renderer { NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!"); } + ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndexed"); vkCmdDrawIndexed(commandBuffer, numIndices, numInstances, indexOffset, vertexOffset, instanceOffset); } @@ -662,6 +699,7 @@ namespace Renderer { NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!"); } + ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndexedIndirect"); VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer); @@ -676,6 +714,7 @@ namespace Renderer { NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!"); } + ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndexedIndirectCount"); VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer); VkBuffer vkDrawCountBuffer = _bufferHandler->GetBuffer(drawCountBuffer); @@ -687,6 +726,8 @@ namespace Renderer { VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID); + ValidateBoundDescriptorSets(commandListID, "vkCmdDispatch"); + vkCmdDispatch(commandBuffer, threadGroupCountX, threadGroupCountY, threadGroupCountZ); } @@ -695,6 +736,8 @@ namespace Renderer VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID); VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer); + ValidateBoundDescriptorSets(commandListID, "vkCmdDispatchIndirect"); + vkCmdDispatchIndirect(commandBuffer, vkArgumentBuffer, argumentBufferOffset); } @@ -1174,6 +1217,7 @@ namespace Renderer vkCmdBindPipeline(commandBuffer, VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline); _commandListHandler->SetBoundGraphicsPipeline(commandListID, pipelineID); + _commandListHandler->SetUnboundDescriptorSets(commandListID, _pipelineHandler->GetUsedDescriptorSetMask(pipelineID)); } void RendererVK::EndPipeline(CommandListID commandListID, GraphicsPipelineID pipelineID) @@ -1194,6 +1238,7 @@ namespace Renderer VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID); _commandListHandler->SetBoundGraphicsPipeline(commandListID, GraphicsPipelineID::Invalid()); + _commandListHandler->SetUnboundDescriptorSets(commandListID, 0); } void RendererVK::BeginPipeline(CommandListID commandListID, ComputePipelineID pipelineID) @@ -1217,6 +1262,7 @@ namespace Renderer vkCmdBindPipeline(commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipeline); _commandListHandler->SetBoundComputePipeline(commandListID, pipelineID); + _commandListHandler->SetUnboundDescriptorSets(commandListID, _pipelineHandler->GetUsedDescriptorSetMask(pipelineID)); } void RendererVK::EndPipeline(CommandListID commandListID, ComputePipelineID pipelineID) @@ -1236,6 +1282,7 @@ namespace Renderer vkCmdBindPipeline(commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipeline); _commandListHandler->SetBoundComputePipeline(commandListID, ComputePipelineID::Invalid()); + _commandListHandler->SetUnboundDescriptorSets(commandListID, 0); } void RendererVK::BeginTimeQuery(CommandListID commandListID, TimeQueryID timeQueryID) @@ -1420,6 +1467,11 @@ namespace Renderer // Bind descriptor set vkCmdBindDescriptorSets(commandBuffer, VK_PIPELINE_BIND_POINT_GRAPHICS, pipelineLayout, slot, 1, &vkDescriptorSet, 0, nullptr); + + // Clear this slot from the "still required to be bound" mask + u8 mask = _commandListHandler->GetUnboundDescriptorSets(commandListID); + mask &= ~static_cast(1u << slot); + _commandListHandler->SetUnboundDescriptorSets(commandListID, mask); } else if (computePipelineID != ComputePipelineID::Invalid()) { @@ -1438,6 +1490,11 @@ namespace Renderer // Bind descriptor set vkCmdBindDescriptorSets(commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipelineLayout, slot, 1, &vkDescriptorSet, 0, nullptr); + + // Clear this slot from the "still required to be bound" mask + u8 mask = _commandListHandler->GetUnboundDescriptorSets(commandListID); + mask &= ~static_cast(1u << slot); + _commandListHandler->SetUnboundDescriptorSets(commandListID, mask); } } diff --git a/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.h b/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.h index 5e68c449..a67b87f6 100644 --- a/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.h +++ b/Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.h @@ -241,6 +241,9 @@ namespace Renderer void RecreateSwapChain(Backend::SwapChainVK* swapChain); void CreateDummyPipeline(); + // Logs an NC_LOG_CRITICAL for every descriptor set slot the bound pipeline statically uses but which has not been bound on this command list. Called from every Draw* / Dispatch* variant. + void ValidateBoundDescriptorSets(CommandListID commandListID, const char* opName); + private: Backend::RenderDeviceVK* _device = nullptr; Backend::BufferHandlerVK* _bufferHandler = nullptr;