vkGetDeviceProcAddr allows returning of non-device functions

[roderickc]

When working on a Vulkan ICD implementation for Wine, I encountered issues related to vkGetDeviceProcAddr. The call is intended to load functions for device or children of device. I noticed Doom and Wolfenstein 2 both used vkGetDeviceProcAddr to load also instance functions (e.g. vkGetPhysicalDeviceSurfaceSupportKHR). Doing some further testing it appears any instance function can be returned (vkCreateDevice, vkDestroyInstance,..)

This behaviour is not correct. I'm not sure if the loader's vkGetDeviceProcAddr can be fixed as 2 major titles as I pointed out already rely on the broken behaviour. My feeling is that at minimum some validation layers / conformance tests should warn developers about this.

[polarina]

I believe tests against this behaviour should be added to the CTS.

[krOoze]

@polarina It is not neccessarily obvious from the spec that implementations are not allowed to return a function pointer. It only clearly says that you are not allowed to use the given function pointer with a VkInstance. Might be an uncareful wording though — it protects against non-enabled extension commands, so there is no reason for it to not also protect against VkInstance level commands.

[lenny-lunarg]

While I definitely agree that the behavior you're describing seems a little strange, my reading of the spec makes me think this is intended. The spec gives the following table to describe the behavior of vkGetDeviceProcAddr:

device	pName	return value
NULL	*	undefined
invalid device	*	undefined
device	NULL	undefined
device	core vulkan command	fp
device	enabled extension commands	fp
device	* (any pName not covered above)	NULL

The fourth row of that table suggests to me that every core Vulkan command should be accessible through vkGetDeviceProcAddr. That being said, the table is also followed by this note:

The returned function pointer must only be called with a dispatchable object (the first parameter) that is device or a child of device. e.g. VkDevice, VkQueue, or VkCommandBuffer.

That note is completely impossible to adhere to for any instance commands, which looks like a contradiction in the spec to me (though you can still call the commands with a parent object, rather than a child). I think that the intent of the spec is that vkGetDeviceProcAddr can be used on any core Vulkan command, but it's very possible I'm wrong.

My feeling is that I'm not willing to change the behavior of the loader (and the same goes for validation layers) without at least a clarification from the spec. If you want to get a change or clarification in this behavior, then feel free to submit an issue to the spec repo. If the spec (or the spec authors) unambiguously state that instance commands should not be accessible via vkGetDeviceProcAddr, then we'll definitely be willing to adjust our tools accordingly. Like you mentioned, it might be too late to change behavior regardless, but I would think that a validation check would be reasonable.

I'm going to close this issue for now because I don't think there's anything actionable right now. If either the spec changes or if you can find something to unambiguously state that getting instance level commands from vkGetDeviceProcAddr is incorrect, then post here and I'd be happy to reopen the issue. But until then, I think it's best to leave this issue closed.

[roderickc]

Thanks for your feedback. I understand your position and the specification isn't exactly clear. I filed a spec ticket for reference: KhronosGroup/Vulkan-Docs#655

[Novum]

Please don't change the loader in a way that breaks existing shipping games like Wolfenstein. This might require bug for bug compatibility.

I will check our code and fix this, but it probably won't be patched for Doom/Wolf.

[lenny-lunarg]

Thanks, I'll try to keep an eye on the spec issue. I definitely agree with @Novum that we can't break compatibility. That means that if we get a response that vkGetDeviceProcAddr shouldn't return instance functionality, we'll probably leave the functionality in the loader, but add a validation check or something like that.

[krOoze]

@lenny-lunarg Actually, either way it is undefined behaviour of those apps. Either they are calling nullptr functions, or they are calling function pointers they are strictly prohibited to use with instance level objects. I don't think you owe them anything, except maybe being nice and provide some time-buffer in which they can patch their apps.

Receiving function pointer you are not permited to use is pointless, and perhaps so not a worry for compatibility.
Unless, you know: https://xkcd.com/1172/

[Novum]

Yeah, no, this is not remotely comparable to that xkcd. Fixing this by returning NULL would break Doom and Wolf New Order to run. For no good reason besides being anal about the spec.

[krOoze]

@Novum Just sayin', it is their bug regardles of how the spec issue is resolved. There's no compatibility issue as far as spec analness goes (unless you count that it "breaks someones workflow who uses a bug\undefined value"). The Loader code should not be responsible for forever supporting some external non-compliant code.

[lenny-lunarg]

Fixing this by returning NULL would break Doom and Wolf New Order to run. For no good reason besides being anal about the spec.

And even then I'm not at all sure that the spec outlaws this behavior. Regardless, we'll make sure compatibility is preserved.

[lenny-lunarg]

Just an update on this issue. The issue on the spec repo got closed with the result being that vkGetDeviceProcAddr should return null for instance-level commands. As a result, the behavior that @roderickc was expecting is correct and the current behavior is wrong.

That being said, I looked into this issue and there are a few things that I found:

• The pointers being returned are invalid. They are direct pointers into the driver that don't properly unwrap instances and physical devices, which means trying to use them will result in a crash.
• Wolfenstein was acquiring, but not using the pointers. This explains why it doesn't choke on the invalid pointers.
• The pointers are actually coming from the drivers, and the behavior reported here is driver dependent. While the loader could have some code to explicitly forbid getting those pointers from the driver, it will behave correctly if the driver behaves correctly.

As a result, we came to the conclusion that this will be resolved via driver fixes. I'm leaving this issue closed, since there's nothing to do in the loader. That being said, this will get fixed as a result of driver updates, but I can't give a time frame for that, since it's up to the vendors.