Handle descriptor set layout compatibility and sets becoming invalidated

* The example case is as follows:
  - program binds a pipeline and 4 sets
  - later, program rebinds a different pipeline with incompatible layout
  - all 4 sets are invalid
  - program only binds 2 sets as new program shaders only statically use
    the first 2 sets.
* If we try to 'restore' the invalid set bindings at any point this is
  invalid and can lead to crashes. We need to detect when sets have been
  invalidated and bail out, assuming that the shader only statically
  uses the sets that were valid.
* Of course if that is an unfounded assumption then we are not binding
  sets that will be used, but this is just as invalid as leaving used
  sets as invalid.
* We also need to change the PostVS data fetch, since we can't use the
  set after all used sets, we have to instead see which set is the last
  statically used one and use the next (even if it's in the original
  layout).

renderdoc/driver/vulkan/vk_core.cpp

@@ -256,7 +256,7 @@ void VkInitParams::Set(const VkInstanceCreateInfo *pCreateInfo, ResourceId inst)
   InstanceID = inst;
 }
 
-WrappedVulkan::WrappedVulkan(const char *logFilename) : m_RenderState(&m_CreationInfo)
+WrappedVulkan::WrappedVulkan(const char *logFilename) : m_RenderState(this, &m_CreationInfo)
 {
 #if ENABLED(RDOC_RELEASE)
   const bool debugSerialiser = false;
@@ -317,7 +317,6 @@ WrappedVulkan::WrappedVulkan(const char *logFilename) : m_RenderState(&m_Creatio
   m_DebugManager = NULL;
 
   m_pSerialiser->SetUserData(m_ResourceManager);
-  m_RenderState.m_ResourceManager = GetResourceManager();
 
   m_Instance = VK_NULL_HANDLE;
   m_PhysicalDevice = VK_NULL_HANDLE;
@@ -2361,8 +2360,7 @@ void WrappedVulkan::ReplayLog(uint32_t startEventID, uint32_t endEventID, Replay
       RDCASSERT(m_Partial[Secondary].resultPartialCmdBuffer == VK_NULL_HANDLE);
       m_Partial[Primary].Reset();
       m_Partial[Secondary].Reset();
-      m_RenderState = VulkanRenderState(&m_CreationInfo);
-      m_RenderState.m_ResourceManager = GetResourceManager();
+      m_RenderState = VulkanRenderState(this, &m_CreationInfo);
     }
 
     VkResult vkr = VK_SUCCESS;
@@ -2414,13 +2412,32 @@ void WrappedVulkan::ReplayLog(uint32_t startEventID, uint32_t endEventID, Replay
                                          VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT, false, 0, NULL, 0, NULL,
                                          (uint32_t)imgBarriers.size(), &imgBarriers[0]);
 
+        const DrawcallDescription *draw = GetDrawcall(endEventID);
+
+        bool rpUnneeded = false;
+
+        // if we're only replaying a draw, and it's not a drawcall or dispatch, don't try and bind
+        // all the replay state as we don't know if it will be valid.
+        if(replayType == eReplay_OnlyDraw)
+        {
+          if(!draw)
+          {
+            rpUnneeded = true;
+          }
+          else if(!(draw->flags & (DrawFlags::Drawcall | DrawFlags::Dispatch)))
+          {
+            rpUnneeded = true;
+          }
+        }
+
         // if a render pass was active, begin it and set up the partial replay state
-        m_RenderState.BeginRenderPassAndApplyState(cmd);
+        m_RenderState.BeginRenderPassAndApplyState(
+            cmd, rpUnneeded ? VulkanRenderState::BindNone : VulkanRenderState::BindGraphics);
       }
       else if(m_RenderState.compute.pipeline != ResourceId())
       {
         // if we had a compute pipeline, need to bind that
-        m_RenderState.BindPipeline(cmd);
+        m_RenderState.BindPipeline(cmd, VulkanRenderState::BindCompute);
       }
     }
 

renderdoc/driver/vulkan/vk_core.h

@@ -714,6 +714,11 @@ public:
   uint32_t GetMaxEID() { return m_Events.back().eventID; }
   const DrawcallDescription *GetDrawcall(uint32_t eventID);
 
+  ResourceId GetDescLayoutForDescSet(ResourceId descSet)
+  {
+    return m_DescriptorSetState[descSet].layout;
+  }
+
   vector<EventUsage> GetUsage(ResourceId id) { return m_ResourceUses[id]; }
   // return the pre-selected device and queue
   VkDevice GetDev()

renderdoc/driver/vulkan/vk_debug.cpp

