KhronosGroup / MoltenVK

MoltenVK is a Vulkan Portability implementation. It layers a subset of the high-performance, industry-standard Vulkan graphics and compute API over Apple's Metal graphics framework, enabling Vulkan applications to run on macOS, iOS and tvOS.
Apache License 2.0
4.76k stars 419 forks source link

Fixed crashes when using VK_EXT_metal_objects from ARC enabled apps #2179

Closed MennoVink closed 6 months ago

MennoVink commented 6 months ago

Fixing https://github.com/KhronosGroup/MoltenVK/issues/2151

While this fixes ARC enabled apps, it will require non-ARC apps to manually release. I think that is to be expected, how else can the pointers be usable for an app-determined amount of time. Should i add some documentation for this somewhere?

Could you review if there need to be checks for making sure there is any output? For example:

auto* pBuffInfo = (VkExportMetalBufferInfoEXT*)next;
auto* mvkDevMem = (MVKDeviceMemory*)pBuffInfo->memory;
pBuffInfo->mtlBuffer = mvkDevMem->getMTLBuffer();
[pBuffInfo->mtlBuffer retain];

It's not checking if mvkDevMem is even valid before calling getMTLBuffer on it. Consequently i didn't check if getMTLBuffer's result is valid before calling retain on it. I'm assuming this is part of Vulkan's VU specs.

cdavis5e commented 6 months ago

Fixing #2151

While this fixes ARC enabled apps, it will require non-ARC apps to manually release. I think that is to be expected, how else can the pointers be usable for an app-determined amount of time. Should i add some documentation for this somewhere?

Probably. Really, it ought to be documented as part of the VK_EXT_metal_objects extension itself. But since AFAICT you're not a member of Khronos, we'll have to get someone else to update the extension...

Could you review if there need to be checks for making sure there is any output? For example:

auto* pBuffInfo = (VkExportMetalBufferInfoEXT*)next;
auto* mvkDevMem = (MVKDeviceMemory*)pBuffInfo->memory;
pBuffInfo->mtlBuffer = mvkDevMem->getMTLBuffer();
[pBuffInfo->mtlBuffer retain];

It's not checking if mvkDevMem is even valid before calling getMTLBuffer on it. Consequently i didn't check if getMTLBuffer's result is valid before calling retain on it. I'm assuming this is part of Vulkan's VU specs.

Generally speaking, if you were to pass a null handle to a Vulkan function where the VU says that the handle must [original emphasis] be valid, the behavior from that point is undefined, and literally anything is allowed to happen--from the program crashing to the program working fine to the command rm -rf / being executed as root to (as compiler engineers like to say) demons spontaneously flying out of your nose. So if there's a VU saying that VkExportMetalBufferInfoEXT::memory must be valid--which, of course, there is--you can avoid checking for null. Not requiring all these null checks allows Vulkan drivers to run faster--the constant validation that OpenGL implementations were required to perform was a notorious performance bottleneck.

billhollings commented 6 months ago

Replaced by #2200.