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

Low level API support for Objective-C scenarios #44659

Closed
AaronRobinsonMSFT opened this issue Nov 13, 2020 · 28 comments · Fixed by #52146
Closed

Low level API support for Objective-C scenarios #44659

AaronRobinsonMSFT opened this issue Nov 13, 2020 · 28 comments · Fixed by #52146
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Interop-coreclr Priority:1 Work that is critical for the release, but we could probably ship without tracking This issue is tracking the completion of other related issues.
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Nov 13, 2020

The Blazor Desktop scenario will require interop support for enabling the usage of native UI components. A new API should be provided to facilitate this scenario.

Goals:

  • Support macOS version of Xamarin-macios running on CoreCLR.
  • Limit code changes to Xamarin bridge.

Non-Goals:

  • Support non-macOS scenarios (e.g. watchOS, tvOS, etc).
  • Provide a flexible API for another Objective-C interop implementation approach.

Proposed API

namespace System.Runtime.InteropServices.ObjectiveC
{
    /// <summary>
    /// Attribute used to indicate a class is tracked from the native environment.
    /// </summary>
    [AttributeUsage(AttributeTargets.Class)]
    public class TrackedNativeReferenceAttribute : Attribute
    {
        /// <summary>
        /// Instantiate a <see cref="TrackedNativeReferenceAttribute"/> instance.
        /// </summary>
        public TrackedNativeReferenceAttribute() { }
    }

    /// <summary>
    /// API to enable an Objective-C bridge.
    /// </summary>
    public static class Bridge
    {
        /// <summary>
        /// Initialize reference tracking for the Objective-C bridge API.
        /// </summary>
        /// <param name="beginEndCallback">Called when tracking begins and ends.</param>
        /// <param name="isReferencedCallback">Called to determine if a managed object instance is referenced elsewhere, and must not be collected by the GC.</param>
        /// <param name="trackedObjectEnteredFinalization">Called when a tracked object enters the finalization queue.</param>
        /// <exception cref="InvalidOperationException">Thrown if this API has already been called.</exception>
        /// <remarks>
        /// All callbacks must be written in native code since they will be called by the GC and
        /// managed code is not able to run at that time.
        ///
        /// The <paramref name="beginEndCallback"/> will be called when reference tracking begins and ends.
        /// The associated begin/end pair will never be nested.
        ///
        /// The <paramref name="isReferencedCallback"/> should return 0 for not reference or 1 for
        /// referenced. Any other value has undefined behavior.
        /// </remarks>
        public static unsafe void InitializeReferenceTracking(
            delegate* unmanaged<void> beginEndCallback,
            delegate* unmanaged<IntPtr, int> isReferencedCallback,
            delegate* unmanaged<IntPtr, void> trackedObjectEnteredFinalization);

        /// <summary>
        /// Request native reference tracking for the supplied object.
        /// </summary>
        /// <param name="obj">The object to track.</param>
        /// <param name="scratchMemory">A pointer to scratch memory.</param>
        /// <returns>Reference tracking GC handle.</returns>
        /// <exception cref="InvalidOperationException">Thrown if the Bridge API has not been initialized.</exception>
        /// <remarks>
        /// Reference tracking in the <see cref="Bridge"/> must be initialized prior to calling
        /// this function.
        ///
        /// The <paramref name="obj"/> must have a type in its hierarchy marked with
        /// <see cref="TrackedNativeReferenceAttribute"/>.
        ///
        /// The "Is Referenced" callback passed to InitializeReferenceTracking
        /// will be passed the <paramref name="scratchMemory"/> returned from this function.
        /// The memory it points at is 2 pointer's worth (for example, 16 bytes on a 64-bit platform) and
        /// will be zeroed out and available until <paramref name="obj"/> is collected by the GC.
        /// The memory pointed to by <paramref name="scratchMemory"/> can be used for any purpose by the
        /// caller of this function and usable during the "Is Referenced" callback.
        ///
        /// Calling this function multiple times with the same <paramref name="obj"/> will
        /// return a new handle each time but the same scratch memory will be returned. The
        /// scratch memory is only guaranteed to be zero initialized on the first call.
        ///
        /// The caller is responsible for freeing the returned <see cref="GCHandle"/>.
        /// </remarks>
        public static GCHandle CreateReferenceTrackingHandle(
            object obj,
            out IntPtr scratchMemory);

