Objective-C msgSend* support for pending exceptions in Release (#52849)

AaronRobinsonMSFT · web-flow · commit 4a782d58ac4c · 2021-05-19T22:03:34.000-07:00
* Support checking for overridden methods during P/Invoke inline pass

* Objective-C runtime msgSend* functions are never inlinable.

* Update CrossGen2.

* Add comment on details of how we are testing the
SetMessageSendPendingException API.

* Disable the crossgen2smoke on OSX ARM64
diff --git a/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs b/src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs
@@ -10,7 +10,7 @@ namespace System.Runtime.InteropServices.ObjectiveC
     public static partial class ObjectiveCMarshal
     {
         /// <summary>
-        /// Sets a pending exception to be thrown the next time the runtime is entered from an overridden msgSend P/Invoke.
+        /// Sets a pending exception to be thrown the next time the runtime is entered from an Objective-C msgSend P/Invoke.
         /// </summary>
         /// <param name="exception">The exception.</param>
         /// <remarks>
diff --git a/src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalHelpers.cs b/src/coreclr/tools/Common/TypeSystem/Interop/IL/MarshalHelpers.cs
@@ -797,5 +797,23 @@ private static MarshallerKind GetArrayElementMarshallerKind(
                 return MarshallerKind.Invalid;
             }
         }
+
+        internal static bool ShouldCheckForPendingException(TargetDetails target, PInvokeMetadata metadata)
+        {
+            if (!target.IsOSX)
+                return false;
+
+            const string ObjectiveCLibrary = "/usr/lib/libobjc.dylib";
+            const string ObjectiveCMsgSend = "objc_msgSend";
+
+            // This is for the objc_msgSend suite of functions.
+            //   objc_msgSend
+            //   objc_msgSend_fpret
+            //   objc_msgSend_stret
+            //   objc_msgSendSuper
+            //   objc_msgSendSuper_stret
+            return metadata.Module.Equals(ObjectiveCLibrary)
+                && metadata.Name.StartsWith(ObjectiveCMsgSend);
+        }
     }
 }
diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/Stubs/PInvokeILEmitter.cs
@@ -43,7 +43,7 @@ private void EmitPInvokeCall(PInvokeILCodeStreams ilCodeStreams)
 
             MetadataType stubHelpersType = InteropTypes.GetStubHelpers(context);
 
