Skip to content

Commit 644e978

Browse files
PurschePursche
authored
Add validation for unbound descriptor sets (#55)
Add validation for unbound descriptor sets Fix bug where DynamicArray would not free
1 parent 67e495f commit 644e978

7 files changed

Lines changed: 122 additions & 5 deletions

File tree

‎Source/Base/Base/Container/DynamicArray.h‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,9 @@ class DynamicArray
3939
{
4040
assert(_count > index);
4141

42-
for (int i = index; index < _count; i++)
42+
for (size_t i = index; i + 1 < _count; i++)
4343
{
44-
if (i + 1 < _count)
45-
{
46-
_data[i] = _data[i + 1];
47-
}
44+
_data[i] = _data[i + 1];
4845
}
4946

5047
_count--;

‎Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ namespace Renderer
3131

3232
i8 renderPassOpenCount = 0;
3333
i8 pipelineOpenCount = 0;
34+
u8 unboundDescriptorSetsMask = 0; // bits set = descriptor set slots required by the bound pipeline that haven't been bound on this command list yet
3435
QueueType queueType = QueueType::Graphics;
3536
};
3637

@@ -321,6 +322,24 @@ namespace Renderer
321322
commandList.pipelineOpenCount = count;
322323
}
323324

325+
u8 CommandListHandlerVK::GetUnboundDescriptorSets(CommandListID id)
326+
{
327+
CommandListHandlerVKData& data = static_cast<CommandListHandlerVKData&>(*_data);
328+
329+
CommandList& commandList = data.commandLists[static_cast<CommandListID::type>(id)];
330+
331+
return commandList.unboundDescriptorSetsMask;
332+
}
333+
334+
void CommandListHandlerVK::SetUnboundDescriptorSets(CommandListID id, u8 mask)
335+
{
336+
CommandListHandlerVKData& data = static_cast<CommandListHandlerVKData&>(*_data);
337+
338+
CommandList& commandList = data.commandLists[static_cast<CommandListID::type>(id)];
339+
340+
commandList.unboundDescriptorSetsMask = mask;
341+
}
342+
324343
tracy::VkCtxManualScope*& CommandListHandlerVK::GetTracyScope(CommandListID id)
325344
{
326345
CommandListHandlerVKData& data = static_cast<CommandListHandlerVKData&>(*_data);

‎Source/Renderer/Renderer/Renderers/Vulkan/Backend/CommandListHandlerVK.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,11 @@ namespace Renderer
5656
i8 GetPipelineOpenCount(CommandListID id);
5757
void SetPipelineOpenCount(CommandListID id, i8 count);
5858

59+
// Bitmask of descriptor set slots the currently bound pipeline statically uses but which haven't been bound on this command list yet.
60+
// Initialized in BeginPipeline from the pipeline's used-set mask, cleared bit-by-bit by BindDescriptorSet, validated by Draw / Dispatch, reset on EndPipeline.
61+
u8 GetUnboundDescriptorSets(CommandListID id);
62+
void SetUnboundDescriptorSets(CommandListID id, u8 mask);
63+
5964
tracy::VkCtxManualScope*& GetTracyScope(CommandListID id);
6065

6166
VkFence GetCurrentFence();

‎Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.cpp‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ namespace Renderer
4343

4444
std::unordered_set<u32> setsUsed;
4545
std::unordered_map<u32, PersistentBitSet> usedBindingsPerSlot;
46+
u8 usedDescriptorSetMask = 0; // bit `slot` set for each statically-used descriptor set slot (including DEBUG)
4647
};
4748

4849
struct ComputePipelineCacheDesc
@@ -65,6 +66,7 @@ namespace Renderer
6566

6667
std::unordered_set<u32> setsUsed;
6768
std::unordered_map<u32, PersistentBitSet> usedBindingsPerSlot;
69+
u8 usedDescriptorSetMask = 0; // bit `slot` set for each statically-used descriptor set slot (including DEBUG)
6870
};
6971