        /// <summary>
        /// Objective-C msgSend function override options.
        /// </summary>
        /// <see href="https://developer.apple.com/documentation/objectivec/1456712-objc_msgsend"/>
        public enum MsgSendFunction
        {
            ObjCMsgSend,
            ObjCMsgSendFpret,
            ObjCMsgSendStret,
            ObjCMsgSendSuper,
            ObjCMsgSendSuperStret,
        }

        /// <summary>
        /// Set function pointer override for an Objective-C runtime message passing export.
        /// </summary>
        /// <param name="msgSendFunction">The export to override.</param>
        /// <param name="func">The function override.</param>
        /// <exception cref="InvalidOperationException">Thrown if the msgSend function has already been overridden.</exception>
        /// <remarks>
        /// Providing an override can enable support for Objective-C
        /// exception propagation and variadic argument support.
        /// </remarks>
        public static void SetMessageSendCallback(MsgSendFunction msgSendFunction, IntPtr func);

        /// <summary>
        /// Sets a pending exception for this thread to be thrown
        /// the next time the runtime is entered from an overridden
        /// msgSend P/Invoke.
        /// </summary>
        /// <param name="exception">The exception.</param>
        /// <remarks>
        /// If <c>null</c> is supplied any pending exception is discarded.
        /// </remarks>
        public static void SetMessageSendPendingExceptionForThread(Exception? exception);

        /// <summary>
        /// Handler for unhandled Exceptions crossing the managed -> native boundary (that is, Reverse P/Invoke).
        /// </summary>
        /// <param name="exception">Unhandled exception.</param>
        /// <param name="lastMethod">Last managed method.</param>
        /// <param name="context">Context provided to the returned function pointer.</param>
        /// <returns>Exception propagation callback.</returns>
        /// <remarks>
        /// If the handler is able to propagate the managed Exception properly to the native environment an
        /// unmanaged callback can be returned, otherwise <c>null</c>. The RuntimeMethodHandle is to the
        /// last managed method that was executed prior to leaving the runtime. Along with the returned callback
        /// the handler can return a context that will be passed to the callback during dispatch.
        ///
        /// The returned handler will be passed the context when called and is the responsibility of the callback
        /// to managed. The handler must not return and is expected to propagate the exception into the native
        /// environment or fail fast.
        /// </remarks>
        public unsafe delegate delegate* unmanaged<IntPtr, void> UnhandledExceptionPropagationHandler(
            Exception exception,
            RuntimeMethodHandle lastMethod,
            out IntPtr context);

        /// <summary>
        /// Event for handling the propagation of unhandled Exceptions across a managed -> native boundary (that is, Reverse P/Invoke).
        /// </summary>
        /// <remarks>
        /// The first handler to return a non-null callback will be used to handle the Exception and no other
        /// handlers will be asked if they can propagate the exception.
        /// </remarks>
        public static event UnhandledExceptionPropagationHandler? UnhandledExceptionPropagation;
    }
}

Example usage

Simple usage example is not available. A prototype consumption can be found in https://github.com/rolfbjarne/xamarin-macios/tree/dotnet-coreclr

@AaronRobinsonMSFT AaronRobinsonMSFT added area-Interop-coreclr User Story A single user-facing feature. Can be grouped under an epic. labels Nov 13, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 6.0.0 milestone Nov 13, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Nov 13, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Nov 13, 2020
@AaronRobinsonMSFT
Copy link
Member Author

@AaronRobinsonMSFT AaronRobinsonMSFT added this to To do in Interop-CoreCLR 6.0 via automation Nov 13, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT moved this from To do to Epics in Interop-CoreCLR 6.0 Nov 13, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added the Priority:0 Work that we can't release without label Nov 13, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT self-assigned this Nov 13, 2020
@dotnet dotnet deleted a comment from AaronRobinsonMSFT Nov 16, 2020
@marek-safar marek-safar removed the User Story A single user-facing feature. Can be grouped under an epic. label Nov 16, 2020
@jeffschwMSFT jeffschwMSFT added the User Story A single user-facing feature. Can be grouped under an epic. label Nov 16, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 19, 2020
@jkotas
Copy link
Member

