search

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.64k stars 402 forks source link

Fix crash when using VK_EXT_metal_objects under ARC. #2200

Closed billhollings closed 3 months ago

billhollings commented 3 months ago

Apple's Automatic Reference Counting automatically releases the Metal objects returned by VK_EXT_metal_objects.

The fetchDependencies script now applies Templates/Vulkan-Headers/VK_EXT_metal_objects-unret.gitdiff to add an __unsafe_unretained ownership qualifier to the Metal object declarations in vulkan_metal.h.

This should be a temporary patch until the VK_EXT_metal_objects extension can be properly modified.

Fixes #2151. Replaces #2179.

@MennoVink Please review.

MennoVink commented 3 months ago

I will check tomorrow, today was an OpenGL work day ;)

billhollings commented 3 months ago

I will check tomorrow, today was an OpenGL work day ;)

Great, thanks.

I have tested this on a modified MoltenVK Cube demo that was derived from the problem discussion in issue #2151. With ARC, it crashed as per the discussion, and this patch fixed it. And all remains good with ARC.

I also now have a PR to modify the VK_EXT_metal_objects extension ready for submission to Khronos, once we determine this works.

MennoVink commented 3 months ago

This fixes it for ARC. Whenever i copy the MTLCommandQueue_id into a id< MTLCommandQueue > ARC automatically retains a reference now so from there the ref counting is fine again.

Note though that it does mean that extra care needs to be taken for non-ARC users. My framework operates under both modes depending on how the client compiles it. In my non-ARC path i was taking care of the implicit __strong reference by transferring the reference (aka releasing it later). This matches this part:

and callers must balance that +1 by releasing or transferring them

I havn't released a version of my framework yet that uses this extension, so i can fix the non-ARC path just fine. I do think this change does mean a breaking change for non-ARC users that were handing the implicit strong annotation correctly though. I need to do the non-ARC testing still but i assume i now need to manually retain the returned id's before i return them from my utility functions that dont use the unsafe_unretained annotation.

So in short i dont think this is an optimal solution but i approve of this MR anyway because now the problem is in non-ARC land where i do have the ability to fix it manually.

billhollings commented 3 months ago

@MennoVink Thanks for the feedback.

I havn't released a version of my framework yet that uses this extension, so i can fix the non-ARC path just fine. I do think this change does mean a breaking change for non-ARC users that were handing the implicit __strong annotation correctly though. I need to do the non-ARC testing still but i assume i now need to manually retain the returned id's before i return them from my utility functions that don't use the __unsafe_unretained annotation.

I did test my mods to the Cube demo under both ARC and non-ARC builds. Under either ARC or non-ARC builds, I could not find any premature deallocation, or memory leaks.

The addition of __unsafe_unretained should affect only the ARC builds. Since MoltenVK itself is non-ARC, nothing changes there, and the retain counts will be exactly the same as they always have been. I might be missing something in your framework structure, but I expect if your non-ARC framework build was working fine before, it should continue to work fine with the __unsafe_unretained change, since if everything is non-ARC, nothing will be paying attention to the change.

But of course, let me know if this change does indeed force changes to your non-ARC builds.