7072
struct PipelineHandlerVKData : IPipelineHandlerVKData
@@ -282,6 +284,18 @@ namespace Renderer
282284
return static_cast<u32>(data.computePipelines[static_cast<cIDType>(id)].setsUsed.contains(setNumber));
283285
}
284286

287+
u8 PipelineHandlerVK::GetUsedDescriptorSetMask(GraphicsPipelineID id)
288+
{
289+
PipelineHandlerVKData& data = static_cast<PipelineHandlerVKData&>(*_data);
290+
return data.graphicsPipelines[static_cast<gIDType>(id)].usedDescriptorSetMask;
291+
}
292+
293+
u8 PipelineHandlerVK::GetUsedDescriptorSetMask(ComputePipelineID id)
294+
{
295+
PipelineHandlerVKData& data = static_cast<PipelineHandlerVKData&>(*_data);
296+
return data.computePipelines[static_cast<cIDType>(id)].usedDescriptorSetMask;
297+
}
298+
285299
const PersistentBitSet* PipelineHandlerVK::GetUsedBindings(GraphicsPipelineID id, u32 slot)
286300
{
287301
PipelineHandlerVKData& data = static_cast<PipelineHandlerVKData&>(*_data);
@@ -476,6 +490,15 @@ namespace Renderer
476490
bindInfoPushConstants.insert(bindInfoPushConstants.end(), bindReflection.pushConstants.begin(), bindReflection.pushConstants.end());
477491
}
478492

493+
// Build the used-set bitmask from reflection. DEBUG is included: if a shader actively uses the DEBUG set we want
494+
// the draw validator to flag a missing bind. The asymmetric "binding DEBUG that the pipeline doesn't use" case
495+
// is still tolerated because BindDescriptorSet silently early-returns for DEBUG when the pipeline doesn't use it.
496+
pipeline.usedDescriptorSetMask = 0;
497+
for (u32 slot : pipeline.setsUsed)
498+
{
499+
pipeline.usedDescriptorSetMask |= static_cast<u8>(1u << slot);
500+
}
501+
479502
// -- Create Descriptor Set Layout from reflected SPIR-V --
480503
for (BindInfo& bindInfo : bindInfos)
481504
{
@@ -822,6 +845,15 @@ namespace Renderer
822845
pipeline.usedBindingsPerSlot[bindInfo.set].Set(bindInfo.binding);
823846
}
824847

848+
// Build the used-set bitmask from reflection. DEBUG is included: if a shader actively uses the DEBUG set we want
849+
// the draw validator to flag a missing bind. The asymmetric "binding DEBUG that the pipeline doesn't use" case
850+
// is still tolerated because BindDescriptorSet silently early-returns for DEBUG when the pipeline doesn't use it.
851+
pipeline.usedDescriptorSetMask = 0;
852+
for (u32 slot : pipeline.setsUsed)
853+
{
854+
pipeline.usedDescriptorSetMask |= static_cast<u8>(1u << slot);
855+
}
856+
825857
std::vector<BindInfo> bindInfos;
826858
std::vector<BindInfoPushConstant> bindInfoPushConstants;
827859
const BindReflection& bindReflection = _shaderHandler->GetFullBindReflection(desc.computeShader);

‎Source/Renderer/Renderer/Renderers/Vulkan/Backend/PipelineHandlerVK.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,10 @@ namespace Renderer
7474
bool UsesDescriptorSet(GraphicsPipelineID id, u32 setNumber);
7575
bool UsesDescriptorSet(ComputePipelineID id, u32 setNumber);
7676

77+
// Returns a bitmask where bit `slot` is set for each descriptor set slot the pipeline statically uses (including DEBUG).
78+
u8 GetUsedDescriptorSetMask(GraphicsPipelineID id);
79+
u8 GetUsedDescriptorSetMask(ComputePipelineID id);
80+
7781
const PersistentBitSet* GetUsedBindings(GraphicsPipelineID id, u32 slot);
7882
const PersistentBitSet* GetUsedBindings(ComputePipelineID id, u32 slot);
7983

‎Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.cpp‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -601,6 +601,39 @@ namespace Renderer
601601
_device->TransitionImageLayout(commandBuffer, image, range.aspectMask, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, VK_IMAGE_LAYOUT_DEPTH_STENCIL_READ_ONLY_OPTIMAL, 1, 1);
602602
}
603603

