gpu: Fix instrumentation desc set binding logic

This binding can disturb application binding in some
circumstances
Add SharedPipelineLayoutSubset tests

layers/core_checks/cc_descriptor.cpp

@@ -382,7 +382,7 @@ bool CoreChecks::ValidateCmdBindDescriptorSets(const vvl::CommandBuffer &cb_stat
             const char *vuid = is_2 ? "VUID-VkBindDescriptorSetsInfoKHR-pDescriptorSets-06563"
                                     : "VUID-vkCmdBindDescriptorSets-pDescriptorSets-06563";
             skip |= LogError(vuid, objlist, set_loc,
-                             "(%s) that does not exist, and the layout was not created "
+                             "(%s) does not exist, and the pipeline layout was not created "
                              "VK_PIPELINE_LAYOUT_CREATE_INDEPENDENT_SETS_BIT_EXT.",
                              FormatHandle(pDescriptorSets[set_idx]).c_str());
         }

layers/gpu/cmd_validation/gpuav_cmd_validation_common.cpp

@@ -64,14 +64,14 @@ void RestorablePipelineState::Create(vvl::CommandBuffer &cb_state, VkPipelineBin
     push_constants_data_ = cb_state.push_constant_data_chunks;
 
     descriptor_sets_.reserve(last_bound.per_set.size());
-    for (std::size_t i = 0; i < last_bound.per_set.size(); i++) {
-        const auto &bound_descriptor_set = last_bound.per_set[i].bound_descriptor_set;
+    for (std::size_t set_i = 0; set_i < last_bound.per_set.size(); set_i++) {
+        const auto &bound_descriptor_set = last_bound.per_set[set_i].bound_descriptor_set;
         if (bound_descriptor_set) {
-            descriptor_sets_.emplace_back(bound_descriptor_set->VkHandle(), static_cast<uint32_t>(i));
+            descriptor_sets_.emplace_back(bound_descriptor_set->VkHandle(), static_cast<uint32_t>(set_i));
             if (bound_descriptor_set->IsPushDescriptor()) {
-                push_descriptor_set_index_ = static_cast<uint32_t>(i);
+                push_descriptor_set_index_ = static_cast<uint32_t>(set_i);
             }
-            dynamic_offsets_.push_back(last_bound.per_set[i].dynamicOffsets);
+            dynamic_offsets_.push_back(last_bound.per_set[set_i].dynamicOffsets);
         }
     }
 

layers/gpu/debug_printf/debug_printf.cpp

@@ -600,7 +600,7 @@ void Validator::AllocateDebugPrintfResources(const VkCommandBuffer cmd_buffer, c
     // VkDescriptorSet printf_desc_set = cb_state->gpu_resources_manager.GetManagedDescriptorSet(GetDebugDescriptorSetLayout());
     std::vector<VkDescriptorSet> desc_sets;
     VkDescriptorPool desc_pool = VK_NULL_HANDLE;
-    VkResult result = desc_set_manager_->GetDescriptorSets(1, &desc_pool, GetDebugDescriptorSetLayout(), &desc_sets);
+    VkResult result = desc_set_manager_->GetDescriptorSets(1, &desc_pool, GetInstrumentationDescriptorSetLayout(), &desc_sets);
     if (result != VK_SUCCESS) {
         InternalError(cmd_buffer, loc, "Unable to allocate descriptor sets.");
         return;
@@ -654,15 +654,15 @@ void Validator::AllocateDebugPrintfResources(const VkCommandBuffer cmd_buffer, c
         const auto pipeline_layout_handle = (last_bound.desc_set_pipeline_layout)
                                                 ? last_bound.desc_set_pipeline_layout
                                                 : pipeline_state->PreRasterPipelineLayoutState()->VkHandle();
-        if (pipeline_layout->set_layouts.size() <= desc_set_bind_index_) {
-            DispatchCmdBindDescriptorSets(cmd_buffer, bind_point, pipeline_layout_handle, desc_set_bind_index_, 1, desc_sets.data(),
-                                          0, nullptr);
+        if (pipeline_layout->set_layouts.size() <= instrumentation_desc_set_bind_index_) {
+            DispatchCmdBindDescriptorSets(cmd_buffer, bind_point, pipeline_layout_handle, instrumentation_desc_set_bind_index_, 1,
+                                          desc_sets.data(), 0, nullptr);
         }
     } else {
         // If no pipeline layout was bound when using shader objects that don't use any descriptor set, bind the debug pipeline
         // layout
-        DispatchCmdBindDescriptorSets(cmd_buffer, bind_point, GetDebugPipelineLayout(), desc_set_bind_index_, 1, desc_sets.data(),
-                                      0, nullptr);
+        DispatchCmdBindDescriptorSets(cmd_buffer, bind_point, GetInstrumentationPipelineLayout(),
+                                      instrumentation_desc_set_bind_index_, 1, desc_sets.data(), 0, nullptr);
     }
     // Record buffer and memory info in CB state tracking
     cb_state->buffer_infos.emplace_back(output_block, desc_sets[0], desc_pool, bind_point, cb_state->action_command_count++);

layers/gpu/instrumentation/gpu_shader_instrumentor.cpp

@@ -267,7 +267,7 @@ void GpuShaderInstrumentor::PostCreateDevice(const VkDeviceCreateInfo *pCreateIn
         std::min(gpu::kMaxAdjustedBoundDescriptorSet, phys_dev_props.limits.maxBoundDescriptorSets);
     // If gpu_validation_reserve_binding_slot: the max slot is where we reserved
     // else: always use the last possible set as least likely to be used
-    desc_set_bind_index_ = adjusted_max_desc_sets_limit - 1;
+    instrumentation_desc_set_bind_index_ = adjusted_max_desc_sets_limit - 1;
 
     // We can't do anything if there is only one.
     // Device probably not a legit Vulkan device, since there should be at least 4. Protect ourselves.
@@ -289,7 +289,7 @@ void GpuShaderInstrumentor::PostCreateDevice(const VkDeviceCreateInfo *pCreateIn
                                                                     static_cast<uint32_t>(instrumentation_bindings_.size()),
                                                                     instrumentation_bindings_.data()};
 
-    result = DispatchCreateDescriptorSetLayout(device, &debug_desc_layout_info, nullptr, &debug_desc_layout_);
+    result = DispatchCreateDescriptorSetLayout(device, &debug_desc_layout_info, nullptr, &instrumentation_desc_layout_);
     if (result != VK_SUCCESS) {
         InternalError(device, loc, "vkCreateDescriptorSetLayout failed for internal descriptor set");
         Cleanup();
@@ -306,18 +306,20 @@ void GpuShaderInstrumentor::PostCreateDevice(const VkDeviceCreateInfo *pCreateIn
     }
 
     std::vector<VkDescriptorSetLayout> debug_layouts;
-    for (uint32_t j = 0; j < desc_set_bind_index_; ++j) {
+    for (uint32_t j = 0; j < instrumentation_desc_set_bind_index_; ++j) {
         debug_layouts.push_back(dummy_desc_layout_);
     }
-    debug_layouts.push_back(debug_desc_layout_);
+    // #ARNO_TODO this last push_back looks wrong
+    debug_layouts.push_back(instrumentation_desc_layout_);
+
     const VkPipelineLayoutCreateInfo debug_pipeline_layout_info = {VK_STRUCTURE_TYPE_PIPELINE_LAYOUT_CREATE_INFO,
                                                                    nullptr,
                                                                    0u,
                                                                    static_cast<uint32_t>(debug_layouts.size()),
                                                                    debug_layouts.data(),
                                                                    0u,
                                                                    nullptr};
-    result = DispatchCreatePipelineLayout(device, &debug_pipeline_layout_info, nullptr, &debug_pipeline_layout_);
+    result = DispatchCreatePipelineLayout(device, &debug_pipeline_layout_info, nullptr, &instrumentation_pipeline_layout_);
     if (result != VK_SUCCESS) {
         InternalError(device, loc, "vkCreateDescriptorSetLayout failed for internal pipeline layout");
         Cleanup();
@@ -326,17 +328,18 @@ void GpuShaderInstrumentor::PostCreateDevice(const VkDeviceCreateInfo *pCreateIn
 }
 
 void GpuShaderInstrumentor::Cleanup() {
-    if (debug_desc_layout_) {
-        DispatchDestroyDescriptorSetLayout(device, debug_desc_layout_, nullptr);
-        debug_desc_layout_ = VK_NULL_HANDLE;
+    if (instrumentation_desc_layout_) {
+        DispatchDestroyDescriptorSetLayout(device, instrumentation_desc_layout_, nullptr);
+        instrumentation_desc_layout_ = VK_NULL_HANDLE;
     }
     if (dummy_desc_layout_) {
         DispatchDestroyDescriptorSetLayout(device, dummy_desc_layout_, nullptr);
         dummy_desc_layout_ = VK_NULL_HANDLE;
     }
-    if (debug_pipeline_layout_) {
-        DispatchDestroyPipelineLayout(device, debug_pipeline_layout_, nullptr);
-        debug_pipeline_layout_ = VK_NULL_HANDLE;
+
+    if (instrumentation_pipeline_layout_) {
+        DispatchDestroyPipelineLayout(device, instrumentation_pipeline_layout_, nullptr);
+        instrumentation_pipeline_layout_ = VK_NULL_HANDLE;
     }
 }
 
@@ -440,10 +443,10 @@ void GpuShaderInstrumentor::PreCallRecordCreatePipelineLayout(VkDevice device, c
                                                               VkPipelineLayout *pPipelineLayout, const RecordObject &record_obj,
                                                               chassis::CreatePipelineLayout &chassis_state) {
     if (gpuav_settings.shader_instrumentation_enabled) {
-        if (chassis_state.modified_create_info.setLayoutCount > desc_set_bind_index_) {
+        if (chassis_state.modified_create_info.setLayoutCount > instrumentation_desc_set_bind_index_) {
             std::ostringstream strm;
             strm << "pCreateInfo::setLayoutCount (" << chassis_state.modified_create_info.setLayoutCount
-                 << ") will conflicts with validation's descriptor set at slot " << desc_set_bind_index_ << ". "
+                 << ") will conflicts with validation's descriptor set at slot " << instrumentation_desc_set_bind_index_ << ". "
                  << "This Pipeline Layout has too many descriptor sets that will not allow GPU shader instrumentation to be setup "
                     "for "
                     "pipelines created with it, therefor no validation error will be repored for them by GPU-AV at "
@@ -454,15 +457,15 @@ void GpuShaderInstrumentor::PreCallRecordCreatePipelineLayout(VkDevice device, c
             // 1. Copying the caller's descriptor set desc_layouts
             // 2. Fill in dummy descriptor layouts up to the max binding
             // 3. Fill in with the debug descriptor layout at the max binding slot
-            chassis_state.new_layouts.reserve(desc_set_bind_index_ + 1);
+            chassis_state.new_layouts.reserve(instrumentation_desc_set_bind_index_ + 1);
             chassis_state.new_layouts.insert(chassis_state.new_layouts.end(), &pCreateInfo->pSetLayouts[0],
                                              &pCreateInfo->pSetLayouts[pCreateInfo->setLayoutCount]);
-            for (uint32_t i = pCreateInfo->setLayoutCount; i < desc_set_bind_index_; ++i) {
+            for (uint32_t i = pCreateInfo->setLayoutCount; i < instrumentation_desc_set_bind_index_; ++i) {
                 chassis_state.new_layouts.push_back(dummy_desc_layout_);
             }
-            chassis_state.new_layouts.push_back(debug_desc_layout_);
+            chassis_state.new_layouts.push_back(instrumentation_desc_layout_);
             chassis_state.modified_create_info.pSetLayouts = chassis_state.new_layouts.data();
-            chassis_state.modified_create_info.setLayoutCount = desc_set_bind_index_ + 1;
+            chassis_state.modified_create_info.setLayoutCount = instrumentation_desc_set_bind_index_ + 1;
         }
     }
     BaseClass::PreCallRecordCreatePipelineLayout(device, pCreateInfo, pAllocator, pPipelineLayout, record_obj, chassis_state);
@@ -545,10 +548,10 @@ void GpuShaderInstrumentor::PreCallRecordCreateShadersEXT(VkDevice device, uint3
         VkShaderCreateInfoEXT new_create_info = pCreateInfos[i];
         auto &instrumentation_data = chassis_state.instrumentations_data[i];
 
-        if (new_create_info.setLayoutCount > desc_set_bind_index_) {
+        if (new_create_info.setLayoutCount > instrumentation_desc_set_bind_index_) {
             std::ostringstream strm;
             strm << "pCreateInfos[" << i << "]::setLayoutCount (" << new_create_info.setLayoutCount
-                 << ") will conflicts with validation's descriptor set at slot " << desc_set_bind_index_ << ". "
+                 << ") will conflicts with validation's descriptor set at slot " << instrumentation_desc_set_bind_index_ << ". "
                  << "This Shader Object has too many descriptor sets that will not allow GPU shader instrumentation to be setup "
                     "for VkShaderEXT created with it, therefor no validation error will be repored for them by GPU-AV at "
                     "runtime.";
@@ -558,15 +561,15 @@ void GpuShaderInstrumentor::PreCallRecordCreateShadersEXT(VkDevice device, uint3
             // 1. Copying the caller's descriptor set desc_layouts
             // 2. Fill in dummy descriptor layouts up to the max binding
             // 3. Fill in with the debug descriptor layout at the max binding slot
-            instrumentation_data.new_layouts.reserve(desc_set_bind_index_ + 1);
+            instrumentation_data.new_layouts.reserve(instrumentation_desc_set_bind_index_ + 1);
             instrumentation_data.new_layouts.insert(instrumentation_data.new_layouts.end(), pCreateInfos[i].pSetLayouts,
                                                     &pCreateInfos[i].pSetLayouts[pCreateInfos[i].setLayoutCount]);
-            for (uint32_t j = pCreateInfos[i].setLayoutCount; j < desc_set_bind_index_; ++j) {
+            for (uint32_t j = pCreateInfos[i].setLayoutCount; j < instrumentation_desc_set_bind_index_; ++j) {
                 instrumentation_data.new_layouts.push_back(dummy_desc_layout_);
             }
-            instrumentation_data.new_layouts.push_back(debug_desc_layout_);
+            instrumentation_data.new_layouts.push_back(instrumentation_desc_layout_);
             new_create_info.pSetLayouts = instrumentation_data.new_layouts.data();
-            new_create_info.setLayoutCount = desc_set_bind_index_ + 1;
+            new_create_info.setLayoutCount = instrumentation_desc_set_bind_index_ + 1;
         }
 
         PreCallRecordShaderObjectInstrumentation(new_create_info, record_obj.location.dot(vvl::Field::pCreateInfos, i),
@@ -972,11 +975,11 @@ bool GpuShaderInstrumentor::NeedPipelineCreationShaderInstrumentation(vvl::Pipel
 
     // If the app requests all available sets, the pipeline layout was not modified at pipeline layout creation and the
     // already instrumented shaders need to be replaced with uninstrumented shaders
-    if (pipeline_state.active_slots.find(desc_set_bind_index_) != pipeline_state.active_slots.end()) {
+    if (pipeline_state.active_slots.find(instrumentation_desc_set_bind_index_) != pipeline_state.active_slots.end()) {
         return false;
     }
     const auto pipeline_layout = pipeline_state.PipelineLayoutState();
-    if (pipeline_layout && pipeline_layout->set_layouts.size() > desc_set_bind_index_) {
+    if (pipeline_layout && pipeline_layout->set_layouts.size() > instrumentation_desc_set_bind_index_) {
         return false;
     }
 
@@ -1346,7 +1349,7 @@ bool GpuShaderInstrumentor::InstrumentShader(const vvl::span<const uint32_t> &in
     gpu::spirv::Settings module_settings{};
     // Use the unique_shader_id as a shader ID so we can look up its handle later in the shader_map.
     module_settings.shader_id = unique_shader_id;
-    module_settings.output_buffer_descriptor_set = desc_set_bind_index_;
+    module_settings.output_buffer_descriptor_set = instrumentation_desc_set_bind_index_;
     module_settings.print_debug_info = gpuav_settings.debug_print_instrumentation_info;
     module_settings.max_instrumented_count = gpuav_settings.debug_max_instrumented_count;
     module_settings.support_int64 = enabled_features.shaderInt64;

layers/gpu/instrumentation/gpu_shader_instrumentor.h

@@ -210,10 +210,9 @@ class GpuShaderInstrumentor : public ValidationStateTracker {
     bool InstrumentShader(const vvl::span<const uint32_t> &input_spirv, uint32_t unique_shader_id, bool has_bindless_descriptors,
                           const Location &loc, std::vector<uint32_t> &out_instrumented_spirv);
 
-    VkDescriptorSetLayout GetDebugDescriptorSetLayout() { return debug_desc_layout_; }
-
   public:
-    VkPipelineLayout GetDebugPipelineLayout() { return debug_pipeline_layout_; }
+    VkDescriptorSetLayout GetInstrumentationDescriptorSetLayout() { return instrumentation_desc_layout_; }
+    VkPipelineLayout GetInstrumentationPipelineLayout() { return instrumentation_pipeline_layout_; }
 
     // When aborting we will disconnect all future chassis calls.
     // If we are deep into a call stack, we can use this to return up to the chassis call.
@@ -224,7 +223,9 @@ class GpuShaderInstrumentor : public ValidationStateTracker {
     PFN_vkSetDeviceLoaderData vk_set_device_loader_data_;
     std::atomic<uint32_t> unique_shader_module_id_ = 1;  // zero represents no shader module found
     // The descriptor slot we will be injecting our error buffer into
-    uint32_t desc_set_bind_index_ = 0;
+    uint32_t instrumentation_desc_set_bind_index_ = 0;
+    // This is a layout used to "pad" a pipeline layout to fill in any gaps to the selected bind index
+    VkDescriptorSetLayout dummy_desc_layout_ = VK_NULL_HANDLE;
     VmaAllocator vma_allocator_ = {};
     VmaPool output_buffer_pool_ = VK_NULL_HANDLE;
     std::unique_ptr<DescriptorSetManager> desc_set_manager_;
@@ -239,11 +240,9 @@ class GpuShaderInstrumentor : public ValidationStateTracker {
 
   private:
     void Cleanup();
-    // This is a layout used to "pad" a pipeline layout to fill in any gaps to the selected bind index
-    VkDescriptorSetLayout dummy_desc_layout_ = VK_NULL_HANDLE;
     // These are objects used to inject our descriptor set into the command buffer
-    VkDescriptorSetLayout debug_desc_layout_ = VK_NULL_HANDLE;
-    VkPipelineLayout debug_pipeline_layout_ = VK_NULL_HANDLE;
+    VkDescriptorSetLayout instrumentation_desc_layout_ = VK_NULL_HANDLE;
+    VkPipelineLayout instrumentation_pipeline_layout_ = VK_NULL_HANDLE;
     // Make sure we call the right versions of any timeline semaphore functions.
     bool timeline_khr_{false};