@@ -4771,7 +4771,7 @@ void VulkanDebugManager::PatchFixedColShader(VkShaderModule &mod, float col[4])
 struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback
 {
   VulkanQuadOverdrawCallback(WrappedVulkan *vk, const vector<uint32_t> &events)
-      : m_pDriver(vk), m_pDebug(vk->GetDebugManager()), m_Events(events), m_PrevState(NULL)
+      : m_pDriver(vk), m_pDebug(vk->GetDebugManager()), m_Events(events), m_PrevState(vk, NULL)
   {
     m_pDriver->SetDrawcallCB(this);
   }
@@ -4951,7 +4951,7 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback
     pipestate.graphics.descSets[pipe.first].descSet = GetResID(m_pDebug->m_QuadDescSet);
 
     if(cmd)
-      pipestate.BindPipeline(cmd);
+      pipestate.BindPipeline(cmd, VulkanRenderState::BindGraphics);
   }
 
   bool PostDraw(uint32_t eid, VkCommandBuffer cmd)
@@ -4963,7 +4963,7 @@ struct VulkanQuadOverdrawCallback : public VulkanDrawcallCallback
     m_pDriver->GetRenderState() = m_PrevState;
 
     RDCASSERT(cmd);
-    m_pDriver->GetRenderState().BindPipeline(cmd);
+    m_pDriver->GetRenderState().BindPipeline(cmd, VulkanRenderState::BindGraphics);
 
     return true;
   }
@@ -6057,7 +6057,7 @@ ResourceId VulkanDebugManager::RenderOverlay(ResourceId texid, DebugOverlay over
       vkr = vt->BeginCommandBuffer(Unwrap(cmd), &beginInfo);
       RDCASSERTEQUAL(vkr, VK_SUCCESS);
 
-      m_pDriver->m_RenderState.BeginRenderPassAndApplyState(cmd);
+      m_pDriver->m_RenderState.BeginRenderPassAndApplyState(cmd, VulkanRenderState::BindGraphics);
 
       VkClearAttachment blackclear = {VK_IMAGE_ASPECT_COLOR_BIT, 0, {}};
       vector<VkClearAttachment> atts;
@@ -7084,7 +7084,7 @@ inline uint32_t MakeSPIRVOp(spv::Op op, uint32_t WordCount)
 }
 
 static void AddOutputDumping(const ShaderReflection &refl, const SPIRVPatchData &patchData,
-                             const char *entryName, uint32_t descSet, uint32_t vertexIndexOffset,
+                             const char *entryName, uint32_t &descSet, uint32_t vertexIndexOffset,
                              uint32_t instanceIndexOffset, uint32_t numVerts,
                              vector<uint32_t> &modSpirv, uint32_t &bufStride)
 {
@@ -7147,12 +7147,20 @@ static void AddOutputDumping(const ShaderReflection &refl, const SPIRVPatchData
   size_t decorateOffset = 0;
   size_t typeVarOffset = 0;
 
+  descSet = 0;
+
   size_t it = 5;
   while(it < spirvLength)
   {
     uint16_t WordCount = spirv[it] >> spv::WordCountShift;
     spv::Op opcode = spv::Op(spirv[it] & spv::OpCodeMask);
 
+    // we will use the descriptor set immediately after the last set statically used by the shader.
+    // This means we don't have to worry about if the descriptor set layout declares more sets which
+    // might be invalid and un-bindable, we just trample over the next set that's unused
+    if(opcode == spv::OpDecorate && spirv[it + 2] == spv::DecorationDescriptorSet)
+      descSet = RDCMAX(descSet, spirv[it + 3] + 1);
+
     if(opcode == spv::OpDecorate && spirv[it + 2] == spv::DecorationBuiltIn &&
        spirv[it + 3] == spv::BuiltInVertexIndex)
       vertidxID = spirv[it + 1];
@@ -7907,52 +7915,26 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID)
   if(drawcall == NULL || drawcall->numIndices == 0 || drawcall->numInstances == 0)
     return;
 
-  uint32_t descSet = (uint32_t)creationInfo.m_PipelineLayout[pipeInfo.layout].descSetLayouts.size();
+  // the SPIR-V patching will determine the next descriptor set to use, after all sets statically
+  // used by the shader. This gets around the problem where the shader only uses 0 and 1, but the
+  // layout declares 0-4, and 2,3,4 are invalid at bind time and we are unable to bind our new set
+  // 5. Instead we'll notice that only 0 and 1 are used and just use 2 ourselves (although it was in
+  // the original set layout, we know it's statically unused by the shader so we can safely steal
+  // it).
+  uint32_t descSet = 0;
 
   // we go through the driver for all these creations since they need to be properly
   // registered in order to be put in the partial replay state
   VkResult vkr = VK_SUCCESS;
   VkDevice dev = m_Device;
 
-  VkDescriptorSetLayout *descSetLayouts;
-
-  // descSet will be the index of our new descriptor set
-  descSetLayouts = new VkDescriptorSetLayout[descSet + 1];
-
-  for(uint32_t i = 0; i < descSet; i++)
-    descSetLayouts[i] = GetResourceManager()->GetCurrentHandle<VkDescriptorSetLayout>(
-        creationInfo.m_PipelineLayout[pipeInfo.layout].descSetLayouts[i]);
-
-  // this layout just says it has one storage buffer
-  descSetLayouts[descSet] = m_MeshFetchDescSetLayout;
-
-  const vector<VkPushConstantRange> &push = creationInfo.m_PipelineLayout[pipeInfo.layout].pushRanges;
-
-  VkPipelineLayoutCreateInfo pipeLayoutInfo = {
-      VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO,
-      NULL,
-      0,
-      descSet + 1,
-      descSetLayouts,
-      (uint32_t)push.size(),
-      push.empty() ? NULL : &push[0],
-  };
-
-  // create pipeline layout with same descriptor set layouts, plus our mesh output set
   VkPipelineLayout pipeLayout;
-  vkr = m_pDriver->vkCreatePipelineLayout(dev, &pipeLayoutInfo, NULL, &pipeLayout);
-  RDCASSERTEQUAL(vkr, VK_SUCCESS);
-
-  SAFE_DELETE_ARRAY(descSetLayouts);
 
   VkGraphicsPipelineCreateInfo pipeCreateInfo;
 
   // get pipeline create info
   MakeGraphicsPipelineInfo(pipeCreateInfo, state.graphics.pipeline);
 
-  // repoint pipeline layout
-  pipeCreateInfo.layout = pipeLayout;
-
   // set primitive topology to point list
   VkPipelineInputAssemblyStateCreateInfo *ia =
       (VkPipelineInputAssemblyStateCreateInfo *)pipeCreateInfo.pInputAssemblyState;
@@ -8153,6 +8135,42 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID)
                    descSet, vertexIndexOffset, drawcall->instanceOffset, numVerts, modSpirv,
                    bufStride);
 