604+
namespace
605+
{
606+
const char* DescriptorSetSlotToString(u32 slot)
607+
{
608+
switch (slot)
609+
{
610+
case DescriptorSetSlot::DEBUG: return "DEBUG";
611+
case DescriptorSetSlot::GLOBAL: return "GLOBAL";
612+
case DescriptorSetSlot::LIGHT: return "LIGHT";
613+
case DescriptorSetSlot::TERRAIN: return "TERRAIN";
614+
case DescriptorSetSlot::MODEL: return "MODEL";
615+
case DescriptorSetSlot::PER_PASS: return "PER_PASS";
616+
case DescriptorSetSlot::PER_DRAW: return "PER_DRAW";
617+
default: return "UNKNOWN";
618+
}
619+
}
620+
}
621+
622+
void RendererVK::ValidateBoundDescriptorSets(CommandListID commandListID, const char* opName)
623+
{
624+
u8 mask = _commandListHandler->GetUnboundDescriptorSets(commandListID);
625+
if (mask == 0)
626+
return;
627+
628+
for (u32 slot = 0; slot < 8; slot++)
629+
{
630+
if ((mask & (1u << slot)) == 0)
631+
continue;
632+
633+
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));
634+
}
635+
}
636+
604637
void RendererVK::Draw(CommandListID commandListID, u32 numVertices, u32 numInstances, u32 vertexOffset, u32 instanceOffset)
605638
{
606639
VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID);
@@ -609,6 +642,7 @@ namespace Renderer
609642
{
610643
NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!");
611644
}
645+
ValidateBoundDescriptorSets(commandListID, "vkCmdDraw");
612646

613647
vkCmdDraw(commandBuffer, numVertices, numInstances, vertexOffset, instanceOffset);
614648
}
@@ -621,6 +655,7 @@ namespace Renderer
621655
{
622656
NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!");
623657
}
658+
ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndirect");
624659

625660
VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer);
626661

@@ -635,6 +670,7 @@ namespace Renderer
635670
{
636671
NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!");
637672
}
673+
ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndirectCount");
638674

639675
VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer);
640676
VkBuffer vkDrawCountBuffer = _bufferHandler->GetBuffer(drawCountBuffer);
@@ -650,6 +686,7 @@ namespace Renderer
650686
{
651687
NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!");
652688
}
689+
ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndexed");
653690

654691
vkCmdDrawIndexed(commandBuffer, numIndices, numInstances, indexOffset, vertexOffset, instanceOffset);
655692
}
@@ -662,6 +699,7 @@ namespace Renderer
662699
{
663700
NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!");
664701
}
702+
ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndexedIndirect");
665703

666704
VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer);
667705

@@ -676,6 +714,7 @@ namespace Renderer
676714
{
677715
NC_LOG_CRITICAL("You tried to draw without first calling BeginPipeline!");
678716
}
717+
ValidateBoundDescriptorSets(commandListID, "vkCmdDrawIndexedIndirectCount");
679718

680719
VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer);
681720
VkBuffer vkDrawCountBuffer = _bufferHandler->GetBuffer(drawCountBuffer);
@@ -687,6 +726,8 @@ namespace Renderer
687726
{
688727
VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID);
689728

729+
ValidateBoundDescriptorSets(commandListID, "vkCmdDispatch");
730+
690731
vkCmdDispatch(commandBuffer, threadGroupCountX, threadGroupCountY, threadGroupCountZ);
691732
}
692733

@@ -695,6 +736,8 @@ namespace Renderer
695736
VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID);
696737
VkBuffer vkArgumentBuffer = _bufferHandler->GetBuffer(argumentBuffer);
697738

739+
ValidateBoundDescriptorSets(commandListID, "vkCmdDispatchIndirect");
740+
698741
vkCmdDispatchIndirect(commandBuffer, vkArgumentBuffer, argumentBufferOffset);
699742
}
700743

