Skip to content

Commit

Permalink
[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action …
Browse files Browse the repository at this point in the history
…delegate. (#11560)

* CoreCLR doesn't support generic Action delegates in reverse (P/Invokes), so
  we need to find a different solution.
* The native CGPDFOperatorTable callback API is not very friendly to us,
  because we can't pass it callback-specific data, which means that the
  managed caller must conform to the FullAOT requirement of the managed
  callback (must be a static function; must have a MonoPInvokeCallback
  attribute).
* We can leverage the new function pointer syntax in C# 9 to make these
  requirements enforced by the C# compiler (unmanaged function pointer +
  UnmanagedCallersOnly attribute). The resulting API is still clunky to use,
  but I don't see any way around that.

Fixes this monotouch-test failure with CoreCLR:

    [FAIL] Tamarin : System.Runtime.InteropServices.MarshalDirectiveException : Cannot marshal 'parameter #3': Non-blittable generic types cannot be marshaled.
               at CoreGraphics.CGPDFOperatorTable.CGPDFOperatorTableSetCallback(IntPtr table, String name, Action`2 callback)
               at CoreGraphics.CGPDFOperatorTable.SetCallback(String name, Action`2 callback)
               at MonoTouchFixtures.CoreGraphics.PDFScannerTest.Tamarin() in xamarin-macios/tests/monotouch-test/CoreGraphics/PDFScannerTest.cs:line 102

Ref: dotnet/runtime#32963
  • Loading branch information
rolfbjarne committed May 17, 2021
1 parent ab14e07 commit e8f2672
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 2 deletions.
34 changes: 32 additions & 2 deletions src/CoreGraphics/CGPDFOperatorTable.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,35 @@ protected virtual void Dispose (bool disposing)

public IntPtr Handle { get; private set; }

// We need this P/Invoke for legacy AOT scenarios (since we have public API taking a 'Action<IntPtr, IntPtr>', and with this particular native function we can't wrap the delegate)
// Unfortunately CoreCLR doesn't support generic Action delegates in P/Invokes: https://github.com/dotnet/runtime/issues/32963
[DllImport (Constants.CoreGraphicsLibrary)]
extern static void CGPDFOperatorTableSetCallback (/* CGPDFOperatorTableRef */ IntPtr table, /* const char */ string name, /* CGPDFOperatorCallback */ Action<IntPtr,IntPtr> callback);

#if NET
// This signature requires C# 9 (so .NET only).
// The good part about this signature is that it enforces at compile time that 'callback' is callable from native code in a FullAOT scenario.
// The bad part is that it's unsafe code (and callers must be in unsafe mode as well).
[DllImport (Constants.CoreGraphicsLibrary)]
unsafe extern static void CGPDFOperatorTableSetCallback (/* CGPDFOperatorTableRef */ IntPtr table, /* const char */ string name, /* CGPDFOperatorCallback */ delegate* unmanaged<IntPtr, IntPtr, void> callback);
#endif

#if MONOMAC
// This signature can work everywhere, but we can't enforce at compile time that 'callback' is a delegate to a static function (which is required for FullAOT scenarios),
// so limit it to non-FullAOT platforms (macOS)
[DllImport (Constants.CoreGraphicsLibrary)]
extern static void CGPDFOperatorTableSetCallback (/* CGPDFOperatorTableRef */ IntPtr table, /* const char */ string name, /* CGPDFOperatorCallback */ CGPDFOperatorCallback callback);

// this won't work with AOT since the callback must be decorated with [MonoPInvokeCallback]
public void SetCallback (string name, Action<CGPDFScanner,object> callback)
{
if (name == null)
throw new ArgumentNullException ("name");

if (callback == null)
CGPDFOperatorTableSetCallback (Handle, name, null);
CGPDFOperatorTableSetCallback (Handle, name, (CGPDFOperatorCallback) null);
else
CGPDFOperatorTableSetCallback (Handle, name, new Action<IntPtr, IntPtr> (delegate (IntPtr reserved, IntPtr gchandle) {
CGPDFOperatorTableSetCallback (Handle, name, new CGPDFOperatorCallback (delegate (IntPtr reserved, IntPtr gchandle) {
// we could do 'new CGPDFScanner (reserved, true)' but we would not get `UserInfo` (managed state) back
// we could GCHandle `userInfo` but that would (even more) diverge both code bases :(
var scanner = GetScannerFromInfo (gchandle);
Expand All @@ -94,7 +109,11 @@ public void SetCallback (string name, Action<CGPDFScanner,object> callback)

[Advice ("Use the nicer SetCallback(string,Action<CGPDFScanner,object>) API when possible.")]
#endif

// this API is ugly - but I do not see a better way with the AOT limitation
#if NET && !MONOMAC
[Obsolete ("Use the overload that takes a function pointer ('delegate*<IntPtr,IntPtr,void>') instead.")]
#endif
public void SetCallback (string name, Action<IntPtr,IntPtr> callback)
{
if (name == null)
Expand All @@ -103,6 +122,17 @@ public void SetCallback (string name, Action<IntPtr,IntPtr> callback)
CGPDFOperatorTableSetCallback (Handle, name, callback);
}

#if NET
// this signature requires C# 9 and unsafe code
public unsafe void SetCallback (string name, delegate* unmanaged<IntPtr, IntPtr, void> callback)
{
if (name == null)
throw new ArgumentNullException (nameof (name));

CGPDFOperatorTableSetCallback (Handle, name, callback);
}
#endif

static public CGPDFScanner GetScannerFromInfo (IntPtr gchandle)
{
return GCHandle.FromIntPtr (gchandle).Target as CGPDFScanner;
Expand Down
17 changes: 17 additions & 0 deletions tests/monotouch-test/CoreGraphics/PDFScannerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
//

using System;
using System.Runtime.InteropServices;

using Foundation;
using CoreGraphics;
using ObjCRuntime;
Expand All @@ -22,7 +24,11 @@ public class PDFScannerTest {
int bt_count;
int do_checks;

#if NET
[UnmanagedCallersOnly]
#else
[MonoPInvokeCallback (typeof (Action<IntPtr,IntPtr>))]
#endif
static void BT (IntPtr reserved, IntPtr info)
{
// sadly the parameters are always identical and we can't know the operator name
Expand All @@ -35,7 +41,11 @@ static void BT (IntPtr reserved, IntPtr info)
(scanner.UserInfo as PDFScannerTest).bt_count++;
}

#if NET
[UnmanagedCallersOnly]
#else
[MonoPInvokeCallback (typeof (Action<IntPtr,IntPtr>))]
#endif
static void Do (IntPtr reserved, IntPtr info)
{
// sadly the parameters are always identical and we can't know the operator name
Expand Down Expand Up @@ -97,6 +107,13 @@ public void Tamarin ()
table.SetCallback ("Do", delegate (CGPDFScanner scanner) {
// ... drill down to the image
});
#elif NET
unsafe {
// BT == new paragraph
table.SetCallback ("BT", &BT);
// Do == the image is inside it
table.SetCallback ("Do", &Do);
}
#else
// BT == new paragraph
table.SetCallback ("BT", BT);
Expand Down

6 comments on commit e8f2672

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Choose a reason for hiding this comment

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

❌ [CI Build] Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

API & Generator diff

API Diff (from PR only) (no change)
Generator Diff (no change)

Packages generated

View packages

Test results

1 tests failed, 192 tests passed.

Failed tests

  • introspection/watchOS 32-bits - simulator/Debug (watchOS 5.0): Failed

Pipeline on Agent XAMBOT-1038.BigSur'
[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. (#11560)

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Tests were not ran (VSTS: device tests tvOS). ⚠️

Results were skipped for this run due to provisioning problems Azure Devops. Please contact the bot administrator.

Pipeline on Agent
[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. (#11560)

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Tests were not ran (VSTS: device tests iOS). ⚠️

Results were skipped for this run due to provisioning problems Azure Devops. Please contact the bot administrator.

Pipeline on Agent
[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. (#11560)

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Choose a reason for hiding this comment

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

❌ Tests failed on macOS Mac Mojave (10.14) ❌

Tests failed on Mac Mojave (10.14).

Failed tests are:

  • introspection
  • xammac_tests

Pipeline on Agent
[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. (#11560)

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Choose a reason for hiding this comment

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

❌ Tests failed on macOS Mac High Sierra (10.13) ❌

Tests failed on Mac High Sierra (10.13).

Failed tests are:

  • introspection
  • xammac_tests

Pipeline on Agent
[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. (#11560)

@vs-mobiletools-engineering-service2
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Tests were not ran (VSTS: device tests iOS32b). ⚠️

Results were skipped for this run due to provisioning problems Azure Devops. Please contact the bot administrator.

Pipeline on Agent
[CoreGraphics] Adjust CGPDFOperatorTable to not use a generic Action delegate. (#11560)

Please sign in to comment.