jkotas commented Nov 19, 2020

GetInstance vs. Inst

Nit: Should Inst be non-abbreviated everywhere?

SetMessageSendCallbacks

Does this also only apply to the global registered one? If I am not the global registered one, how to I do the error/exception handling?

record BlockProxy

  • I would stay away from records in public APIs. records come with a lot of auto-generated baggage and it is not understood whether they are good fit for public .NET runtime APIs (my guess is that they are not a good fit).
  • Is there any special interaction with the core runtime that this has, or is it included just for convenience (ie it would be fine for this to be in the managed part).
  • What does the BlockProxy finalizer do?

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Nov 19, 2020

Nit: Should Inst be non-abbreviated everywhere?

It was getting verbose so I abbreviated it. It is either "Instance" or "Id". The "id" is the type used in Objective-C so that will make sense to anyone using this API. I think I will replace it with "id".

Does this also only apply to the global registered one?

Not necessarily but your question does prompt me to say it should. Perhaps changing this to a series of out params and having the Runtime call a GetMessageSendCallbacks when/if RegisterAsGlobal is called?

If I am not the global registered one, how to I do the error/exception handling?

That is a good question. My initial response would be you can't. However, I don't see any reason for not having a getter method to provide the wrapper overrides. Thoughts?

  • I would stay away from records in public APIs. records come with a lot of auto-generated baggage and it is not understood whether they are good fit for public .NET runtime APIs (my guess is that they are not a good fit).

Good to know.

  • Is there any special interaction with the core runtime that this has, or is it included just for convenience (ie it would be fine for this to be in the managed part).

It is a convenience type to avoid exposing the Block ABI and lifetime. The contents are all that is needed for performing dispatch in managed code.

  • What does the BlockProxy finalizer do?

Handles the release of the native side of the Block.

@marek-safar
Copy link
Contributor

public abstract class Wrappers

Should it be abstract and not static? It seems like it would limit the usage.

protected abstract void StartThreadPoolWorkItem();

I'd not mix it with this proposal

public IntPtr CreateBlock

Do we need 3 parameters which carry the same information?

GetOrCreateObjectForInst
GetOrRegisterObjectForInst

Maybe they should be named same

@rolfbjarne
Copy link
Member

I think a much more comprehensive sample is needed to determine if this API is viable and discover any problems with it. There are many pitfalls, and it's easy to paint oneself into a corner.

