Skip to content

Commit

Permalink
[generator] Make the block callbacks UnmanagedCallersOnly functions i…
Browse files Browse the repository at this point in the history 
…n .NET. (#17741)

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

Contributes towards #15783.
  • Loading branch information
@rolfbjarne
rolfbjarne committed Mar 14, 2023
1 parent 9178171 commit 29633a6
Show file tree
Hide file tree
Showing 9 changed files with 61 additions and 226 deletions.
5 changes: 5 additions & 0 deletions src/Security/SecProtocolOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -518,8 +518,13 @@ public void SetKeyUpdateCallback (SecProtocolKeyUpdate keyUpdate, DispatchQueue
ObjCRuntime.ThrowHelper.ThrowArgumentNullException (nameof (keyUpdateQueue));

unsafe {
#if NET
delegate* unmanaged<IntPtr, NativeHandle, NativeHandle, void> trampoline = &Trampolines.SDSecProtocolKeyUpdate.Invoke;
using var block = new BlockLiteral (trampoline, keyUpdate, typeof (Trampolines.SDSecProtocolKeyUpdate), nameof (Trampolines.SDSecProtocolKeyUpdate.Invoke));
#else
using var block = new BlockLiteral ();
block.SetupBlockUnsafe (Trampolines.SDSecProtocolKeyUpdate.Handler, keyUpdate);
#endif
sec_protocol_options_set_key_update_block (Handle, &block, keyUpdateQueue.Handle);
}
}
Expand Down
18 changes: 14 additions & 4 deletions src/bgen/Generator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,7 @@ public TrampolineInfo MakeTrampoline (Type t)
var rt = mi.ReturnType;
var rts = IsNativeEnum (rt) ? "var" : RenderType (rt);
var trampoline_name = Nomenclator.GetTrampolineName (t);
var functionPointerSignature = string.Join (", ", pars.Select (v => v.Type)) + ", " + returntype;
var ti = new TrampolineInfo (userDelegate: FormatType (null, t),
delegateName: "D" + trampoline_name,
pars: string.Join (", ", pars.Select (v => v.Type + " " + v.ParameterName)),
Expand All @@ -827,7 +828,8 @@ public TrampolineInfo MakeTrampoline (Type t)
returnFormat: returnformat,
clear: clear.ToString (),
postConvert: postConvert.ToString (),
type: t);
type: t,
functionPointerSignature: functionPointerSignature);