+  {
+    VkDescriptorSetLayout *descSetLayouts;
+
+    // descSet will be the index of our new descriptor set
+    descSetLayouts = new VkDescriptorSetLayout[descSet + 1];
+
+    for(uint32_t i = 0; i < descSet; i++)
+      descSetLayouts[i] = GetResourceManager()->GetCurrentHandle<VkDescriptorSetLayout>(
+          creationInfo.m_PipelineLayout[pipeInfo.layout].descSetLayouts[i]);
+
+    // this layout just says it has one storage buffer
+    descSetLayouts[descSet] = m_MeshFetchDescSetLayout;
+
+    const vector<VkPushConstantRange> &push =
+        creationInfo.m_PipelineLayout[pipeInfo.layout].pushRanges;
+
+    VkPipelineLayoutCreateInfo pipeLayoutInfo = {
+        VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO,
+        NULL,
+        0,
+        descSet + 1,
+        descSetLayouts,
+        (uint32_t)push.size(),
+        push.empty() ? NULL : &push[0],
+    };
+
+    // create pipeline layout with same descriptor set layouts, plus our mesh output set
+    vkr = m_pDriver->vkCreatePipelineLayout(dev, &pipeLayoutInfo, NULL, &pipeLayout);
+    RDCASSERTEQUAL(vkr, VK_SUCCESS);
+
+    SAFE_DELETE_ARRAY(descSetLayouts);
+
+    // repoint pipeline layout
+    pipeCreateInfo.layout = pipeLayout;
+  }
+
   // create vertex shader with modified code
   VkShaderModuleCreateInfo moduleCreateInfo = {
       VK_STRUCTURE_TYPE_SHADER_MODULE_CREATE_INFO, NULL,         0,
@@ -8265,7 +8283,7 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID)
     RDCASSERTEQUAL(vkr, VK_SUCCESS);
 
     // do single draw
-    modifiedstate.BeginRenderPassAndApplyState(cmd);
+    modifiedstate.BeginRenderPassAndApplyState(cmd, VulkanRenderState::BindGraphics);
     ObjDisp(cmd)->CmdDraw(Unwrap(cmd), drawcall->numIndices, drawcall->numInstances,
                           drawcall->vertexOffset, drawcall->instanceOffset);
     modifiedstate.EndRenderPass(cmd);