I'd like to see:

  • A three-way split in the sample (maybe three separate projects?):
    • This API and an implementation for it
    • A much more complete binding for some Objective-C API. This will require a native Objective-C library as well. I can help with providing complex scenarios 😁
    • A consumer of that binding. I'm assuming that since the goal is to support macOS version of xamarin-macios running on CoreCLR, there should basically be no change for the consumer with regards to the current managed API we expose.
  • Memory profiling to make sure memory is freed when it should be (and not when it shouldn't be).

One example of more complex Objective-C code:

typedef void (^myBlock)();
@interface MyClass : NSObject {
}
	@property (copy) myBlock myProperty;
	@property (copy, class) myBlock myStaticProperty;
@end

The C# binding would look something like this:

delegate void MyBlock ();
public class MyClass : NSObject {
    public MyBlock MyProperty { get; set; }
    public static MyBlock MyStaticProperty { get; set; }
}

and then it could be consumed like this:

var obj = new MyClass ();
obj.MyProperty (); // call some Objective-C block
obj.MyProperty = () => { Console.WriteLine ("Hello World!"); };
obj.MyProperty (); // will print "Hello World!"

// let's make the static version a bit more complex
MyClass.MyStaticProperty = () => { Console.WriteLine ("Static Hello World, instance is: {0}", obj); }; // this lambda captures a local variable, which means it can't be represented as a static function
GC.Collect (); // does the GC collect the lambda?
MyClass.MyStaticProperty (); // the GC should not have collected the lambda
MyClass.MyStaticProperty = null;
GC.Collect (); // the GC should now be free to collect the lambda

another scenario I don't think the current sample handles:

@interface MyClass2 : NSObject {
}
    @property NSObject MyProperty; // initialized to an instance of a MyOtherClassThatIsNotBound in the constructor
@end
@interface MyOtherClassThatIsNotBound : NSObject {
}
@end

binding:

public class MyClass2 : NSObject {
    public NSObject MyProperty { get; set; }
}

sample:

var obj = new MyClass2 ();
Console.WriteLine (obj.MyProperty.GetType ()); // This should print "NSObject", because that's the closest managed type to the Objective-C type `MyOtherClassThatIsNotBound`

@jkotas
Copy link
Member

jkotas commented Nov 19, 2020

What does the BlockProxy finalizer do?
Handles the release of the native side of the Block.

Should it also have Dispose method (in addition, or instead of the finalizer)? It is not good to exclusively depend on finalizers to cleanup things that are likely to be created frequently.

protected abstract void StartThreadPoolWorkItem();
I'd not mix it with this proposal

If it is a general purpose callback like StartThreadPoolWorkItem/EndThreadPoolWorkItem , I agree that it is better to not mix it with this proposal.

If it is a Objective-C specific design like #44213 (comment), I think it makes sense to include it in this proposal.

@jkotas
Copy link
Member

jkotas commented Nov 19, 2020

The "id" is the type used in Objective-C so that will make sense to anyone using this API.

I can see "Instance" used in number of places in Objective-C, but I not able to find "id" anywhere. Could you please share some links?

If I am not the global registered one, how to I do the error/exception handling?
That is a good question. My initial response would be you can't. However, I don't see any reason for not having a getter method to provide the wrapper overrides. Thoughts?

Saying you cannot do error handling for the non-global ones does not sound right to me. It would only work if errors in Objective-C are almost always fatal. It may be better to just not introduce the non-global ones in the first place if we were not willing to deal with the error handling problem.

The key problem seems to be how runtime finds the right (non-global) message callbacks to use for the given call-site. Maybe the assembly helpers that make the error handling possible should be part of this low-level support and this type should expose higher-level concept.

@CoffeeFlux
Copy link
Contributor

CoffeeFlux commented Nov 19, 2020

I can see "Instance" used in number of places in Objective-C, but I not able to find "id" anywhere. Could you please share some links?

Their public API appears to use Instance quite a bit, but the actual typedef for the struct is id: https://developer.apple.com/documentation/objectivec/id

@rolfbjarne
Copy link
Member

I can see "Instance" used in number of places in Objective-C, but I not able to find "id" anywhere. Could you please share some links?

id is a keyword that basically means any type.

Ref: https://stackoverflow.com/a/7904028/183422

@rolfbjarne
Copy link
Member

It would only work if errors in Objective-C are almost always fatal.

They're almost always fatal. Apple says you should treat them as fatal (which doesn't prevent them from throwing exceptions that shouldn't necessarily be fatal).

However, making Objective-C exceptions fatal is quite annoying when debugging an app, and half the properties you inspect in an object throws exceptions that end up terminating the app or corrupting the process. Being able to convert them into managed exceptions that show up as such nicely in the IDE makes debugging not a horribly frustrating experience.

@AaronRobinsonMSFT
Copy link
Member Author

I think a much more comprehensive sample is needed to determine if this API is viable and discover any problems with it.

@rolfbjarne Agreed. I was hoping you would say "This API makes perfect sense and will work" but was expecting a more comprehensive example. Your example Objective-C scenarios are helpful and I will fold them into my example. This will likely require a custom runtime to expose a hook so I can actually implement a real E2E. Suggestions on memory profiling tool to use on macOS?

Their public API appears to use Instance quite a bit, but the actual typedef for the struct is id: https://developer.apple.com/documentation/objectivec/id

Yep. I was suggesting id because this is a low-level API and consumers will likely be familiar - similar to how we use IUnknown for COM.

If it is a general purpose callback like StartThreadPoolWorkItem/EndThreadPoolWorkItem , I agree that it is better to not mix it with this proposal.
If it is a Objective-C specific design like #44213 (comment), I think it makes sense to include it in this proposal.

Okay. I will change it to align with the comment. I think the suggestion to make it either generic at a higher level or keep it simple and direct here makes sense.

GetOrCreateObjectForInst
GetOrRegisterObjectForInst
Maybe they should be named same

@marek-safar These are two different ideas. Perhaps we only need one the register and then a simple get. The GetOrCreateObjectForInst is for the case when an Objective-C id enters the runtime and may or may not already have a wrapper. If it doesn't a callback will be called to determine the class, if it does then the existing wrapper will be returned. The register is for cases when one already has a .NET wrapper (i.e. construction).

Saying you cannot do error handling for the non-global ones does not sound right to me. It would only work if errors in Objective-C are almost always fatal. It may be better to just not introduce the non-global ones in the first place if we were not willing to deal with the error handling problem.

@jkotas One could do non-global but then the non-global instance wouldn't be able to override the P/Invoke look up path, but could still provide raw function pointers for people to use.

The key problem seems to be how runtime finds the right (non-global) message callbacks to use for the given call-site. Maybe the assembly helpers that make the error handling possible should be part of this low-level support and this type should expose higher-level concept.

I don't understand this at all. We have a singular case that needs support P/Invoke lookup - that could be handled by a Wrappers instance when RegisterAsGlobal is called. This would update the API to be a "get" with outs. Also providing a series of function pointers that developers could consume directly through C# function pointers would be nice merely to slowly move away from the DllImport pattern. We definitely don't need that but it is an option.

If I assume the "assembly helpers" are those implementations written by Xamarin then we would need to bake them into the runtime or provide some dllmap API - not against it but doesn't seem like a good idea. From there we could at this API provide an "OnObjectiveCException" event handle for that would be called on the global instance. Also not against this, but we are still butting up against the problem of how are the customized messaging APIs delivered. Either they are apart of the runtime or they are provided. If they are provided, how? Native or Managed API? I am proposing keeping as much of this in managed code so recommend the protected "get" with outs I mention above.

@jkotas
Copy link
Member

jkotas commented Nov 19, 2020

I was suggesting id because this is a low-level API and consumers will likely be familiar - similar to how we use IUnknown for COM.

Does Objective-C use id in any of the (low-level) API names?

FWIW, we have used the ComInstance in the ComWrappers version of these APIs, even through it could have been IUnknown.

This makes me think, should this be named ObjectiveCWrappers to mirror ComWrappers name?

@jkotas One could do non-global but then the non-global instance wouldn't be able to override the P/Invoke look up path, but could still provide raw function pointers for people to use.

The use case for the non-global instances is embedded Objective C interop (ie as a NuGet package author, I want to do local Objective C interop, without picking up dependencies on Xamarion macOS, but also want to peacefully co-exist with Xamarin macOS if it gets into picture).

We should either make this work well, or not bother. If we want to make this work well, I would like to see how the API makes it possible. (I think it is ok to say that you cannot depend on the DllImport entrypoint remapping in the embedded case.)

I see the DllImport entrypoint remapping as legacy quirk. There are number of ways how we can make it work. I would pick the best way once we have the larger rocks like whether the non-global instances are a thing figured out.

@AaronRobinsonMSFT
Copy link
Member Author

FWIW, we have used the ComInstance in the ComWrappers version of these APIs, even through it could have been IUnknown.

Yes and I consider that a mistake. I think IUnknown would have been more appropriate. So sounds like Instance is winning out.

This makes me think, should this be named ObjectiveCWrappers to mirror ComWrappers name?

I think COM is a special case given its history with .NET so having it in InteropServices makes sense. For this I placed it under the ObjectiveC namespace so dropped that on the type name.

We should either make this work well, or not bother. If we want to make this work well, I would like to see how the API makes it possible. (I think it is ok to say that you cannot depend on the DllImport entrypoint remapping in the embedded case.)

Okay. This shouldn't be too hard to express. Let me update the API.

I see the DllImport entrypoint remapping as legacy quirk. There are number of ways how we can make it work. I would pick the best way once we have the larger rocks like whether the non-global instances are a thing figured out.

Will do.

@Therzok
Copy link
Contributor

Therzok commented Jan 22, 2021

One of the main issues we've hit in VSMac is the disposal of shared instances. In example, there are many static fields exposed in AppKit like [NSFont systemFontOfSize:size] would be mapped to only one managed instance, as there is one native instance for each size. In the case of Xamarin.Mac specifically, this would dispose the shared instance, so if you had something like:

class A
{
    // This is a shared instance in native, so managed will have one shared instance too.
    public NSFont Font = NSFont.SystemFontOfSize(13);
}

class Program
{
    public static void Main()
    {
        A a1 = new A();
        A a2 = new A();

        a2.Font.Dispose();
        // a1.Font is now disposed.
    }
}

The implementation seems to handle the case where you can customize the unmanaged reference values, but there seems to be nothing for managed code. Would it be possible to have a reference counted mechanism in place for managed, so as to not dispose shared instances?

The workaround we've had in place was to create a new managed wrapper every time for our code, but that is inefficient allocation wise and does not stop extensions in our app from breaking each other, since they would normally use Xamarin.Mac APIs.

@AaronRobinsonMSFT
Copy link
Member Author

Would it be possible to have a reference counted mechanism in place for managed, so as to not dispose shared instances?

@Therzok Thanks for the question. This should be possible by the implementing interop system. The Dispose() on the ObjectiveCBase type can be overridden as desired. Internally to the runtime the call was by default going to sever the connection between the Objective-C instance and managed wrapper. There are a few arguments for leaving the semantic the same (i.e. compat and implied nature of Dispose()). I am open to being convinced changing the built in behavior is more appropriate as you've mentioned though.

@jkoritzinsky
Copy link
Member

I've done some investigation into the proposed API for handling Blocks and it looks like we're missing one part. We don't have an accurate way to determine whether or not we need to set the BLOCK_HAS_STRET flag on the block. I think we need to extend the GetBlockInvokeAndSignature method to have a way to specify to the runtime that the block uses the stret calling convention (has a return buffer).

@AaronRobinsonMSFT
Copy link
Member Author

@jkoritzinsky I was unable to find any uses of that so didn't include it. Can you point at a case where it is needed?

@jkoritzinsky
Copy link
Member

I don't know if it's "needed" per say, but I was able to put together a simple example on Compiler Explorer that causes Clang to set the flag: https://godbolt.org/z/c64oxx

I don't know what reads these flags at any point so I don't really know if any of them are required, but I have a feeling that if a flag exists, then it's probably required in some case.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Jan 26, 2021

but I have a feeling that if a flag exists, then it's probably required in some case.

Yeah. You would think that. The documentation for this is is a bit of a comedy of errors around some of these flags - https://clang.llvm.org/docs/Block-ABI-Apple.html. I do agree it appears this flag is needed in some cases. I don't see it being used in the Xamarin.Mac scenarios so didn't think we needed it but your example seems reasonable to me. I would start with simply changing GetBlockInvokeAndSignature to make the CreateBlockFlags flags a by-ref. It was actually like that in a previous iteration but I didn't have any flags at the time so changed it.

@rolfbjarne
Copy link
Member

rolfbjarne commented Jan 26, 2021 via email

@jkoritzinsky
Copy link
Member

Another quick question from things I've observed when playing around with Compiler Explorer. Do we need to support a custom objc_msgSend_fpret? Based on my tests on Compiler Explorer and the docs, it's only used on x86 systems, not on x64, ARM64, or ARM32. Since .NET 6+ doesn't support x86 MacOS, do we need to support a custom objc_msgSend_fpret implementation?

@AaronRobinsonMSFT
Copy link
Member Author

@jkoritzinsky That is a fair question. My approach was to ensure all msgSend* options exist. It costs very little to have the option. I am assuming the logic here is if the callee returns null we take that as an indication not to override it.

@rolfbjarne
Copy link
Member

I am assuming the logic here is if the callee returns null we take that as an indication not to override it.

You'll need this capability anyway, because not all objc_msgSend variants exist on all platforms (the *stret* versions don't exist on ARM64 for instance).

