layers: Free commandbuffer data when deleting pool
Fixes for https://vulkan.lunarg.com/issue/view/59eeea8166315153977b0fe6
Issue #720 - Possible crash in layers when deleting command pools.
Ensured pool commandbuffers fully freed in the layer before deleting the
pool using refactored common code from FreeCommandBuffers.
Added postive test which exercises the implicit free path.
Change-Id: I71e1fcb6ffcd9a2977a89c0a8b388639e9a743cb
diff --git a/layers/core_validation.cpp b/layers/core_validation.cpp
index 8431972..429d851 100644
--- a/layers/core_validation.cpp
+++ b/layers/core_validation.cpp
@@ -4088,6 +4088,24 @@
return skip;
}
+// Free all command buffers in given list, removing all references/links to them using resetCB
+static void FreeCommandBufferStates(layer_data *dev_data, COMMAND_POOL_NODE *pool_state, const uint32_t command_buffer_count,
+ const VkCommandBuffer *command_buffers) {
+ for (uint32_t i = 0; i < command_buffer_count; i++) {
+ auto cb_state = GetCBNode(dev_data, command_buffers[i]);
+ // Remove references to command buffer's state and delete
+ if (cb_state) {
+ // reset prior to delete, removing various references to it.
+ // TODO: fix this, it's insane.
+ resetCB(dev_data, cb_state->commandBuffer);
+ // Remove the cb_state's references from layer_data and COMMAND_POOL_NODE
+ dev_data->commandBufferMap.erase(cb_state->commandBuffer);
+ pool_state->commandBuffers.erase(command_buffers[i]);
+ delete cb_state;
+ }
+ }
+}
+
VKAPI_ATTR void VKAPI_CALL FreeCommandBuffers(VkDevice device, VkCommandPool commandPool, uint32_t commandBufferCount,
const VkCommandBuffer *pCommandBuffers) {
layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(device), layer_data_map);
@@ -4105,20 +4123,7 @@
if (skip) return;
auto pPool = GetCommandPoolNode(dev_data, commandPool);
- for (uint32_t i = 0; i < commandBufferCount; i++) {
- auto cb_node = GetCBNode(dev_data, pCommandBuffers[i]);
- // Delete CB information structure, and remove from commandBufferMap
- if (cb_node) {
- // reset prior to delete for data clean-up
- // TODO: fix this, it's insane.
- resetCB(dev_data, cb_node->commandBuffer);
- dev_data->commandBufferMap.erase(cb_node->commandBuffer);
- delete cb_node;
- }
-
- // Remove commandBuffer reference from commandPoolMap
- pPool->commandBuffers.remove(pCommandBuffers[i]);
- }
+ FreeCommandBufferStates(dev_data, pPool, commandBufferCount, pCommandBuffers);
lock.unlock();
dev_data->dispatch_table.FreeCommandBuffers(device, commandPool, commandBufferCount, pCommandBuffers);
@@ -4164,49 +4169,40 @@
return result;
}
-static bool PreCallValidateDestroyCommandPool(layer_data *dev_data, VkCommandPool pool, COMMAND_POOL_NODE **cp_state) {
- *cp_state = GetCommandPoolNode(dev_data, pool);
+static bool PreCallValidateDestroyCommandPool(layer_data *dev_data, VkCommandPool pool) {
+ COMMAND_POOL_NODE *cp_state = GetCommandPoolNode(dev_data, pool);
if (dev_data->instance_data->disabled.destroy_command_pool) return false;
bool skip = false;
- if (*cp_state) {
+ if (cp_state) {
// Verify that command buffers in pool are complete (not in-flight)
- skip |= checkCommandBuffersInFlight(dev_data, *cp_state, "destroy command pool with", VALIDATION_ERROR_24000052);
+ skip |= checkCommandBuffersInFlight(dev_data, cp_state, "destroy command pool with", VALIDATION_ERROR_24000052);
}
return skip;
}
-static void PostCallRecordDestroyCommandPool(layer_data *dev_data, VkCommandPool pool, COMMAND_POOL_NODE *cp_state) {
- // Must remove cmdpool from cmdpoolmap, after removing all cmdbuffers in its list from the commandBufferMap
- for (auto cb : cp_state->commandBuffers) {
- auto cb_node = GetCBNode(dev_data, cb);
- clear_cmd_buf_and_mem_references(dev_data, cb_node);
- // Remove references to this cb_node prior to delete
- // TODO : Need better solution here, resetCB?
- for (auto obj : cb_node->object_bindings) {
- removeCommandBufferBinding(dev_data, &obj, cb_node);
- }
- for (auto framebuffer : cb_node->framebuffers) {
- auto fb_state = GetFramebufferState(dev_data, framebuffer);
- if (fb_state) fb_state->cb_bindings.erase(cb_node);
- }
- dev_data->commandBufferMap.erase(cb); // Remove this command buffer
- delete cb_node; // delete CB info structure
+static void PostCallRecordDestroyCommandPool(layer_data *dev_data, VkCommandPool pool) {
+ COMMAND_POOL_NODE *cp_state = GetCommandPoolNode(dev_data, pool);
+ // Remove cmdpool from cmdpoolmap, after freeing layer data for the command buffers
+ // "When a pool is destroyed, all command buffers allocated from the pool are freed."
+ if (cp_state) {
+ // Create a vector, as FreeCommandBufferStates deletes from cp_state->commandBuffers during iteration.
+ std::vector<VkCommandBuffer> cb_vec{cp_state->commandBuffers.begin(), cp_state->commandBuffers.end()};
+ FreeCommandBufferStates(dev_data, cp_state, static_cast<uint32_t>(cb_vec.size()), cb_vec.data());
+ dev_data->commandPoolMap.erase(pool);
}
- dev_data->commandPoolMap.erase(pool);
}
// Destroy commandPool along with all of the commandBuffers allocated from that pool
VKAPI_ATTR void VKAPI_CALL DestroyCommandPool(VkDevice device, VkCommandPool commandPool, const VkAllocationCallbacks *pAllocator) {
layer_data *dev_data = GetLayerDataPtr(get_dispatch_key(device), layer_data_map);
- COMMAND_POOL_NODE *cp_state = nullptr;
unique_lock_t lock(global_lock);
- bool skip = PreCallValidateDestroyCommandPool(dev_data, commandPool, &cp_state);
+ bool skip = PreCallValidateDestroyCommandPool(dev_data, commandPool);
if (!skip) {
lock.unlock();
dev_data->dispatch_table.DestroyCommandPool(device, commandPool, pAllocator);
lock.lock();
if (commandPool != VK_NULL_HANDLE) {
- PostCallRecordDestroyCommandPool(dev_data, commandPool, cp_state);
+ PostCallRecordDestroyCommandPool(dev_data, commandPool);
}
}
}
@@ -5074,7 +5070,7 @@
if (pPool) {
for (uint32_t i = 0; i < pCreateInfo->commandBufferCount; i++) {
// Add command buffer to its commandPool map
- pPool->commandBuffers.push_back(pCommandBuffer[i]);
+ pPool->commandBuffers.insert(pCommandBuffer[i]);
GLOBAL_CB_NODE *pCB = new GLOBAL_CB_NODE;
// Add command buffer to map
dev_data->commandBufferMap[pCommandBuffer[i]] = pCB;
diff --git a/layers/core_validation_types.h b/layers/core_validation_types.h
index 120be0a..7dd2992 100644
--- a/layers/core_validation_types.h
+++ b/layers/core_validation_types.h
@@ -93,8 +93,8 @@
struct COMMAND_POOL_NODE : public BASE_NODE {
VkCommandPoolCreateFlags createFlags;
uint32_t queueFamilyIndex;
- // TODO: why is this std::list?
- std::list<VkCommandBuffer> commandBuffers; // container of cmd buffers allocated from this pool
+ // Cmd buffers allocated from this pool
+ std::unordered_set<VkCommandBuffer> commandBuffers;
};
// Generic wrapper for vulkan objects
diff --git a/tests/layer_validation_tests.cpp b/tests/layer_validation_tests.cpp
index 6d7a73f..e0045e2 100644
--- a/tests/layer_validation_tests.cpp
+++ b/tests/layer_validation_tests.cpp
@@ -20325,6 +20325,58 @@
m_errorMonitor->VerifyNotFound();
}
+TEST_F(VkPositiveLayerTest, CommandPoolDeleteWithReferences) {
+ TEST_DESCRIPTION("Ensure the validation layers bookkeeping tracks the implicit command buffer frees.");
+ ASSERT_NO_FATAL_FAILURE(Init());
+
+ VkCommandPoolCreateInfo cmd_pool_info = {};
+ cmd_pool_info.sType = VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO;
+ cmd_pool_info.pNext = NULL;
+ cmd_pool_info.queueFamilyIndex = m_device->graphics_queue_node_index_;
+ cmd_pool_info.flags = VK_COMMAND_POOL_CREATE_RESET_COMMAND_BUFFER_BIT;
+ cmd_pool_info.flags = 0;
+
+ VkCommandPool secondary_cmd_pool;
+ VkResult res = vkCreateCommandPool(m_device->handle(), &cmd_pool_info, NULL, &secondary_cmd_pool);
+ ASSERT_VK_SUCCESS(res);
+
+ VkCommandBufferAllocateInfo cmdalloc = vk_testing::CommandBuffer::create_info(secondary_cmd_pool);
+ cmdalloc.level = VK_COMMAND_BUFFER_LEVEL_SECONDARY;
+
+ VkCommandBuffer secondary_cmds;
+ res = vkAllocateCommandBuffers(m_device->handle(), &cmdalloc, &secondary_cmds);
+
+ VkCommandBufferInheritanceInfo cmd_buf_inheritance_info = {};
+ cmd_buf_inheritance_info.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_INHERITANCE_INFO;
+ cmd_buf_inheritance_info.pNext = NULL;
+ cmd_buf_inheritance_info.renderPass = VK_NULL_HANDLE;
+ cmd_buf_inheritance_info.subpass = 0;
+ cmd_buf_inheritance_info.framebuffer = VK_NULL_HANDLE;
+ cmd_buf_inheritance_info.occlusionQueryEnable = VK_FALSE;
+ cmd_buf_inheritance_info.queryFlags = 0;
+ cmd_buf_inheritance_info.pipelineStatistics = 0;
+
+ VkCommandBufferBeginInfo secondary_begin = {};
+ secondary_begin.sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO;
+ secondary_begin.pNext = NULL;
+ secondary_begin.flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;
+ secondary_begin.pInheritanceInfo = &cmd_buf_inheritance_info;
+
+ res = vkBeginCommandBuffer(secondary_cmds, &secondary_begin);
+ ASSERT_VK_SUCCESS(res);
+ vkEndCommandBuffer(secondary_cmds);
+
+ m_commandBuffer->begin();
+ vkCmdExecuteCommands(m_commandBuffer->handle(), 1, &secondary_cmds);
+ m_commandBuffer->end();
+
+ // DestroyCommandPool *implicitly* frees the command buffers allocated from it
+ vkDestroyCommandPool(m_device->handle(), secondary_cmd_pool, NULL);
+ // If bookkeeping has been lax, validating the reset will attempt to touch deleted data
+ res = vkResetCommandPool(m_device->handle(), m_commandPool->handle(), 0);
+ ASSERT_VK_SUCCESS(res);
+}
+
TEST_F(VkPositiveLayerTest, SecondaryCommandBufferClearColorAttachments) {
TEST_DESCRIPTION("Create a secondary command buffer and record a CmdClearAttachments call into it");
ASSERT_NO_FATAL_FAILURE(Init());