@@ -8406,7 +8424,7 @@ void VulkanDebugManager::InitPostVSBuffers(uint32_t eventID)
     m_pDriver->vkUpdateDescriptorSets(dev, 1, &write, 0, NULL);
 
     // do single draw
-    modifiedstate.BeginRenderPassAndApplyState(cmd);
+    modifiedstate.BeginRenderPassAndApplyState(cmd, VulkanRenderState::BindGraphics);
     ObjDisp(cmd)->CmdDrawIndexed(Unwrap(cmd), (uint32_t)indices.size(), drawcall->numInstances, 0,
                                  drawcall->baseVertex, drawcall->instanceOffset);
     modifiedstate.EndRenderPass(cmd);

renderdoc/driver/vulkan/vk_info.cpp

@@ -81,6 +81,45 @@ void DescSetLayout::CreateBindingsArray(vector<DescriptorSetSlot *> &descBinding
   }
 }
 
+bool DescSetLayout::operator==(const DescSetLayout &other) const
+{
+  // shortcut for equality to ourselves
+  if(this == &other)
+    return true;
+
+  // descriptor set layouts are different if they have different set of bindings.
+  if(bindings.size() != other.bindings.size())
+    return false;
+
+  // iterate over each binding (we know this loop indexes validly in both arrays
+  for(size_t i = 0; i < bindings.size(); i++)
+  {
+    const Binding &a = bindings[i];
+    const Binding &b = other.bindings[i];
+
+    // if the type/stages/count are different, the layout is different
+    if(a.descriptorCount != b.descriptorCount || a.descriptorType != b.descriptorType ||
+       a.stageFlags != b.stageFlags)
+      return false;
+
+    // if one has immutable samplers but the other doesn't, they're different
+    if((a.immutableSampler && !b.immutableSampler) || (!a.immutableSampler && b.immutableSampler))
+      return false;
+
+    // if we DO have immutable samplers, they must all point to the same sampler objects.
+    if(a.immutableSampler)
+    {
+      for(uint32_t s = 0; s < a.descriptorCount; s++)
+      {
+        if(a.immutableSampler[s] != b.immutableSampler[s])
+          return false;
+      }
+    }
+  }
+
+  return true;
+}
+
 void VulkanCreationInfo::Pipeline::Init(VulkanResourceManager *resourceMan, VulkanCreationInfo &info,
                                         const VkGraphicsPipelineCreateInfo *pCreateInfo)
 {

renderdoc/driver/vulkan/vk_info.h

@@ -71,6 +71,9 @@ struct DescSetLayout
   vector<Binding> bindings;
 
   uint32_t dynamicCount;
+
+  bool operator==(const DescSetLayout &other) const;
+  bool operator!=(const DescSetLayout &other) const { return !(*this == other); }
 };
 
 struct VulkanCreationInfo

renderdoc/driver/vulkan/vk_state.cpp

@@ -23,10 +23,12 @@
  ******************************************************************************/
 
 #include "vk_state.h"
+#include "vk_core.h"
 #include "vk_info.h"
 #include "vk_resources.h"
 
-VulkanRenderState::VulkanRenderState(VulkanCreationInfo *createInfo) : m_CreationInfo(createInfo)
+VulkanRenderState::VulkanRenderState(WrappedVulkan *driver, VulkanCreationInfo *createInfo)
+    : m_CreationInfo(createInfo), m_pDriver(driver)
 {
   compute.pipeline = graphics.pipeline = renderPass = framebuffer = ResourceId();
   compute.descSets.clear();
@@ -43,8 +45,6 @@ VulkanRenderState::VulkanRenderState(VulkanCreationInfo *createInfo) : m_Creatio
   RDCEraseEl(back);
   RDCEraseEl(pushconsts);
 
-  m_ResourceManager = NULL;
-
   renderPass = ResourceId();
   subpass = 0;
 
@@ -83,7 +83,7 @@ VulkanRenderState &VulkanRenderState::operator=(const VulkanRenderState &o)
   return *this;
 }
 
-void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd)
+void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd, PipelineBinding binding)
 {
   RDCASSERT(renderPass != ResourceId());
 
@@ -106,7 +106,7 @@ void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd)
   };
   ObjDisp(cmd)->CmdBeginRenderPass(Unwrap(cmd), &rpbegin, VK_SUBPASS_CONTENTS_INLINE);
 
-  BindPipeline(cmd);
+  BindPipeline(cmd, binding);
 
   if(ibuffer.buf != ResourceId())
     ObjDisp(cmd)->CmdBindIndexBuffer(
@@ -120,9 +120,9 @@ void VulkanRenderState::BeginRenderPassAndApplyState(VkCommandBuffer cmd)
         &vbuffers[i].offs);
 }
 
