Static registrar generates incorrect code for methods that return INativeObjects

[rolfbjarne]

The static registrar generates this code for methods that return INativeObjects:

	if (!retval) {
		res = NULL;
	} else {
		id retobj;
		retobj = xamarin_get_handle_for_inativeobject ((MonoObject *) retval, &exception_gchandle);
		if (exception_gchandle != 0) goto exception_handling;
		xamarin_framework_peer_lock ();
		[retobj retain];
		xamarin_framework_peer_unlock ();
		[retobj autorelease];
		mt_dummy_use (retval);
		res = retobj;
	}

the problem is that the static registrar casts the INativeObject to an id, and assumes it can call [id retain], which isn't always the case.

In particular this causes a crash in the Xamarin.Mac.Tests.CAOpenGLLayerTest test, because the type in question (CGLPixelFormat), is retained/released with custom C functions (CGLRetainPixelFormat/CGLReleasePixelFormat).

Ideas:

• Add a warning or error to the static registrar if it encounters return values that are INativeObject.
• Make all our types that implements INativeObject subclass NativeObject instead.
• Add support to NativeObject to call their Retain/Release implementations.
• Call those Retain/Release implementations from the generated static registrar code.

Cons:

• It's going to be slow to transition to managed code again (twice) for these Retain/Release calls.

Another idea:

• Add an attribute on these types that implement INativeObject or subclass NativeObject that specifies which native methods can be used to retain/release the handle:

[NativeObject (RetainFunction = "CGLRetainPixelFormat", ReleaseFunction = "CGLReleasePixelFormat")]
public class CGLPixelFormat : INativeObject { ... }

as an optimization for the static registrar.

[spouliot]

I wonder how common it is. This one, OpenGL, is deprecated.

The attribute is a pretty good idea 👍

Add a warning or error to the static registrar if it encounters return values that are INativeObject.

We should have an intro test too, so we don't ship those without being reviewed. The warning still makes sense for user code (and bindings).

Make all our types that implements INativeObject subclass NativeObject instead.

We should do it - but for other reasons. It's also a large set of changes.

[retobj autorelease];

To be replaced with CFAutorelease ?

I don't recall any type, that replace CFRetain and CFRelease, providing a replacement for auto release (but it might be because I never checked).

[octopus-russell]

We have hit this issue running OpenGL code build in release with Xamarin.MacOS. Additionally, this Xamarin.Mac sample crashes in release: https://github.com/xamarin/mac-samples/tree/main/OpenGLLayer

While OpenGL is 'deprecated' on MacOS, MacOS ships with OpenGL drivers and OpenGL software functions correctly, even on Apple's latest M1/M2 GPUs.

What would a workaround be for us to run release builds calling CopyCGLPixelFormatForDisplayMask? At the moment, we can't make any working release builds :(

[rolfbjarne]

@octopus-russell the workaround for Release builds is to not use the static registrar. You can do this by passing either --registrar:dynamic or --registrar:partial-static as an additional mmp argument in the project's Mac Build options (for the release configuration). For first option (dynamic) should work, but it's slower than partial-static (which I also believe should work, but I'm not entirely sure).

[rolfbjarne]

Looks like I fixed this here: 7d229665c88b692bcd28bbf3992b498820b0ccf8.