@AaronRobinsonMSFT AaronRobinsonMSFT added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Apr 27, 2021
@AaronRobinsonMSFT
Copy link
Member Author

Some offline feedback prior to review.

Would it make sense to make the unhandled exception callback general purpose, e.g. on Marshal? It looks like a general purpose API.
Naming nit: MsgSendFunction vs. SetMessageSendCallback. We should be using Msg or Message consistently.
Naming nit: SetMessageSendPendingExceptionForThread -> SetMessageSendPendingException. APIs typically avoid explicitly mentioning thread in the method name, even when the method operates on thread locals. (One of many examples: Marshal.SetLastError.)

@bartonjs
Copy link
Member

bartonjs commented Apr 29, 2021

Video

  • Is the ObjectiveC namespace justified? It may end up with a low number of types.
    • We decided yes, for symmetry with the others.
  • Bridge.CreateReferenceTrackingHandle should out Span instead of just IntPtr so it can communicate that you get two pointers worth of memory (and avoids the famous last words of "it'll always be two").
  • Rename Bridge.CreateReferenceTrackingHandle@scratchMemory to taggedMemory
  • Remove the UnhandledExceptionPropagation event and make it be only one delegate, bound in the Initialize call
  • TrackedNativeReferencesAttribute should be sealed, Inherited=true, AllowMultiple=false
  • Is "Bridge" too general name even for an out of the way namespace?
    • We suggested ObjectiveCMarshal
  • We renamed TrackedNativeReferenceAttribute to ObjectiveCTrackedTypeAttribute
  • We renamed SetMessageSendPendingExceptionForThread to SetMessageSendPendingException (dropped "ForThread")
