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

Redesign Block support to make it work with UnmanagedCallersOnlyAttribute #15783

Closed
4 tasks done
rolfbjarne opened this issue Aug 26, 2022 · 6 comments · Fixed by #17794
Closed
4 tasks done

Redesign Block support to make it work with UnmanagedCallersOnlyAttribute #15783

rolfbjarne opened this issue Aug 26, 2022 · 6 comments · Fixed by #17794
Assignees
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement feature A feature to be implemented performance If an issue or pull request is related to performance request-for-comments The issue is a suggested idea seeking feedback
Milestone

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Aug 26, 2022

  • Add support to the BlockLiteral struct to accept function pointers.
  • Add support to the generator for the new block implementation.
  • Add support to our optimizations to handle the new block implementation.
  • Update all our manual binding code to use the new block implementation.

Ref: #10470 (comment)

Ideally any changes would also cover (which I've closed as duplicates of this issue):

@rolfbjarne rolfbjarne added enhancement The issue or pull request is an enhancement performance If an issue or pull request is related to performance dotnet An issue or pull request related to .NET (6) feature A feature to be implemented labels Aug 26, 2022
@rolfbjarne rolfbjarne added this to the .NET 8 milestone Aug 26, 2022
@rolfbjarne
Copy link
Member Author

rolfbjarne commented Oct 19, 2022

Tentative implementation: rolfbjarne@3139dfb
Example update: rolfbjarne@5f41593

This is the important part:

unsafe {
	var trampoline = (delegate* unmanaged<IntPtr, IntPtr, byte, byte>) &TrampolineRegistrationHandlerCallback;
	using var block = new BlockLiteral (trampoline, registrationHandler, typeof (CTFontManager), nameof (TrampolineRegistrationHandlerCallback));
	CTFontManagerRegisterFontURLs (arr.Handle, scope, enabled, &block);
}

it's a bit more verbose than I wished, but I don't think there's a way around that:

  1. There's no way to convert a function to a function pointer using the unmanaged signature anywhere.

    So either this:

    var trampoline = (delegate* unmanaged<IntPtr, IntPtr, byte, byte>) &TrampolineRegistrationHandlerCallback;

    or:

    delegate* unmanaged<IntPtr, IntPtr, byte, byte> trampoline = &TrampolineRegistrationHandlerCallback;

    Both are kind of ugh.

  2. We need to get the signature of the trampoline at runtime (to compute the objc encoding of the signature, which we have to include in the block). We can't reflect on the function pointer (it's just an IntPtr), so we need to pass the MethodInfo of the trampoline method (we can't create a delegate either, because you can't create a delegate of an [UnmanagedCallersOnly] function). In the code sample I'm passing a type and the method name, which turned out slightly smaller than getting the MethodInfo using reflection in code creating the block, but it's not much of a difference:

    unsafe {
    	var trampoline = (delegate* unmanaged<IntPtr, IntPtr, byte, byte>) &TrampolineRegistrationHandlerCallback;
    	using var block = new BlockLiteral (trampoline, registrationHandler, typeof (CTFontManager).GetMethod (nameof (TrampolineRegistrationHandlerCallback), BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public);
    	CTFontManagerRegisterFontURLs (arr.Handle, scope, enabled, &block);
    }

    The linker will compute the signature and replace the reflection with a plain string (of the signature) - this has not been implemented in the POC (it would have to be for a production fix).

@rolfbjarne
Copy link
Member Author

rolfbjarne commented Feb 8, 2023

If dotnet/designs#282 is ever approved and implemented, a minor improvement might be something like this:

unsafe {
	var trampoline = (delegate* unmanaged<IntPtr, IntPtr, byte, byte>) &TrampolineRegistrationHandlerCallback;
    var trampolineType = typeof (delegate* unmanaged<IntPtr, IntPtr, byte, byte>);
	using var block = new BlockLiteral (trampoline, registrationHandler, trampolineType);
	CTFontManagerRegisterFontURLs (arr.Handle, scope, enabled, &block);
}

@rolfbjarne rolfbjarne added the request-for-comments The issue is a suggested idea seeking feedback label Feb 13, 2023
@stephen-hawley
Copy link
Contributor

How gross are we allowed to get in the code? Like, can we assume that a C# delegate is just a struct with two machine pointers in it and pass that through along with the delegate type then on the receiving end of the trampoline hard cast it back and invoke it? I feel like I did something like this in BTfS.

@rolfbjarne
Copy link
Member Author

How gross are we allowed to get in the code? Like, can we assume that a C# delegate is just a struct with two machine pointers in it and pass that through along with the delegate type then on the receiving end of the trampoline hard cast it back and invoke it? I feel like I did something like this in BTfS.

Ideally the code would be nice, because it'll be how users use blocks (which isn't very common, but once in a while it happens). Also, we can't use a delegate to pass the signature, because you can't create a delegate of a method with an [UnmanagedCallersOnly] attribute.

@dalexsoto
Copy link
Member

from the options to step 1 I think I lean more for

delegate* unmanaged<IntPtr, IntPtr, byte, byte> trampoline = &TrampolineRegistrationHandlerCallback;

less parenthesis heh, it feels just slightly more readable to me but just slightly 😃 the rest 👍

@rolfbjarne
Copy link
Member Author

It seems we'll be able to do something like this in the future:

using Signature = delegate* unmanaged<IntPtr, IntPtr, byte, byte>;
unsafe {
	Signature trampoline = &TrampolineRegistrationHandlerCallback;
	using var block = new BlockLiteral (trampoline, registrationHandler, typeof (CTFontManager), nameof (TrampolineRegistrationHandlerCallback));
	CTFontManagerRegisterFontURLs (arr.Handle, scope, enabled, &block);
}