-void VulkanRenderState::BindPipeline(VkCommandBuffer cmd)
+void VulkanRenderState::BindPipeline(VkCommandBuffer cmd, PipelineBinding binding)
 {
-  if(graphics.pipeline != ResourceId())
+  if(graphics.pipeline != ResourceId() && binding == BindGraphics)
   {
     ObjDisp(cmd)->CmdBindPipeline(
         Unwrap(cmd), VK_PIPELINE_BIND_POINT_GRAPHICS,
@@ -193,6 +193,26 @@ void VulkanRenderState::BindPipeline(VkCommandBuffer cmd)
 
       if(i < graphics.descSets.size() && graphics.descSets[i].descSet != ResourceId())
       {
+        // if we come to a descriptor set that isn't compatible, stop setting descriptor sets from
+        // here on.
+        // We can get into this situation if for example we have many sets bound at some point, then
+        // there's a pipeline change that causes most or all of them to be invalidated as
+        // incompatible, then the program only re-binds some subset that it knows is statically used
+        // by the next drawcall. The remaining sets are invalid, but also unused and this is
+        // explicitly allowed by the spec. We just have to make sure we don't try to actively bind
+        // an incompatible descriptor set.
+        ResourceId createdDescSetLayoutId =
+            m_pDriver->GetDescLayoutForDescSet(graphics.descSets[i].descSet);
+
+        if(descSetLayouts[i] != createdDescSetLayoutId)
+        {
+          const DescSetLayout &createdDescLayout =
+              m_CreationInfo->m_DescSetLayout[createdDescSetLayoutId];
+
+          if(descLayout != createdDescLayout)
+            break;
+        }
+
         // if there are dynamic buffers, pass along the offsets
 
         uint32_t *dynamicOffsets = NULL;
@@ -235,7 +255,7 @@ void VulkanRenderState::BindPipeline(VkCommandBuffer cmd)
     }
   }
 
-  if(compute.pipeline != ResourceId())
+  if(compute.pipeline != ResourceId() && binding == BindCompute)
   {
     ObjDisp(cmd)->CmdBindPipeline(
         Unwrap(cmd), VK_PIPELINE_BIND_POINT_COMPUTE,
@@ -305,3 +325,8 @@ void VulkanRenderState::EndRenderPass(VkCommandBuffer cmd)
 {
   ObjDisp(cmd)->CmdEndRenderPass(Unwrap(cmd));
 }
+
+VulkanResourceManager *VulkanRenderState::GetResourceManager()
+{
+  return m_pDriver->GetResourceManager();
+}

renderdoc/driver/vulkan/vk_state.h

@@ -29,14 +29,22 @@
 
 struct VulkanCreationInfo;
 class VulkanResourceManager;
+class WrappedVulkan;
 
 struct VulkanRenderState
 {
-  VulkanRenderState(VulkanCreationInfo *createInfo);
+  enum PipelineBinding
+  {
+    BindNone = 0x0,
+    BindGraphics = 0x1,
+    BindCompute = 0x2,
+  };
+
+  VulkanRenderState(WrappedVulkan *driver, VulkanCreationInfo *createInfo);
   VulkanRenderState &operator=(const VulkanRenderState &o);
-  void BeginRenderPassAndApplyState(VkCommandBuffer cmd);
+  void BeginRenderPassAndApplyState(VkCommandBuffer cmd, PipelineBinding binding);
   void EndRenderPass(VkCommandBuffer cmd);
-  void BindPipeline(VkCommandBuffer cmd);
+  void BindPipeline(VkCommandBuffer cmd, PipelineBinding binding);
 
   // dynamic state
   vector<VkViewport> views;
@@ -88,7 +96,7 @@ struct VulkanRenderState
   };
   vector<VertBuffer> vbuffers;
 
-  VulkanResourceManager *GetResourceManager() { return m_ResourceManager; }
-  VulkanResourceManager *m_ResourceManager;
+  VulkanResourceManager *GetResourceManager();
   VulkanCreationInfo *m_CreationInfo;
+  WrappedVulkan *m_pDriver;
 };

renderdoc/driver/vulkan/wrappers/vk_descriptor_funcs.cpp

@@ -339,6 +339,8 @@ VkResult WrappedVulkan::vkAllocateDescriptorSets(VkDevice device,
     else
     {
       GetResourceManager()->AddLiveResource(id, pDescriptorSets[i]);
+
+      m_DescriptorSetState[id].layout = GetResID(pAllocateInfo->pSetLayouts[i]);
     }
   }