@@ -1174,6 +1217,7 @@ namespace Renderer
11741217
vkCmdBindPipeline(commandBuffer, VK_PIPELINE_BIND_POINT_GRAPHICS, pipeline);
11751218

11761219
_commandListHandler->SetBoundGraphicsPipeline(commandListID, pipelineID);
1220+
_commandListHandler->SetUnboundDescriptorSets(commandListID, _pipelineHandler->GetUsedDescriptorSetMask(pipelineID));
11771221
}
11781222

11791223
void RendererVK::EndPipeline(CommandListID commandListID, GraphicsPipelineID pipelineID)
@@ -1194,6 +1238,7 @@ namespace Renderer
11941238

11951239
VkCommandBuffer commandBuffer = _commandListHandler->GetCommandBuffer(commandListID);
11961240
_commandListHandler->SetBoundGraphicsPipeline(commandListID, GraphicsPipelineID::Invalid());
1241+
_commandListHandler->SetUnboundDescriptorSets(commandListID, 0);
11971242
}
11981243

11991244
void RendererVK::BeginPipeline(CommandListID commandListID, ComputePipelineID pipelineID)
@@ -1217,6 +1262,7 @@ namespace Renderer
12171262
vkCmdBindPipeline(commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipeline);
12181263

12191264
_commandListHandler->SetBoundComputePipeline(commandListID, pipelineID);
1265+
_commandListHandler->SetUnboundDescriptorSets(commandListID, _pipelineHandler->GetUsedDescriptorSetMask(pipelineID));
12201266
}
12211267

12221268
void RendererVK::EndPipeline(CommandListID commandListID, ComputePipelineID pipelineID)
@@ -1236,6 +1282,7 @@ namespace Renderer
12361282
vkCmdBindPipeline(commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipeline);
12371283

12381284
_commandListHandler->SetBoundComputePipeline(commandListID, ComputePipelineID::Invalid());
1285+
_commandListHandler->SetUnboundDescriptorSets(commandListID, 0);
12391286
}
12401287

12411288
void RendererVK::BeginTimeQuery(CommandListID commandListID, TimeQueryID timeQueryID)
@@ -1420,6 +1467,11 @@ namespace Renderer
14201467

14211468
// Bind descriptor set
14221469
vkCmdBindDescriptorSets(commandBuffer, VK_PIPELINE_BIND_POINT_GRAPHICS, pipelineLayout, slot, 1, &vkDescriptorSet, 0, nullptr);
1470+
1471+
// Clear this slot from the "still required to be bound" mask
1472+
u8 mask = _commandListHandler->GetUnboundDescriptorSets(commandListID);
1473+
mask &= ~static_cast<u8>(1u << slot);
1474+
_commandListHandler->SetUnboundDescriptorSets(commandListID, mask);
14231475
}
14241476
else if (computePipelineID != ComputePipelineID::Invalid())
14251477
{
@@ -1438,6 +1490,11 @@ namespace Renderer
14381490

14391491
// Bind descriptor set
14401492
vkCmdBindDescriptorSets(commandBuffer, VK_PIPELINE_BIND_POINT_COMPUTE, pipelineLayout, slot, 1, &vkDescriptorSet, 0, nullptr);
1493+
1494+
// Clear this slot from the "still required to be bound" mask
1495+
u8 mask = _commandListHandler->GetUnboundDescriptorSets(commandListID);
1496+
mask &= ~static_cast<u8>(1u << slot);
1497+
_commandListHandler->SetUnboundDescriptorSets(commandListID, mask);
14411498
}
14421499
}
14431500

‎Source/Renderer/Renderer/Renderers/Vulkan/RendererVK.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,9 @@ namespace Renderer
241241
void RecreateSwapChain(Backend::SwapChainVK* swapChain);
242242
void CreateDummyPipeline();
243243

244+
// 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.
245+
void ValidateBoundDescriptorSets(CommandListID commandListID, const char* opName);
246+
244247
private:
245248
Backend::RenderDeviceVK* _device = nullptr;
246249
Backend::BufferHandlerVK* _bufferHandler = nullptr;

0 commit comments

Comments
 (0)