search

KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.75k stars 462 forks source link

vkGetPipelinePropertiesEXT should not take a VkBaseOutStructure* #2405

Open Waffl3x opened 1 month ago

Waffl3x commented 1 month ago

https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_pipeline_properties.html#_issues https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkGetPipelinePropertiesEXT.html#_c_specification

The API as it currently is requires the user of the command to reinterpret_cast to VkBaseOutStructure (or static_cast<VkBaseOutStructure>(static_cast<void*>(p)) but that's a psycho move), while this is not UB per the standard, it's still a confusing API. Furthermore, it's easy to misuse as one might think to use a VkBaseOutStructure and put the other structs in the pNext of it. I don't think that's actually a terrible idea, but that's simply not how it's currently specified to work.

In my opinion, there should be another explicit struct that is used for this purpose, and all the other structs be chained in through it's pNext member. It could also just be changed to void*, which might look like a downgrade, but it's more true to how the API actually works, and doesn't require a cast to actually call the function.

While there is no UB invoked on the user side of the API here (converting the pointer isn't UB, dereferencing it as an unrelated type would be), it gets pretty close to it. As a side note, the usage of VkBaseOutStructure and VkBaseInStructure in the loader is UB, which I intend to fix at some point, (type punning with memcpy generally does emit equivalent code and is well defined.) While this case is still well-defined, I don't think the spec should be approaching mandating UB at the spec level, especially when there are alternatives that are arguably superior.

I argue that this is approaching UB as the driver dereferencing the pointer as a VkBaseOutStructure* would be UB, this is super unintuitive. I admit that since this crosses API boundaries, the compiler on the user side will never be able to perceive the UB, the compiler on the driver side will be able to though.

oddhack commented 1 month ago

Do you mean "undefined behavior" by "UB"?

Waffl3x commented 4 weeks ago

Yes, that's correct, sorry I'm pretty used to just abbreviating it.

dgkoch commented 3 weeks ago

This was an intentional choice and is discussed in issue (2) of the extension appendix:

(2) This is intended to be a general pipeline properties query, but is currently only retrieving the pipeline identifier. Should the pipeline identifier query be mandatory for this extension and for all queries using this command?

RESOLVED: Use VkBaseOutStructure for the return parameter. Currently this is required to actually be a VkPipelinePropertiesIdentifierEXT structure, but that could be relaxed in the future to allow other structure types or to allow other structures to be chained in along with this one.

We wanted vkGetPipelinePropertiesEXT to be usable for other properties in the future without requiring a pipeline identifier to queried in all cases, or going through the hassle of chaining through a dummy structure just for extensibility. Quite frankly - if we'd had VkBaseIn/OutStructure in the API from the beginning, we might have done this more.

But, regardless of philosophical opinions on this, we can't change this extension at this point, although we could reconsider if this is ever promoted.

Waffl3x commented 3 weeks ago

Yes, I read the reasoning behind it, I understand why it was decided and the reasoning makes sense, I just don't think the conclusion was correct. Hopefully it will be reconsidered if it is ever considered for promotion. I was hoping that since it was not ratified it would be open to change at this point still. I don't really understand why it can't be changed though as it wouldn't be a breaking change on an ABI level as far as I know, actually I guess it does break linking maybe? Not really sure, maybe it really is best to leave it as it is for now.

TomOlson commented 2 weeks ago

@Waffl3x, since you mentioned ratification - ratification status really only tells you whether an extension is covered by the Khronos IP policy. That matters to GPU and device vendors, but typically not to anyone else, which is why we don't talk about it much. We try very hard not to make compatibility-breaking changes in a published extension spec, whether it's ratified or not. So the only point at which we could make the type of change you're talking about would be if we promoted the extension to KHR or core.