xamarin-macios icon indicating copy to clipboard operation
xamarin-macios copied to clipboard

.NET: Use UnmanagedCallersOnlyAttribute instead of (or in addition to) MonoPInvokeCallbackAttribute

Open rolfbjarne opened this issue 4 years ago • 7 comments

.NET 5/C# 9 has a new UnmanagedCallersOnlyAttribute, which we should start using instead of the MonoPInvokeCallbackAttribute (the main advantage is that the compiler will tell you if you use it wrong, as opposed to the MonoPInvokeCallbackAttribute, where the AOT compiler will crash and not tell you why if you use it wrong).

Ref: https://github.com/dotnet/runtime/issues/47177

Using UnmanagedCallersOnlyAttribute for block callbacks is rather complex, and has been moved to a separate issue: #15783 (see https://github.com/xamarin/xamarin-macios/issues/10470#issuecomment-933443703).

rolfbjarne avatar Jan 20 '21 06:01 rolfbjarne

I'd rather have it instead of (than in addition to) so we're sure we're testing the new attribute and behaviour. It also reduce the amount of final metadata inside the app - even if that should not be a huge size :)

spouliot avatar Jan 20 '21 14:01 spouliot

The old/existing attribute won't be in CoreCLR, which is pri0 https://github.com/xamarin/xamarin-macios/issues/10569 That makes it a pre-requirement to fix this earlier than pri1

spouliot avatar Feb 03 '21 15:02 spouliot

The old/existing attribute won't be in CoreCLR, which is pri0 #10569 That makes it a pre-requirement to fix this earlier than pri1

The MonoPInvokeCallbackAttribute is only required on platforms where we run the AOT compiler. CoreCLR won't be available (at least for now) on any platforms where we need the AOT compiler. Thus I don't think we need to implement this before CoreCLR support (and we define the attribute ourselves).

rolfbjarne avatar Feb 03 '21 15:02 rolfbjarne

I don't recall if all those existing attributes are under #if !MONOMAC or similar conditionals... If so then it's fine, if not then we have to exclude them (which is about the same work as replacing them)

spouliot avatar Feb 03 '21 16:02 spouliot

It turns out this is a rather involved task, in particular when it comes to blocks:

  1. Add support to the BlockLiteral struct to accept function pointers. This is not trivial (might even be easier to create a new type instead of adding support to the existing type).
  2. Add support to the generator for the new block implementation.
  3. Add support to our optimizations to handle the new block implementation.
  4. Update all our manual binding code to use the new block implementation.

In the normal case (where the callback is supposed to be just a plain C function), things are a bit simpler, in that we can port our manual binding code incrementally.

I've ported a single callback (which at least makes sure that using function pointers work): https://github.com/xamarin/xamarin-macios/pull/12920

Since the work for blocks is not trivial, and requires some design and thought, it can be postponed until .NET 7.

rolfbjarne avatar Oct 04 '21 12:10 rolfbjarne

Adding to keep track of incomplete work for using [UnmanagedCallersOnly] The following files use SetupBlockUnsafe (this is not exhaustive) AddressBook CoreText CoreGraphics/PDFArray.cs

stephen-hawley avatar Sep 08 '22 18:09 stephen-hawley

The following packages use SetupBlock or SetupBlockUnsafe: AddressBook - SetupBlockUnsafe CoreText - SetupBlockUnsafe CoreGraphics - PDFArray.cs, SetupBlockUnsafe Metal Network ObjCRuntime UIKit VideoToolbox

ImageIO - mixed usage of callback as delegate and an unmanaged delegate. This is weird.

stephen-hawley avatar Sep 13 '22 13:09 stephen-hawley

@stephen-hawley is this all done now?

rolfbjarne avatar Mar 15 '23 13:03 rolfbjarne

@stephen-hawley is this all done now?

The generator needs to be updated too: https://github.com/xamarin/xamarin-macios/issues/18685

rolfbjarne avatar Aug 10 '23 16:08 rolfbjarne

I believe this is complete now, we're only missing manual P/Invokes (neither the generator nor blocks), which is handled in #15684.

rolfbjarne avatar May 02 '24 11:05 rolfbjarne