Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when using VK_EXT_metal_objects under ARC. #2200

Merged

Conversation

billhollings
Copy link
Contributor

@billhollings billhollings commented Apr 4, 2024

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.

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.
Copy link
Collaborator

@cdavis5e cdavis5e left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work--the compiler should carry the __unsafe_unretained qualifier as part of the typedef over to the struct. But I think it would be prudent to wait until @MennoVink has a chance to test this.

@MennoVink
Copy link
Contributor

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

@billhollings
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

billhollings commented Apr 6, 2024

@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.

@billhollings billhollings merged commit 3f6a3c2 into KhronosGroup:main Apr 6, 2024
5 checks passed
@billhollings billhollings deleted the VK_EXT_metal_objects-ARC branch April 6, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VK_EXT_metal_objects crashes (reference counting?)
3 participants