which, together with the function pointer type change, could become:

using Signature = delegate* unmanaged<IntPtr, IntPtr, byte, byte>;
unsafe {
	using var block = new BlockLiteral ((Signature) &trampoline, registrationHandler, typeof (Signature));
	CTFontManagerRegisterFontURLs (arr.Handle, scope, enabled, &block);
}

Ref: dotnet/csharplang#4284

rolfbjarne added a commit that referenced this issue Feb 24, 2023
)

* Make BlockLiteral disposable.

    This also means to modify the cleanup logic so that it's safe to call
    Dispose more than once.

* Create a helper class to create a block for a simple Action delegate.

    This allows us to simplify a good chunk of code.

* Update all block creation to use the new block API, where blocks are
  disposable. This makes the code pattern a lot simpler.

    I've changed all the P/Invokes to use an unsafe 'BlockLiteral*' pointer,
    because 'using' variables can't be passed as ref arguments, so the choice
    was either to make the parameter type 'IntPtr' and cast away the pointer:

        using var block = new BlockLiteral ();
        PInvoke ((IntPtr) &block);

    or make the parameter an unsafe 'BlockLiteral*' pointer:

        unsafe {
            using var block = new BlockLiteral ();
            PInvoke (&block);
        }

    The upcoming support for function pointers don't have this choice:
    function pointers are always unsafe, so I chose to go the unsafe route
    here as well, since it makes the code simpler once support for function
    pointers has been implemented.

Contributes towards #15783.

This PR might be easier to review commit-by-commit.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Feb 24, 2023
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Feb 28, 2023
This is mostly converting 'bool' arguments to 'byte' arguments, and 'string'
arguments to 'IntPtr' with custom utf8->string conversions.

This is necessary in order to convert all block callbacks to use
UnmanagedCallersOnly function pointers (which can't have non-blittable types
in their signature).

Contributes towards xamarin#15783.
rolfbjarne added a commit that referenced this issue Mar 1, 2023
This is mostly converting 'bool' arguments to 'byte' arguments, and 'string'
arguments to 'IntPtr' with custom utf8->string conversions.

This is necessary in order to convert all block callbacks to use
UnmanagedCallersOnly function pointers (which can't have non-blittable types
in their signature).

Contributes towards #15783.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 1, 2023
This is necessary in order to convert all block callbacks to use
UnmanagedCallersOnly function pointers (which can't have non-blittable types
in their signature).

Contributes towards xamarin#15783.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 1, 2023
This is necessary in order to convert all block callbacks to use
UnmanagedCallersOnly function pointers (which can't have non-blittable types
in their signature).

Contributes towards xamarin#15783.
rolfbjarne added a commit that referenced this issue Mar 2, 2023
This is necessary in order to convert all block callbacks to use
UnmanagedCallersOnly function pointers (which can't have non-blittable types
in their signature).

Contributes towards #15783.
rolfbjarne added a commit that referenced this issue Mar 6, 2023
Add support for function pointers to BlockLiteral, and use it to update
almost all manually bound block code to use function pointers (in .NET).

Also add support to the linker for optimizing the new block API.

Contributes towards #15783.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 6, 2023
…ypes.

Change the generated block callbacks so that we only use blittable types (so
that the callbacks can become [UnmanagedCallersOnly]).

This involved two changes:

* Use pointer types instead of ref/out types ('int*' instead of 'ref/out
  int').
* Use 'byte' instead of 'bool'.

Contributes towards xamarin#15783.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 7, 2023
…ypes.

Change the generated block callbacks so that we only use blittable types (so
that the callbacks can become [UnmanagedCallersOnly]).

This involved two changes:

* Use pointer types instead of ref/out types ('int*' instead of 'ref/out
  int').
* Use 'byte' instead of 'bool'.

Contributes towards xamarin#15783.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 7, 2023
…ypes.

Change the generated block callbacks so that we only use blittable types (so
that the callbacks can become [UnmanagedCallersOnly]).

This involved two changes:

* Use pointer types instead of ref/out types ('int*' instead of 'ref/out
  int').
* Use 'byte' instead of 'bool'.

Contributes towards xamarin#15783.
@rolfbjarne rolfbjarne self-assigned this Mar 7, 2023
rolfbjarne added a commit that referenced this issue Mar 7, 2023
…ypes. (#17712)

Change the generated block callbacks so that we only use blittable types
(so that the callbacks can become [UnmanagedCallersOnly]).

This involved two changes:

* Use pointer types instead of ref/out types ('int*' instead of 'ref/out int').
* Use 'byte' instead of 'bool'.

Contributes towards #15783.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 7, 2023
rolfbjarne added a commit that referenced this issue Mar 14, 2023
…n .NET. (#17741)

This also required updating a manual binding since it poked into
generated internals.

Contributes towards #15783.
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 14, 2023
…our own code. Fixes xamarin#15783.

This is the final step in our improved block support.

Fixes xamarin#15783.
rolfbjarne added a commit that referenced this issue Mar 15, 2023
…our own code. Fixes #15783. (#17794)

This is the final step in our improved block support.

Fixes #15783.

Co-authored-by: Manuel de la Pena <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dotnet An issue or pull request related to .NET (6) enhancement The issue or pull request is an enhancement feature A feature to be implemented performance If an issue or pull request is related to performance request-for-comments The issue is a suggested idea seeking feedback
Projects
None yet
3 participants