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

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

Closed
Tracked by #80905 ...
rolfbjarne opened this issue Jan 20, 2021 · 10 comments
Assignees
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement iOS Issues affecting iOS macOS Issues affecting macOS performance If an issue or pull request is related to performance
Milestone

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Jan 20, 2021

.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: dotnet/runtime#47177

Using UnmanagedCallersOnlyAttribute for block callbacks is rather complex, and has been moved to a separate issue: #15783 (see #10470 (comment)).

@rolfbjarne rolfbjarne added macOS Issues affecting macOS iOS Issues affecting iOS dotnet An issue or pull request related to .NET (6) labels Jan 20, 2021
@rolfbjarne rolfbjarne added this to the .NET 6 milestone Jan 20, 2021
@spouliot
Copy link
Contributor

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 :)

@rolfbjarne rolfbjarne added dotnet-pri1 .NET 6: important for stable release enhancement The issue or pull request is an enhancement labels Feb 3, 2021
@spouliot
Copy link
Contributor

spouliot commented Feb 3, 2021

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

@rolfbjarne
Copy link
Member Author

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

@spouliot
Copy link
Contributor

spouliot commented Feb 3, 2021

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)

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Oct 4, 2021
…n unmanaged function pointer instead of delegate. Very partial fix for xamarin#10470.

In .NET 6 we can use the [UnmanagedCallersOnly] attribute to mark functions
that are called from native code (instead of the [MonoPInvokeCallback]
attribute). It turns out this is a rather major refactoring (in particular for
blocks), so start by doing it for just a single function, to make sure using
[UnmanagedCallersOnly] at least works somewhere.

Ref: xamarin#10470
@rolfbjarne
Copy link
Member Author

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): #12920

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

@rolfbjarne rolfbjarne modified the milestones: .NET 6, .NET 7 Oct 4, 2021
@rolfbjarne rolfbjarne removed dotnet-pri1 .NET 6: important for stable release estimate-1w labels Oct 4, 2021
rolfbjarne added a commit that referenced this issue Oct 5, 2021
…n unmanaged function pointer instead of delegate. Very partial fix for #10470. (#12920)

In .NET 6 we can use the [UnmanagedCallersOnly] attribute to mark functions
that are called from native code (instead of the [MonoPInvokeCallback]
attribute). It turns out this is a rather major refactoring (in particular for
blocks), so start by doing it for just a single function, to make sure using
[UnmanagedCallersOnly] at least works somewhere.

Ref: #10470
@rolfbjarne rolfbjarne changed the title .NET 6: Use UnmanagedCallersOnlyAttribute instead of (or in addition to) MonoPInvokeCallbackAttribute .NET: Use UnmanagedCallersOnlyAttribute instead of (or in addition to) MonoPInvokeCallbackAttribute Oct 5, 2021
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Oct 6, 2021
…llback] for the ScheduledAudioFileRegionCallback function.

Ref xamarin#10470.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Oct 7, 2021
…okeCallback] for the CGPDFDictionaryApplyFunction function.

I had to change the first argument from 'string' to 'IntPtr', because 'string'
isn't blittable. In order to diverge the code paths as little as possible, I
also made this change for the legacy Xamarin version.

Ref xamarin#10470.
rolfbjarne added a commit that referenced this issue Oct 8, 2021
…okeCallback] for the CGPDFDictionaryApplyFunction function. (#12956)

I had to change the first argument from 'string' to 'IntPtr', because 'string'
isn't blittable. In order to diverge the code paths as little as possible, I
also made this change for the legacy Xamarin version.

Ref #10470.
rolfbjarne added a commit that referenced this issue Oct 8, 2021
…llback] for the ScheduledAudioFileRegionCallback function. (#12948)

Ref #10470.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Oct 8, 2021
…eCallback] for the managed callbacks called from native code.

Ref xamarin#10470.
rolfbjarne added a commit that referenced this issue Dec 17, 2021
…eCallback] for the managed callbacks called from native code. (#13567)

Ref: #10470.
@rolfbjarne rolfbjarne added the performance If an issue or pull request is related to performance label Aug 26, 2022
@rolfbjarne rolfbjarne modified the milestones: .NET 7, .NET 8 Aug 26, 2022
stephen-hawley added a commit to stephen-hawley/xamarin-macios that referenced this issue Aug 30, 2022
stephen-hawley added a commit that referenced this issue Sep 7, 2022
stephen-hawley added a commit that referenced this issue Sep 8, 2022
@stephen-hawley
Copy link
Contributor

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 added a commit that referenced this issue Sep 12, 2022
…eCallback] Partial Fix for #10470 (#15906)

Completed the work for CoreGraphics.
One exception is PDFArray.cs which uses `SetupBlockUnsafe` (noted in the
issue)

Co-authored-by: Manuel de la Pena <[email protected]>
stephen-hawley added a commit that referenced this issue Sep 13, 2022
@stephen-hawley
Copy link
Contributor

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.

@rolfbjarne
Copy link
Member Author

@stephen-hawley is this all done now?

@rolfbjarne
Copy link
Member Author

@stephen-hawley is this all done now?

The generator needs to be updated too: #18685

@rolfbjarne rolfbjarne modified the milestones: .NET 8, .NET 9 Sep 20, 2023
@rolfbjarne
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement iOS Issues affecting iOS macOS Issues affecting macOS performance If an issue or pull request is related to performance
Projects
Status: Done
Development

No branches or pull requests

3 participants