namespace System.Runtime.InteropServices.ObjectiveC
{
    /// <summary>
    /// Attribute used to indicate a class is tracked from the native environment.
    /// </summary>
    [AttributeUsage(AttributeTargets.Class, Inherited=true, AllowMultiple=false)]
    public sealed class ObjectiveCTrackedTypeAttribute : Attribute
    {
        /// <summary>
        /// Instantiate a <see cref="ObjectiveCTrackedTypeAttribute"/> instance.
        /// </summary>
        public ObjectiveCTrackedTypeAttribute() { }
    }

    /// <summary>
    /// API to enable an Objective-C bridge.
    /// </summary>
    public static class ObjectiveCMarshal
    {
        /// <summary>
        /// Handler for unhandled Exceptions crossing the managed -> native boundary (that is, Reverse P/Invoke).
        /// </summary>
        /// <param name="exception">Unhandled exception.</param>
        /// <param name="lastMethod">Last managed method.</param>
        /// <param name="context">Context provided to the returned function pointer.</param>
        /// <returns>Exception propagation callback.</returns>
        /// <remarks>
        /// If the handler is able to propagate the managed Exception properly to the native environment an
        /// unmanaged callback can be returned, otherwise <c>null</c>. The RuntimeMethodHandle is to the
        /// last managed method that was executed prior to leaving the runtime. Along with the returned callback
        /// the handler can return a context that will be passed to the callback during dispatch.
        ///
        /// The returned handler will be passed the context when called and is the responsibility of the callback
        /// to managed. The handler must not return and is expected to propagate the exception into the native
        /// environment or fail fast.
        /// </remarks>
        public unsafe delegate delegate* unmanaged<IntPtr, void> UnhandledExceptionPropagationHandler(
            Exception exception,
            RuntimeMethodHandle lastMethod,
            out IntPtr context);

