[release/6.0] Fix crossgen2 calli GC hole by unifying MethodDesc/MethodSignature...

* Unify MethodDesc/MethodSignature IsMarshallingRequired logic

For unmanaged calli crossgen2 was not properly checking all necessary
conditions for a pinvoke being required. In particular it did not check
for managed byrefs. Unify the MethodDesc/MethodSignature logic to get
all the checks.

Fix #58259

* Add a simple regression test

* Fix test build

Co-authored-by: Jakob Botsch Nielsen <jakob.botsch.nielsen@gmail.com>

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
index 7da42e0..927148c 100644 (file)
--- a/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs
+++ b/src/coreclr/tools/aot/ILCompiler.ReadyToRun/Interop/IL/Marshaller.ReadyToRun.cs
@@ -40,21 +40,18 @@ namespace Internal.TypeSystem.Interop
             }
         }
 
-        public static Marshaller[] GetMarshallersForMethod(MethodDesc targetMethod)
+        private static Marshaller[] GetMarshallers(
+            MethodSignature methodSig,
+            PInvokeFlags flags,
+            ParameterMetadata[] parameterMetadataArray)
         {
-            Debug.Assert(targetMethod.IsPInvoke);
-
-            MarshalDirection direction = MarshalDirection.Forward;
-            MethodSignature methodSig = targetMethod.Signature;
-            PInvokeFlags flags = targetMethod.GetPInvokeMethodMetadata().Flags;
-
-            ParameterMetadata[] parameterMetadataArray = targetMethod.GetParameterMetadata();
             Marshaller[] marshallers = new Marshaller[methodSig.Length + 1];
-            ParameterMetadata parameterMetadata;
 
             for (int i = 0, parameterIndex = 0; i < marshallers.Length; i++)
             {
                 Debug.Assert(parameterIndex == parameterMetadataArray.Length || i <= parameterMetadataArray[parameterIndex].Index);
+
+                ParameterMetadata parameterMetadata;
                 if (parameterIndex == parameterMetadataArray.Length || i < parameterMetadataArray[parameterIndex].Index)
                 {
                     // if we don't have metadata for the parameter, create a dummy one
@@ -72,7 +69,7 @@ namespace Internal.TypeSystem.Interop
                                                     methodSig.GetEmbeddedSignatureData(),
                                                     MarshallerType.Argument,
                                                     parameterMetadata.MarshalAsDescriptor,
-                                                    direction,
+                                                    MarshalDirection.Forward,
                                                     marshallers,
                                                     parameterMetadata.Index,
                                                     flags,
@@ -84,6 +81,24 @@ namespace Internal.TypeSystem.Interop
             return marshallers;
         }
 
+
+        public static Marshaller[] GetMarshallersForMethod(MethodDesc targetMethod)
+        {
+            Debug.Assert(targetMethod.IsPInvoke);
+            return GetMarshallers(
+                targetMethod.Signature,
+                targetMethod.GetPInvokeMethodMetadata().Flags,
+                targetMethod.GetParameterMetadata());
+        }
+
+        public static Marshaller[] GetMarshallersForSignature(MethodSignature methodSig, ParameterMetadata[] paramMetadata)
+        {
+            return GetMarshallers(
+                methodSig,
+                new PInvokeFlags(PInvokeAttributes.None),
+                paramMetadata);
+        }
+
         public static bool IsMarshallingRequired(MethodDesc targetMethod)
         {
             Debug.Assert(targetMethod.IsPInvoke);
@@ -115,25 +130,10 @@ namespace Internal.TypeSystem.Interop
 
         public static bool IsMarshallingRequired(MethodSignature methodSig, ParameterMetadata[] paramMetadata)
         {
-            for (int i = 0, paramIndex = 0; i < methodSig.Length + 1; i++)
+            Marshaller[] marshallers = GetMarshallersForSignature(methodSig, paramMetadata);
+            for (int i = 0; i < marshallers.Length; i++)
             {
-                ParameterMetadata parameterMetadata = (paramIndex == paramMetadata.Length || i < paramMetadata[paramIndex].Index) ?
-                    new ParameterMetadata(i, ParameterMetadataAttributes.None, null) :
-                    paramMetadata[paramIndex++];
-
-                TypeDesc parameterType = (i == 0) ? methodSig.ReturnType : methodSig[i - 1];  //first item is the return type
-
-                MarshallerKind marshallerKind = MarshalHelpers.GetMarshallerKind(
-                    parameterType,
-                    parameterIndex: i,
-                    customModifierData: methodSig.GetEmbeddedSignatureData(),
-                    parameterMetadata.MarshalAsDescriptor,
-                    parameterMetadata.Return,
-                    isAnsi: true,
-                    MarshallerType.Argument,
-                    out MarshallerKind elementMarshallerKind);
-
-                if (IsMarshallingRequired(marshallerKind))
+                if (marshallers[i].IsMarshallingRequired())
                     return true;
             }
 

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.cs b/src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.cs
new file mode 100644 (file)
index 0000000..7e49a3c
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.cs
@@ -0,0 +1,27 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+public unsafe class Runtime_58259
+{
+    public static int Main()
+    {
+        M(out _);
+        return 100;
+    }
+
+    static delegate* unmanaged<out int, void> _f;
+
+    public static void M(out int index)
+    {
+        if (_f != null)
+        {
+            _f(out index);
+            _f(out index);
+        }
+        else
+        {
+            index = 0;
+        }
+    }
+}
+

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.csproj
new file mode 100644 (file)
index 0000000..d1dc805
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.csproj
@@ -0,0 +1,14 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+    <CLRTestPriority>1</CLRTestPriority> <!-- This is a regression test for a crossgen only scenario -->
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>