Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No error reported when trying to allocate a descriptor from a pool not sized to contain that type #2041

Closed
hrydgard opened this issue Jul 21, 2020 · 7 comments
Assignees
Milestone

Comments

@hrydgard
Copy link
Contributor

When creating a descriptor pool, you specify a series of sizes of various descriptor types, through VkDescriptorPoolCreateInfo.pPoolSizes.

I ran into a case where I didn't specify a VkDescriptorPoolSize { VK_DESCRIPTOR_TYPE_STORAGE_BUFFER , n }, but later went ahead and allocated a descriptor set using a descriptor set layout that had a storage buffer descriptor in it anyway.

A certain vendor's driver just went ahead and returned a working descriptor set, while others crash (which is fine, since usage isn't valid).

Validation layers don't catch this case, which is odd. I guess the validation layers don't want to catch the case where you run out of descriptors since it's valid for the vkAllocateDescriptorSets function to return VK_ERROR_OUT_OF_POOL_MEMORY or even VK_ERROR_FRAGMENTED_POOL. But it really should be able to catch this case of trying to allocate a type that you haven't even specified a size for.

@mark-lunarg
Copy link
Contributor

Weird -- there's a VU for this:

VUID-VkDescriptorPoolSize-descriptorCount-00302: descriptorCount must be greater than 0

...which is implemented in Parameter Validation, and there's a test for it in vklayertests_descriptor_renderpass_framebuffer.cpp. I could see it getting skipped if pPoolSizes was zero, but I'm guessing this is not the case in your situation?

@mark-lunarg mark-lunarg self-assigned this Jul 21, 2020
@jeffbolznv
Copy link
Contributor

I think you mean this one:

  * [[VUID-VkDescriptorSetAllocateInfo-descriptorPool-00307]]
    pname:descriptorPool must: have enough free descriptor capacity
    remaining to allocate the descriptor sets of the specified layouts

This was removed by maintenance1/Vulkan 1.1, and replaced by an optional error to be returned by the driver. So it's valid for drivers to support this case without error (it is valid usage), and it's expected that validation doesn't report an error here (though maybe a bestpractices warning would be nice).

In general, there's no guarantee that validation will catch issues that affect one implementation when run on another implementation.

@hrydgard
Copy link
Contributor Author

hrydgard commented Jul 21, 2020

I mean I'm fine if it doesn't warn if there's not enough capacity, but if you haven't even specified a size for that type, then that's kinda different from the pool running out of them, right? Definitely at the very least a bestpractices warning but I think the VU could be construed to allow erroring on this.

(Regardless of whether having 0 capacity of something is equivalent to not specifying the count at all, not warning on this is very surprising and unexpected behavior).

@mark-lunarg
Copy link
Contributor

If pPoolSizes was > 0 and descriptorCount was zero for a VkDescriptorPoolSize structure, then you should get 00302. I guess a simple repro case or at least some pseudo-code might clarify things a bit.

@hrydgard
Copy link
Contributor Author

hrydgard commented Jul 21, 2020

There was no entry in pPoolSizes for that descriptor type (STORAGE_BUFFER in this case).

I'm arguing that having no entry at all in pPoolSizes for a descriptor type in a pool, and still allocating descriptor sets with descriptors of that type from the pool is suspicious enough for the validation layer to warn, even though the error from running out is now optional. Running out of something you specified in pPoolSizes is one thing, trying to get something that's not even there is another.

@hrydgard
Copy link
Contributor Author

hrydgard commented Jul 21, 2020

Okay so some very shortened pseudo:

VkDescriptorPoolSize poolSizes {
   { UNIFORM_BUFFER, 4 }
};
pool = vkCreateDescriptorPool( info { poolSizes } ).

layout = vkCreateDescriptorSetLayout( { UNIFORM_BUFFER, 1 }  { STORAGE_BUFFER, 1} );

// I'm arguing that this should not be VU since STORAGE_BUFFER was never even specified in poolSizes, even though the requirement of enough space was removed.
descSet = vkAllocateDescriptorSets(pool, {layout})[0];

@mark-lunarg
Copy link
Contributor

@hrydgard, I think that this would be basically equivalent to allocating a zero-sized block of that specific DescriptorType. However, I was unable to find any language in the spec which would disallow this, though it seems obvious. Perhaps you might raise an issue on Vulkan-Docs? In any case, I'll move this over to the Best Practices tracking issue #24. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

4 participants