        /// <summary>
        /// Initialize reference tracking for the Objective-C bridge API.
        /// </summary>
        /// <param name="beginEndCallback">Called when tracking begins and ends.</param>
        /// <param name="isReferencedCallback">Called to determine if a managed object instance is referenced elsewhere, and must not be collected by the GC.</param>
        /// <param name="trackedObjectEnteredFinalization">Called when a tracked object enters the finalization queue.</param>
        /// <param name="unhandledExceptionPropagationHandler">Handler for the propagation of unhandled Exceptions across a managed -> native boundary (that is, Reverse P/Invoke).</param>
        /// <exception cref="InvalidOperationException">Thrown if this API has already been called.</exception>
        /// <remarks>
        /// All callbacks must be written in native code since they will be called by the GC and
        /// managed code is not able to run at that time.
        ///
        /// The <paramref name="beginEndCallback"/> will be called when reference tracking begins and ends.
        /// The associated begin/end pair will never be nested.
        ///
        /// The <paramref name="isReferencedCallback"/> should return 0 for not reference or 1 for
        /// referenced. Any other value has undefined behavior.
        /// </remarks>
        public static unsafe void Initialize(
            delegate* unmanaged<void> beginEndCallback,
            delegate* unmanaged<IntPtr, int> isReferencedCallback,
            delegate* unmanaged<IntPtr, void> trackedObjectEnteredFinalization,
            UnhandledExceptionPropagationHandler unhandledExceptionPropagationHandler);

