renderdoc icon indicating copy to clipboard operation
renderdoc copied to clipboard

Fix `vkEnumeratePhysicalDeviceGroups` being non-conformant with Vulkan specification

Open KacperZielinski-Intel opened this issue 11 months ago • 1 comments

Fix for #3261

Description

RenderDoc's vkEnumeratePhysicalDeviceGroups implementation was non-compliant with Vulkan specification.

When vkEnumeratePhysicalDeviceGroups has been called to query less than the total number of available device groups, then pPhysicalDeviceGroupCount parameter was overridden with the total amount of available device groups. However, Vulkan specification says that the amount of device groups written to pPhysicalDeviceGroupProperties should be returned instead.

In such cases, this was causing Vulkan loader (vulkan-1.dll) to crash on preparing vkEnumeratePhysicalDeviceGroups trampoline.

To fix this issue, I have reworked vkEnumeratePhysicalDeviceGroups implementation to be Vulkan specification compliant. I have also made some improvements, like early exit and replacement of raw memory management with rdcarray container.

KacperZielinski-Intel avatar Feb 28 '24 10:02 KacperZielinski-Intel

Unless I'm missing something I think there's a different problem with the new code. From reading your description, it seems like a simpler commit to only change:

  if(pPhysicalDeviceGroupCount)
    *pPhysicalDeviceGroupCount = RDCMIN(outputSpace, numPhys);

Would fix the problem? Since the array handling is fine we just don't clamp the returned output count to the space given by the application.

We cannot apply such a simple fix to the old code, as it don't differentiate cases of possible pPhysicalDeviceGroupCount return values:

  • total number of available device groups; when pPhysicalDeviceGroupProperties == NULL
  • actual number of written physical device groups; when pPhysicalDeviceGroupProperties != NULL

I insist on merging this fix with my other changes to vkEnumeratePhysicalDeviceGroups function. Nevertheless, if you don't like style of my changes, I can try to just emplace fix in original function body.

KacperZielinski-Intel avatar Feb 28 '24 15:02 KacperZielinski-Intel

Ah OK I see what you mean, yes it needs more than the one line fix. I was just trying to clarify which of the changes are necessary or not, as generally speaking a simpler fix is preferable, I wasn't suggesting that I would reject the fix entirely.

baldurk avatar Mar 05 '24 16:03 baldurk