ti.UserDelegateTypeAttribute = FormatType (null, t);
Expand Down Expand Up @@ -1681,14 +1683,17 @@ void GenerateTrampolinesForQueue (TrampolineInfo [] queue)
// but we have a workaround in place because we can't fix old, binary bindings so...
// print ("[Preserve (Conditional=true)]");
// For .NET we fix it using the DynamicDependency attribute below
#if !NET
print ("unsafe static internal readonly {0} Handler = Invoke;", ti.DelegateName);
print ("");
#endif
#if NET
print ("[Preserve (Conditional = true)]");
print ("[global::System.Diagnostics.CodeAnalysis.DynamicDependency (\"Handler\")]");
#endif
print ("[UnmanagedCallersOnly]");
#else
print ("[MonoPInvokeCallback (typeof ({0}))]", ti.DelegateName);
print ("static unsafe {0} Invoke ({1}) {{", ti.ReturnType, ti.Parameters);
#endif
print ("internal static unsafe {0} Invoke ({1}) {{", ti.ReturnType, ti.Parameters);
indent++;
print ("var descriptor = (BlockLiteral *) block;");
print ("var del = ({0}) (descriptor->Target);", ti.UserDelegate);
Expand Down Expand Up @@ -1737,9 +1742,14 @@ void GenerateTrampolinesForQueue (TrampolineInfo [] queue)
print ("internal static unsafe BlockLiteral CreateBlock ({0} callback)", ti.UserDelegate);
print ("{");
indent++;
#if NET
print ("delegate* unmanaged<{0}> trampoline = &Invoke;", ti.FunctionPointerSignature);
print ("return new BlockLiteral (trampoline, callback, typeof ({0}), nameof (Invoke));", ti.StaticName);
#else
print ("var block = new BlockLiteral ();");
print ("block.SetupBlockUnsafe (Handler, callback);");
print ("return block;");
#endif
indent--;
print ("}");
indent--;
Expand Down
4 changes: 3 additions & 1 deletion src/bgen/TrampolineInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,12 @@ public class TrampolineInfo {
public string? OutReturnType;
public string PostConvert;
public string? UserDelegateTypeAttribute;
public string FunctionPointerSignature;
public Type Type;

public TrampolineInfo (string userDelegate, string delegateName, string pars,
string convert, string invoke, string returnType, string delegateReturnType, string returnFormat,
string clear, string postConvert, Type type)
string clear, string postConvert, Type type, string functionPointerSignature)
{
UserDelegate = userDelegate;
DelegateName = delegateName;
Expand All @@ -43,6 +44,7 @@ public TrampolineInfo (string userDelegate, string delegateName, string pars,
Clear = clear;
PostConvert = postConvert;
Type = type;
FunctionPointerSignature = functionPointerSignature;
}

// Name for the static class generated that contains the Objective-C to C# block bridge
Expand Down
3 changes: 3 additions & 0 deletions tests/bindings-test/StructsAndEnums.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ public static class CFunctions {
[DllImport ("__Internal")]
public static extern void x_call_block (ref BlockLiteral block);

[DllImport ("__Internal")]
public static extern IntPtr x_call_func_3 (IntPtr fptr, IntPtr p1, IntPtr p2, IntPtr p3);

[DllImport ("__Internal")]
public static extern void x_get_matrix_float2x2 (IntPtr self, string sel, out float r0c0, out float r0c1, out float r1c0, out float r1c1);

Expand Down
203 changes: 4 additions & 199 deletions tests/cecil-tests/GenericPInvokes.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,75 +11,20 @@
#nullable enable

namespace Cecil.Tests {
enum GenericCheckResult {
Ok,
ContainsGenerics,
HasSuspectBranchTarget,
HasWeirdInstruction,
}

struct MethodAndResult {
public MethodDefinition Method;
public GenericCheckResult Result;
}

[TestFixture]
public class GenericPInvokesTest {
[TestCaseSource (typeof (Helper), nameof (Helper.NetPlatformImplementationAssemblyDefinitions))]
public void CheckSetupBlockUnsafeUsage (AssemblyInfo info)
{
// this scans the specified assmebly for all methods
// that call SetupBlockUnsafe and then scans the method
// to see if the first argument to SetupBlockUnsafe is
// a generic delegate type.
//
// the results are not just a list of booleans.
// some of the results are indeterminant.
// for example, if the call to SetupBlockUnsafe is
// also a branch target within the method then all
// bets are off for what's on the stack. If the call
// to pass the last argument is also a branch target,
// then all bets are off. Why? Imagine this scenario:
// SetupBlockUnsafe (someCondition ? val1 : val2, arg2);
// This means that the actual argument could be one of
// two things and it's probably better for a human to
// look at those cases instead of this code.
//
// most of the code generated by the C# compiler is
// pretty boring, which means that there is a very
// limited set of instructions that get used to set up
// this call. They amount to Ldarg, Ldvar, Ldfld variants
// which we like. The only thing that's "funny" is that
// when the method is an instance method, then
// arg 0 is the "this" pointer which is present on the
// stack, but isn't in the formal parameter this.
// So there's a little juggling to make sure we don't
// look past the array of parameters.
// We should not call BlockLiteral.SetupBlockUnsafe in our code at all.
// All our code should use the function pointer syntax for block creation:
// var block = new BlockLiteral (&function, nameof (<type where function is defined), nameof (function))

var assembly = info.Assembly;
var callsToSetupBlock = AllSetupBlocks (assembly);
Assert.IsTrue (callsToSetupBlock.Count () > 0);
var results = callsToSetupBlock.Select (GenericCheckDelegateArgument);
var allResults = callsToSetupBlock.Zip (results, (m, r) => new MethodAndResult { Method = m, Result = r });
var failures = allResults.Where (mar => mar.Result != GenericCheckResult.Ok);

var failingMethods = ListOfFailingGenerics (failures);

Assert.IsTrue (failures.Count () == 0,
$"There {failures.Count ()} calls to SetUpBlockUnsafe that have or might have generic delegates as arguments:{failingMethods}\n" +
"Check each method for usage. If it's a false positive, add the method name to the IsSetupBlockUnsafeOK method in the test.");
}

static bool IsSetupBlockUnsafeOK (MethodDefinition method)
{
var fullName = method.FullName;
switch (fullName) {
default:
return true;
}
Assert.That (callsToSetupBlock.Select (v => v.FullName), Is.Empty, "No calls at all to BlockLiteral.SetupBlockUnsafe");
}


[TestCaseSource (typeof (Helper), nameof (Helper.NetPlatformImplementationAssemblyDefinitions))]
public void CheckAllPInvokes (AssemblyInfo info)
{
Expand All @@ -94,29 +39,6 @@ public void CheckAllPInvokes (AssemblyInfo info)
$"There are {failures.Count ()} pinvoke methods that contain generics. This will not work in .NET 7 and above (see https://github.com/xamarin/xamarin-macios/issues/11771 ):{failingMethods}");
}

string ListOfFailingGenerics (IEnumerable<MethodAndResult> methodAndResults)
{
var list = new StringBuilder ();
foreach (var mar in methodAndResults) {
list.Append ("\n\"").Append (mar.Method.FullName).Append ("\" : ");
switch (mar.Result) {
case GenericCheckResult.ContainsGenerics:
list.Append ("method contains a generic argument for the first arg of SetupBlockUnsafe. This is problematic in .NET 7 and above.");
break;
case GenericCheckResult.HasSuspectBranchTarget:
list.Append ("one of the instructions in calling SetupBlockUnsafe is a branch target which means that checking this usage is indeterminant. Check this call yourself and if it has a generic, fix that. If it's OK then add the fullname of the method (<- that thing in quotes) to the exception method IsSetupBlockUnsafeOK in this test.");
break;
case GenericCheckResult.HasWeirdInstruction:
list.Append ("one of the setup instructions in calling SetupBlockUnsafe is...weird? It was something not expected and probably something that leads to an indeterminant situation. Check this call yourself and if it has a generic, fix that. It it's OK then add the fullname of the method (<- that thing in the quotes) to the exception method IsSetupBlockUnsafeOK in this test.");
break;
default:
list.Append ($"Got an unexpected result from checking - expected an error but got {mar.Result}");
break;
}
}
return list.ToString ();
}

string ListOfFailingMethods (IEnumerable<MethodDefinition> methods)
{
var list = new StringBuilder ();
Expand Down Expand Up @@ -187,122 +109,5 @@ static bool IsCall (Instruction instr)
return instr.OpCode == OpCodes.Call ||
instr.OpCode == OpCodes.Calli;
}

static GenericCheckResult GenericCheckDelegateArgument (MethodDefinition method)
{
if (method.Body is null)
return GenericCheckResult.Ok;
var instrs = method.Body.Instructions;
for (int i = 0; i < instrs.Count (); i++) {
if (i > 1 && IsCallToSetupBlockUnsafe (instrs [i])) {
var penultimate = instrs [i - 1];
if (!IsLastArgUsageIsReasonable (penultimate))
return GenericCheckResult.HasWeirdInstruction;
if (IsBranchTarget (method, penultimate))
return GenericCheckResult.HasSuspectBranchTarget;

var instr = instrs [i - 2];
var typeOfLastArg = GetLastArgType (method, instr);
if (typeOfLastArg.HasGenericParameters || typeOfLastArg.IsGenericInstance || typeOfLastArg.IsGenericParameter)
return GenericCheckResult.ContainsGenerics;
if (IsBranchTarget (method, instrs [i]))
return GenericCheckResult.HasSuspectBranchTarget;
}
}
return GenericCheckResult.Ok;
}

static OpCode [] reasonableOps = new OpCode [] {
OpCodes.Ldarg_0, OpCodes.Ldarg_1, OpCodes.Ldarg_2,
OpCodes.Ldarg_3, OpCodes.Ldarg_S, OpCodes.Ldloc_0,
OpCodes.Ldloc_1, OpCodes.Ldloc_2, OpCodes.Ldloc_3,
OpCodes.Ldloc_S
};

static bool IsLastArgUsageIsReasonable (Instruction instr)
{
return Array.IndexOf (reasonableOps, instr.OpCode) >= 0;
}

static bool IsBranchTarget (MethodDefinition method, Instruction target)
{
foreach (var instr in method.Body.Instructions) {
if (IsBranch (instr) && instr.Operand == target)
return true;
}
return false;
}

static OpCode [] branches = new OpCode [] {
OpCodes.Br_S, OpCodes.Brfalse_S, OpCodes.Brtrue_S,
OpCodes.Beq_S, OpCodes.Bge_S, OpCodes.Bgt_S,
OpCodes.Ble_S, OpCodes.Blt_S, OpCodes.Bne_Un_S,
OpCodes.Bge_Un_S, OpCodes.Bgt_Un_S, OpCodes.Ble_Un_S,
OpCodes.Blt_Un_S, OpCodes.Br, OpCodes.Brfalse,
OpCodes.Brtrue, OpCodes.Beq, OpCodes.Bge,
OpCodes.Bgt, OpCodes.Ble, OpCodes.Blt,
OpCodes.Bne_Un, OpCodes.Bge_Un, OpCodes.Bgt_Un,
OpCodes.Ble_Un, OpCodes.Blt_Un
};

static bool IsBranch (Instruction instr)
{
return Array.IndexOf (branches, instr.OpCode) >= 0;
}

static TypeReference GetLastArgType (MethodDefinition method, Instruction instr)
{
var paramDef = GetOperandType (method, instr);
if (paramDef is null) {
throw new NotImplementedException ($"Last instruction before call to SetupBlockUnsafe ({instr.ToString ()}) was not a Ldfld, Ldarg or Ldloc - this is quite unexpected - something's changed in the code base!");
}
return paramDef;
}

static TypeReference? GetOperandType (MethodDefinition method, Instruction instr)
{
var thisOffset = method.IsStatic ? 0 : -1;
if (instr.OpCode == OpCodes.Ldarg_0)
return GetCheckParameterType (method, 0 + thisOffset);
if (instr.OpCode == OpCodes.Ldarg_1)
return GetCheckParameterType (method, 1 + thisOffset);
if (instr.OpCode == OpCodes.Ldarg_2)
return GetCheckParameterType (method, 2 + thisOffset);
if (instr.OpCode == OpCodes.Ldarg_3)
return GetCheckParameterType (method, 3 + thisOffset);
if (instr.OpCode == OpCodes.Ldarg_S)
return (instr.Operand as ParameterDefinition)?.ParameterType;
if (instr.OpCode == OpCodes.Ldloc_0)
return GetLocalType (method, 0);

if (instr.OpCode == OpCodes.Ldloc_1)
return GetLocalType (method, 1);

if (instr.OpCode == OpCodes.Ldloc_2)
return GetLocalType (method, 2);

if (instr.OpCode == OpCodes.Ldloc_3)
return GetLocalType (method, 3);

if (instr.OpCode == OpCodes.Ldloc_S)
return (instr.Operand as VariableDefinition)?.VariableType;
if (instr.OpCode == OpCodes.Ldsfld)
return (instr.Operand as FieldReference)?.FieldType;

return null;
}

static TypeReference GetCheckParameterType (MethodDefinition method, int index)
{
Assert.IsFalse (index >= method.Parameters.Count (),
"This is unexpected - asked to get parameter {index} from method {method.ToString ()}, but it's not there");

return method.Parameters [index].ParameterType;
}

static TypeReference GetLocalType (MethodDefinition method, int localIndex)
{
return method.Body.Variables [localIndex].VariableType;
}
}
}
45 changes: 23 additions & 22 deletions tests/monotouch-test/Photos/LivePhotoEditingContextTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,30 +73,31 @@ public unsafe void FrameProcessingBlock2 ()
var m = t.GetMethod ("Invoke", BindingFlags.Static | BindingFlags.NonPublic);
Assert.NotNull (m, "Invoke");
var d = m.CreateDelegate (typeof (DPHLivePhotoFrameProcessingBlock2));
#if NET
var fptr = m.MethodHandle.GetFunctionPointer ();
var del = new DPHLivePhotoFrameProcessingBlock2 ((IntPtr a, NativeHandle b, NativeHandle* c) => (NativeHandle) global::Bindings.Test.CFunctions.x_call_func_3 (fptr, (IntPtr) a, (IntPtr) b, (IntPtr) (void*) c));
#else
var del = (DPHLivePhotoFrameProcessingBlock2) d;
#endif
Action userDelegate = new Action (() => Console.WriteLine ("Hello world!"));
BlockLiteral bl = new BlockLiteral ();
#if NET
using var bl = new BlockLiteral ((void*) fptr, managed, t, "Invoke");
#else
using var bl = new BlockLiteral ();
bl.SetupBlock (d, managed);
try {
var block = &bl;
var b = (IntPtr) block;
// simulate a call that does not produce an error
var args = new object [] { b, NativeHandle.Zero, IntPtr.Zero };
error_faker = null;
Assert.That (m.Invoke (null, args), Is.EqualTo (NativeHandle.Zero), "1");
// simulate a call that does produce an error
error_faker = new NSError ((NSString) "domain", 42);
IntPtr ptr = IntPtr.Zero;
args [2] = (IntPtr) (IntPtr*) &ptr;
Assert.That (m.Invoke (null, args), Is.EqualTo (NativeHandle.Zero), "2");
Assert.That (Marshal.ReadIntPtr ((IntPtr) args [2]), Is.EqualTo ((IntPtr) error_faker.Handle), "error 1");
Assert.That (ptr, Is.EqualTo ((IntPtr) error_faker.Handle), "error 2");
} finally {
bl.CleanupBlock ();
}
#endif
var block = &bl;
var b = (IntPtr) block;
// simulate a call that does not produce an error
error_faker = null;
Assert.That (del (b, NativeHandle.Zero, null), Is.EqualTo (NativeHandle.Zero), "1");
// simulate a call that does produce an error
error_faker = new NSError ((NSString) "domain", 42);
NativeHandle ptr = NativeHandle.Zero;
Assert.That (del (b, NativeHandle.Zero, &ptr), Is.EqualTo (NativeHandle.Zero), "2");
Assert.That ((IntPtr) ptr, Is.EqualTo ((IntPtr) error_faker.Handle), "error 2");
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/test-libraries/libtest.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ void useZLib ();

typedef void (^x_block_callback)();
void x_call_block (x_block_callback block);
void* x_call_func_3 (void* (*fptr)(void*, void*, void*), void* p1, void* p2, void* p3);

void x_get_matrix_float2x2 (id self, const char *sel, float* r0c0, float* r0c1, float* r1c0, float* r1c1);
void x_get_matrix_float3x3 (id self, const char *sel, float* r0c0, float* r0c1, float* r0c2, float* r1c0, float* r1c1, float* r1c2, float* r2c0, float* r2c1, float* r2c2);
Expand Down
Loading

6 comments on commit 29633a6

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

@vs-mobiletools-engineering-service2

This comment was marked as outdated.

Sign in to view

Please sign in to comment.