        /// <summary>
        /// Request native reference tracking for the supplied object.
        /// </summary>
        /// <param name="obj">The object to track.</param>
        /// <param name="scratchMemory">A pointer to scratch memory.</param>
        /// <returns>Reference tracking GC handle.</returns>
        /// <exception cref="InvalidOperationException">Thrown if the Bridge API has not been initialized.</exception>
        /// <remarks>
        /// Reference tracking in the <see cref="Bridge"/> must be initialized prior to calling
        /// this function.
        ///
        /// The <paramref name="obj"/> must have a type in its hierarchy marked with
        /// <see cref="TrackedNativeReferenceAttribute"/>.
        ///
        /// The "Is Referenced" callback passed to InitializeReferenceTracking
        /// will be passed the <paramref name="scratchMemory"/> returned from this function.
        /// The memory it points at is 2 pointer's worth (for example, 16 bytes on a 64-bit platform) and
        /// will be zeroed out and available until <paramref name="obj"/> is collected by the GC.
        /// The memory pointed to by <paramref name="scratchMemory"/> can be used for any purpose by the
        /// caller of this function and usable during the "Is Referenced" callback.
        ///
        /// Calling this function multiple times with the same <paramref name="obj"/> will
        /// return a new handle each time but the same scratch memory will be returned. The
        /// scratch memory is only guaranteed to be zero initialized on the first call.
        ///
        /// The caller is responsible for freeing the returned <see cref="GCHandle"/>.
        /// </remarks>
        public static GCHandle CreateReferenceTrackingHandle(
            object obj,
            out Span<IntPtr> taggedMemory);

        /// <summary>
        /// Objective-C msgSend function override options.
        /// </summary>
        /// <see href="https://developer.apple.com/documentation/objectivec/1456712-objc_msgsend"/>
        public enum MessageSendFunction
        {
            MsgSend,
            MsgSendFpret,
            MsgSendStret,
            MsgSendSuper,
            MsgSendSuperStret,
        }

        /// <summary>
        /// Set function pointer override for an Objective-C runtime message passing export.
        /// </summary>
        /// <param name="msgSendFunction">The export to override.</param>
        /// <param name="func">The function override.</param>
        /// <exception cref="InvalidOperationException">Thrown if the msgSend function has already been overridden.</exception>
        /// <remarks>
        /// Providing an override can enable support for Objective-C
        /// exception propagation and variadic argument support.
        /// </remarks>
        public static void SetMessageSendCallback(MessageSendFunction msgSendFunction, IntPtr func);

        /// <summary>
        /// Sets a pending exception for this thread to be thrown
        /// the next time the runtime is entered from an overridden
        /// msgSend P/Invoke.
        /// </summary>
        /// <param name="exception">The exception.</param>
        /// <remarks>
        /// If <c>null</c> is supplied any pending exception is discarded.
        /// </remarks>
        public static void SetMessageSendPendingException(Exception? exception);
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work labels Apr 29, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 30, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels May 12, 2021
.NET 6.0 automation moved this from Proposed to Completed May 13, 2021
Interop-CoreCLR 6.0 automation moved this from Epics to Done May 13, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Interop-coreclr Priority:1 Work that is critical for the release, but we could probably ship without tracking This issue is tracking the completion of other related issues.
Projects
.NET 6.0
  
Completed
Development

Successfully merging a pull request may close this issue.