-            // if the SetLastError flag is set in DllImport, clear the error code before doing P/Invoke 
+            // if the SetLastError flag is set in DllImport, clear the error code before doing P/Invoke
             if (_importMetadata.Flags.SetLastError)
             {
                 callsiteSetupCodeStream.Emit(ILOpcode.call, emitter.NewToken(
@@ -78,6 +78,9 @@ private MethodIL EmitIL()
             if (!_importMetadata.Flags.PreserveSig)
                 throw new NotSupportedException();
 
+            if (MarshalHelpers.ShouldCheckForPendingException(_targetMethod.Context.Target, _importMetadata))
+                throw new NotSupportedException();
+
             if (_targetMethod.IsUnmanagedCallersOnly)
                 throw new NotSupportedException();
 
diff --git a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs
@@ -93,14 +93,18 @@ public static bool IsMarshallingRequired(MethodDesc targetMethod)
             if (targetMethod.IsUnmanagedCallersOnly)
                 return true;
 
-            PInvokeFlags flags = targetMethod.GetPInvokeMethodMetadata().Flags;
+            PInvokeMetadata metadata = targetMethod.GetPInvokeMethodMetadata();
+            PInvokeFlags flags = metadata.Flags;
 
             if (flags.SetLastError)
                 return true;
 
             if (!flags.PreserveSig)
                 return true;
 
+            if (MarshalHelpers.ShouldCheckForPendingException(targetMethod.Context.Target, metadata))
+                return true;
+
             var marshallers = GetMarshallersForMethod(targetMethod);
             for (int i = 0; i < marshallers.Length; i++)
             {
diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp
@@ -2657,7 +2657,9 @@ void PInvokeStaticSigInfo::PreInit(MethodDesc* pMD)
 }
 
 PInvokeStaticSigInfo::PInvokeStaticSigInfo(
-    _In_ MethodDesc* pMD, _Outptr_opt_ LPCUTF8 *pLibName, _Outptr_opt_ LPCUTF8 *pEntryPointName)
+    _In_ MethodDesc* pMD,
+    _Outptr_opt_ LPCUTF8* pLibName,
+    _Outptr_opt_ LPCUTF8* pEntryPointName)
 {
     CONTRACTL
     {
@@ -2755,7 +2757,10 @@ PInvokeStaticSigInfo::PInvokeStaticSigInfo(
     InitCallConv(CallConvWinApiSentinel, FALSE);
 }
 
-void PInvokeStaticSigInfo::DllImportInit(_In_ MethodDesc* pMD, _Outptr_opt_ LPCUTF8 *ppLibName, _Outptr_opt_ LPCUTF8 *ppEntryPointName)
+void PInvokeStaticSigInfo::DllImportInit(
+    _In_ MethodDesc* pMD,
+    _Outptr_opt_ LPCUTF8* ppLibName,
+    _Outptr_opt_ LPCUTF8* ppEntryPointName)
 {
     CONTRACTL
     {
@@ -2784,7 +2789,6 @@ void PInvokeStaticSigInfo::DllImportInit(_In_ MethodDesc* pMD, _Outptr_opt_ LPCU
         return;
     }
 
-    // out parameter pEntryPointName
     if (ppEntryPointName && *ppEntryPointName == NULL)
         *ppEntryPointName = pMD->GetName();
 
@@ -3128,16 +3132,12 @@ BOOL NDirect::MarshalingRequired(
                 return TRUE;
         }
 
-        // SetLastError is handled by stub
-        PInvokeStaticSigInfo sigInfo(pMD);
-        if (sigInfo.GetLinkFlags() & nlfLastError)
-            return TRUE;
-
-        // LCID argument is handled by stub
-        if (GetLCIDParameterIndex(pMD) != -1)
-            return TRUE;
-
-        if (pMD->IsNDirect())
+        PInvokeStaticSigInfo sigInfo;
+        if (!pMD->IsNDirect())
+        {
+            new (&sigInfo) PInvokeStaticSigInfo(pMD);
+        }
+        else
         {
             // A P/Invoke marked with UnmanagedCallersOnlyAttribute
             // doesn't technically require marshalling. However, we
@@ -3154,13 +3154,23 @@ BOOL NDirect::MarshalingRequired(
             if (pNMD->IsClassConstructorTriggeredByILStub())
                 return TRUE;
 
+            InitializeSigInfoAndPopulateNDirectMethodDesc(pNMD, &sigInfo);
+
 #ifndef CROSSGEN_COMPILE
             // Pending exceptions are handled by stub
             if (Interop::ShouldCheckForPendingException(pNMD))
                 return TRUE;
 #endif // !CROSSGEN_COMPILE
         }
 
+        // SetLastError is handled by stub
+        if (sigInfo.GetLinkFlags() & nlfLastError)
+            return TRUE;
+
+        // LCID argument is handled by stub
+        if (GetLCIDParameterIndex(pMD) != -1)
+            return TRUE;
+
         callConv = sigInfo.GetCallConv();
     }
 
@@ -4225,7 +4235,11 @@ static void CreateNDirectStubAccessMetadata(
 
 namespace
 {
-    void PopulateNDirectMethodDescImpl(_Inout_ NDirectMethodDesc* pNMD, _In_ const PInvokeStaticSigInfo& sigInfo, _In_opt_z_ LPCUTF8 libName, _In_opt_z_ LPCUTF8 entryPointName)
+    void PopulateNDirectMethodDescImpl(
+        _Inout_ NDirectMethodDesc* pNMD,
+        _In_ const PInvokeStaticSigInfo& sigInfo,
+        _In_opt_z_ LPCUTF8 libName,
+        _In_opt_z_ LPCUTF8 entryPointName)
     {
         CONTRACTL
         {
@@ -4263,9 +4277,8 @@ namespace
             pNMD->ndirect.m_pszEntrypointName.SetValueMaybeNull(entryPointName);
         }
 
-        // Call this exactly ONCE per thread. Do not publish incomplete prestub flags
-        // or you will introduce a race condition.
-        pNMD->InterlockedSetNDirectFlags(ndirectflags);
+        // Do not publish incomplete prestub flags or you will introduce a race condition.
+        pNMD->InterlockedSetNDirectFlags(ndirectflags | NDirectMethodDesc::kNDirectPopulated);
     }
 }
 
@@ -4281,6 +4294,9 @@ void NDirect::PopulateNDirectMethodDesc(_Inout_ NDirectMethodDesc* pNMD)
     if (pNMD->IsSynchronized())
         COMPlusThrow(kTypeLoadException, IDS_EE_NOSYNCHRONIZED);
 
+    if (pNMD->IsPopulated())
+        return;
+
     LPCUTF8 szLibName = NULL, szEntryPointName = NULL;
     PInvokeStaticSigInfo sigInfo(pNMD, &szLibName, &szEntryPointName);
     PopulateNDirectMethodDescImpl(pNMD, sigInfo, szLibName, szEntryPointName);
@@ -4302,6 +4318,9 @@ void NDirect::InitializeSigInfoAndPopulateNDirectMethodDesc(_Inout_ NDirectMetho
     LPCUTF8 szLibName = NULL, szEntryPointName = NULL;
     new (pSigInfo) PInvokeStaticSigInfo(pNMD, &szLibName, &szEntryPointName);
 
+    if (pNMD->IsPopulated())
+        return;
+
     PopulateNDirectMethodDescImpl(pNMD, *pSigInfo, szLibName, szEntryPointName);
 }
 
diff --git a/src/coreclr/vm/dllimport.h b/src/coreclr/vm/dllimport.h
@@ -75,6 +75,10 @@ class NDirect
     static HRESULT HasNAT_LAttribute(IMDInternalImport *pInternalImport, mdToken token, DWORD dwMemberAttrs);
 
     // Either MD or signature & module must be given.
+    // Note: This method can be called at a time when the associated NDirectMethodDesc
+    // has not been fully populated. This means the optimized path for this call is to rely
+    // on the most basic P/Invoke metadata. An example when this can happen is when the JIT
+    // is compiling a method containing a P/Invoke that is being considered for inlining.
     static BOOL MarshalingRequired(
         _In_opt_ MethodDesc* pMD,
         _In_opt_ PCCOR_SIGNATURE pSig = NULL,
@@ -312,12 +316,12 @@ struct PInvokeStaticSigInfo
 
     PInvokeStaticSigInfo(_In_ MethodDesc* pMdDelegate);
 
-    PInvokeStaticSigInfo(_In_ MethodDesc* pMD, _Outptr_opt_ LPCUTF8 *pLibName, _Outptr_opt_ LPCUTF8 *pEntryPointName);
+    PInvokeStaticSigInfo(_In_ MethodDesc* pMD, _Outptr_opt_ LPCUTF8* pLibName, _Outptr_opt_ LPCUTF8* pEntryPointName);
 
 private:
     void ThrowError(WORD errorResourceID);
     void InitCallConv(CorInfoCallConvExtension callConv, BOOL bIsVarArg);
-    void DllImportInit(_In_ MethodDesc* pMD, _Outptr_opt_ LPCUTF8 *pLibName, _Outptr_opt_ LPCUTF8 *pEntryPointName);
+    void DllImportInit(_In_ MethodDesc* pMD, _Outptr_opt_ LPCUTF8* pLibName, _Outptr_opt_ LPCUTF8* pEntryPointName);
     void PreInit(Module* pModule, MethodTable *pClass);
     void PreInit(MethodDesc* pMD);
 
diff --git a/src/coreclr/vm/interoplibinterface.h b/src/coreclr/vm/interoplibinterface.h
@@ -147,7 +147,7 @@ class ObjCMarshalNative
     static bool IsTrackedReference(_In_ OBJECTREF object, _Out_ bool* isReferenced);
 
 public: // Identification
-    static bool IsRuntimeMsgSendFunctionOverridden(
+    static bool IsRuntimeMessageSendFunction(
         _In_z_ const char* libraryName,
         _In_z_ const char* entrypointName);
 
diff --git a/src/coreclr/vm/interoplibinterface_objc.cpp b/src/coreclr/vm/interoplibinterface_objc.cpp
@@ -127,14 +127,16 @@ namespace
         OBJC_MSGSEND "Super_stret",
     };
 
-    const void* STDMETHODCALLTYPE MessageSendPInvokeOverride(_In_z_ const char* libraryName, _In_z_ const char* entrypointName)
+    bool IsObjectiveCMessageSendFunction(_In_z_ const char* libraryName, _In_z_ const char* entrypointName)
     {
-        // All overrides are in libobjc
-        if (strcmp(libraryName, ObjectiveCLibrary) != 0)
-            return nullptr;
+        // Is the function in libobjc and named appropriately.
+        return ((strcmp(libraryName, ObjectiveCLibrary) == 0)
+                && (strncmp(entrypointName, OBJC_MSGSEND, _countof(OBJC_MSGSEND) -1) == 0));
+    }
 
-        // All overrides start with objc_msgSend
-        if (strncmp(entrypointName, OBJC_MSGSEND, _countof(OBJC_MSGSEND) -1) != 0)
+    const void* STDMETHODCALLTYPE MessageSendPInvokeOverride(_In_z_ const char* libraryName, _In_z_ const char* entrypointName)
+    {
+        if (!IsObjectiveCMessageSendFunction(libraryName, entrypointName))
             return nullptr;
 
         for (int i = 0; i < _countof(MsgSendEntryPoints); ++i)
@@ -227,7 +229,7 @@ bool ObjCMarshalNative::IsTrackedReference(_In_ OBJECTREF object, _Out_ bool* is
     return true;
 }
 
-bool ObjCMarshalNative::IsRuntimeMsgSendFunctionOverridden(
+bool ObjCMarshalNative::IsRuntimeMessageSendFunction(
     _In_z_ const char* libraryName,
     _In_z_ const char* entrypointName)
 {
@@ -240,7 +242,7 @@ bool ObjCMarshalNative::IsRuntimeMsgSendFunctionOverridden(
     }
     CONTRACTL_END;
 
-    return MessageSendPInvokeOverride(libraryName, entrypointName) != NULL;
+    return IsObjectiveCMessageSendFunction(libraryName, entrypointName);
 }
 
 namespace
diff --git a/src/coreclr/vm/interoplibinterface_shared.cpp b/src/coreclr/vm/interoplibinterface_shared.cpp
@@ -3,6 +3,7 @@
 
 // Runtime headers
 #include "common.h"
+#include "dllimport.h"
 
 #include "interoplibinterface.h"
 
@@ -24,7 +25,7 @@ bool Interop::ShouldCheckForPendingException(_In_ NDirectMethodDesc* md)
     if (libraryName == NULL || entrypointName == NULL)
         return false;
 
-    if (ObjCMarshalNative::IsRuntimeMsgSendFunctionOverridden(libraryName, entrypointName))
+    if (ObjCMarshalNative::IsRuntimeMessageSendFunction(libraryName, entrypointName))
         return true;
 #endif // FEATURE_OBJCMARSHAL
 
diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp
@@ -3027,6 +3027,8 @@ class NDirectMethodDesc : public MethodDesc
         kIsQCall                        = 0x1000,
 
         kDefaultDllImportSearchPathsStatus = 0x2000, // either method has custom attribute or not.
+
+        kNDirectPopulated               = 0x8000, // Indicate if the NDirect has been fully populated.
     };
 
     // Resolve the import to the NDirect target and set it on the NDirectMethodDesc.
@@ -3159,6 +3161,12 @@ class NDirectMethodDesc : public MethodDesc
         return (ndirect.m_wFlags & kDefaultDllImportSearchPathsIsCached) != 0;
     }
 
+    BOOL IsPopulated()
+    {
+        LIMITED_METHOD_CONTRACT;
+        return (ndirect.m_wFlags & kNDirectPopulated) != 0;
+    }
+
     ULONG DefaultDllImportSearchPathsAttributeCachedValue()
     {
         LIMITED_METHOD_CONTRACT;
diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveC/ObjectiveCMarshal.PlatformNotSupported.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveC/ObjectiveCMarshal.PlatformNotSupported.cs
@@ -127,14 +127,13 @@ public enum MessageSendFunction
         /// <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.
+        /// Providing an override can enable support for Objective-C variadic argument support.
         /// </remarks>
         public static void SetMessageSendCallback(MessageSendFunction msgSendFunction, IntPtr func)
             => throw new PlatformNotSupportedException();
 
         /// <summary>
-        /// Sets a pending exception to be thrown the next time the runtime is entered from an overridden msgSend P/Invoke.
+        /// Sets a pending exception to be thrown the next time the runtime is entered from an Objective-C msgSend P/Invoke.
         /// </summary>
         /// <param name="exception">The exception.</param>
         /// <remarks>
diff --git a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveC/ObjectiveCMarshal.cs b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveC/ObjectiveCMarshal.cs
@@ -160,8 +160,7 @@ public enum MessageSendFunction
         /// <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.
+        /// Providing an override can enable support for Objective-C variadic argument support.
         /// </remarks>
         public static void SetMessageSendCallback(MessageSendFunction msgSendFunction, IntPtr func)
         {
diff --git a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/ObjectiveC/LibObjC.cs b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/ObjectiveC/LibObjC.cs
@@ -44,6 +44,21 @@ public struct objc_super
             public IntPtr super_class;
         }
 
+        public static bool SupportedOnPlatform(MessageSendFunction msgSend)
+        {
+            // The objc_msgSend_fpret, objc_msgSend_stret, and objc_msgSendSuper_stret exports
+            // are not present on the ARM64 platform.
+            if (RuntimeInformation.ProcessArchitecture == Architecture.Arm64
+                && (msgSend == MessageSendFunction.MsgSendFpret
+                    || msgSend == MessageSendFunction.MsgSendStret
+                    || msgSend == MessageSendFunction.MsgSendSuperStret))
+            {
+                return false;
+            }
+
+            return true;
+        }
+
         public static IntPtr CallPInvoke(MessageSendFunction msgSend, IntPtr inst, IntPtr sel)
         {
             switch (msgSend)
diff --git a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/ObjectiveC/MessageSendTests.cs b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/ObjectiveC/MessageSendTests.cs
@@ -104,6 +104,11 @@ private static void SetMessageSendCallbackImpl(MessageSendFunction[] funcsToOver
         {
             foreach (var (msgSend, func) in msgSendOverrides)
             {
+                if (!LibObjC.SupportedOnPlatform(msgSend))
+                {
+                    continue;
+                }
+
                 bool shouldOverride = Array.IndexOf(funcsToOverride, msgSend) >= 0;
 
                 IntPtr expected;
diff --git a/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/ObjectiveC/PendingExceptionTests.cs b/src/libraries/System.Runtime.InteropServices/tests/System/Runtime/InteropServices/ObjectiveC/PendingExceptionTests.cs
diff --git a/src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.Mono.cs b/src/mono/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.Mono.cs
diff --git a/src/tests/issues.targets b/src/tests/issues.targets

Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,7 @@ namespace System.Runtime.InteropServices.ObjectiveC`
`10`	`10`	`public static partial class ObjectiveCMarshal`
`11`	`11`	`{`
`12`	`12`	`/// <summary>`
`13`		`- /// Sets a pending exception to be thrown the next time the runtime is entered from an overridden msgSend P/Invoke.`
	`13`	`+ /// Sets a pending exception to be thrown the next time the runtime is entered from an Objective-C msgSend P/Invoke.`
`14`	`14`	`/// </summary>`
`15`	`15`	`/// <param name="exception">The exception.</param>`
`16`	`16`	`/// <remarks>`
Original file line number	Diff line number	Diff line change
`@@ -797,5 +797,23 @@ private static MarshallerKind GetArrayElementMarshallerKind(`
`797`	`797`	`return MarshallerKind.Invalid;`
`798`	`798`	`}`
`799`	`799`	`}`
	`800`	`+`
	`801`	`+ internal static bool ShouldCheckForPendingException(TargetDetails target, PInvokeMetadata metadata)`
	`802`	`+ {`
	`803`	`+ if (!target.IsOSX)`
	`804`	`+ return false;`
	`805`	`+`
	`806`	`+ const string ObjectiveCLibrary = "/usr/lib/libobjc.dylib";`
	`807`	`+ const string ObjectiveCMsgSend = "objc_msgSend";`
	`808`	`+`
	`809`	`+ // This is for the objc_msgSend suite of functions.`
	`810`	`+ // objc_msgSend`
	`811`	`+ // objc_msgSend_fpret`
	`812`	`+ // objc_msgSend_stret`
	`813`	`+ // objc_msgSendSuper`
	`814`	`+ // objc_msgSendSuper_stret`
	`815`	`+ return metadata.Module.Equals(ObjectiveCLibrary)`
	`816`	`+ && metadata.Name.StartsWith(ObjectiveCMsgSend);`
	`817`	`+ }`
`800`	`818`	`}`
`801`	`819`	`}`