Change `callvirt` into `calli` for virtual delegates (#83461)
authorVlad-Alexandru Ionescu <114913836+LeVladIonescu@users.noreply.github.com>
Wed, 26 Apr 2023 06:27:29 +0000 (08:27 +0200)
committerGitHub <noreply@github.com>
Wed, 26 Apr 2023 06:27:29 +0000 (08:27 +0200)
JIT delegates do not depend on the target method and an call is used

---------

Signed-off-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-2.local>
Co-authored-by: Vlad - Alexandru Ionescu <vlad-alexandruionescu@Vlads-MacBook-Pro-5.local>
13 files changed:
src/mono/mono/metadata/icall-signatures.h
src/mono/mono/metadata/icall.c
src/mono/mono/metadata/jit-icall-reg.h
src/mono/mono/metadata/loader-internals.h
src/mono/mono/metadata/marshal-lightweight.c
src/mono/mono/metadata/marshal.c
src/mono/mono/metadata/marshal.h
src/mono/mono/metadata/object-internals.h
src/mono/mono/mini/mini-runtime.c
src/tests/JIT/Methodical/delegate/GSDelegate.cs [new file with mode: 0644]
src/tests/JIT/Methodical/delegate/GSDelegate.csproj [new file with mode: 0644]
src/tests/JIT/Methodical/delegate/VirtualDelegate.cs [new file with mode: 0644]
src/tests/JIT/Methodical/delegate/VirtualDelegate.csproj [new file with mode: 0644]

index 16b35ef..d45f1a5 100644 (file)
@@ -206,6 +206,7 @@ ICALL_SIG (3, (ptr, object, int))           \
 ICALL_SIG (3, (ptr, ptr, int))                 \
 ICALL_SIG (3, (ptr, ptr, int32))               \
 ICALL_SIG (3, (ptr, ptr, ptr))                 \
+ICALL_SIG (3, (ptr, ptr, object))       \
 ICALL_SIG (3, (ptr, ptr, ptrref))              \
 ICALL_SIG (3, (ptr, uint32, ptrref))           \
 ICALL_SIG (3, (uint32, double, double))                \
index 63a83b0..0a7dbc9 100644 (file)
@@ -6176,7 +6176,7 @@ mono_method_get_unmanaged_wrapper_ftnptr_internal (MonoMethod *method, gboolean
        } else {
                g_assert (!only_unmanaged_callers_only);
        }
-       return mono_get_runtime_callbacks ()->get_ftnptr (method, error);
+       return mono_get_runtime_callbacks ()->get_ftnptr (method, FALSE, error);
 }
 
 MonoBoolean
index 984c267..122d8dc 100644 (file)
@@ -203,6 +203,7 @@ MONO_JIT_ICALL (mono_gc_wbarrier_generic_nostore_internal) \
 MONO_JIT_ICALL (mono_gc_wbarrier_range_copy) \
 MONO_JIT_ICALL (mono_gchandle_get_target_internal) \
 MONO_JIT_ICALL (mono_generic_class_init) \
+MONO_JIT_ICALL (mono_get_addr_compiled_method) \
 MONO_JIT_ICALL (mono_get_assembly_object) \
 MONO_JIT_ICALL (mono_get_method_object) \
 MONO_JIT_ICALL (mono_get_native_calli_wrapper) \
index 8356a0b..3f64f48 100644 (file)
@@ -61,11 +61,11 @@ typedef struct {
        GHashTable *delegate_end_invoke_cache;
        GHashTable *runtime_invoke_signature_cache;
        GHashTable *runtime_invoke_sig_cache;
+       GHashTable *delegate_abstract_invoke_cache;
 
        /*
         * indexed by SignaturePointerPair
         */
-       GHashTable *delegate_abstract_invoke_cache;
        GHashTable *delegate_bound_static_invoke_cache;
 
        /*
index 54e37d2..06a2012 100644 (file)
@@ -1990,7 +1990,7 @@ emit_delegate_end_invoke_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *sig)
 }
 
 static void
-emit_delegate_invoke_internal_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoClass *target_class, MonoGenericContext *ctx, MonoGenericContainer *container)
+emit_delegate_invoke_internal_ilgen (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, MonoMethodSignature *target_method_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoClass *target_class, MonoGenericContext *ctx, MonoGenericContainer *container)
 {
        int local_i, local_len, local_delegates, local_d, local_target, local_res = 0;
        int pos0, pos1, pos2;
@@ -2088,19 +2088,12 @@ emit_delegate_invoke_internal_ilgen (MonoMethodBuilder *mb, MonoMethodSignature
 
        if (callvirt) {
                if (!closed_over_null) {
-                       /* if target_method is not really virtual, turn it into a direct call */
-                       if (!(target_method->flags & METHOD_ATTRIBUTE_VIRTUAL) || m_class_is_valuetype (target_class)) {
-                               mono_mb_emit_ldarg (mb, 1);
-                               for (i = 1; i < sig->param_count; ++i)
-                                       mono_mb_emit_ldarg (mb, i + 1);
-                               mono_mb_emit_op (mb, CEE_CALL, target_method);
-                       } else {
-                               mono_mb_emit_ldarg (mb, 1);
-                               mono_mb_emit_op (mb, CEE_CASTCLASS, target_class);
-                               for (i = 1; i < sig->param_count; ++i)
-                                       mono_mb_emit_ldarg (mb, i + 1);
-                               mono_mb_emit_op (mb, CEE_CALLVIRT, target_method);
-                       }
+                       for (i = 1; i <= sig->param_count; ++i)
+                               mono_mb_emit_ldarg (mb, i);
+                       mono_mb_emit_ldarg (mb, 1);
+                       mono_mb_emit_ldarg (mb, 0);
+                       mono_mb_emit_icall (mb, mono_get_addr_compiled_method);
+                       mono_mb_emit_op (mb, CEE_CALLI, target_method_sig);
                } else {
                        mono_mb_emit_byte (mb, CEE_LDNULL);
                        for (i = 0; i < sig->param_count; ++i)
index e7aff60..977ecd9 100644 (file)
@@ -324,6 +324,7 @@ mono_marshal_init (void)
                register_icall (monoeg_g_free, mono_icall_sig_void_ptr, FALSE);
                register_icall (mono_object_isinst_icall, mono_icall_sig_object_object_ptr, TRUE);
                register_icall (mono_struct_delete_old, mono_icall_sig_void_ptr_ptr, FALSE);
+               register_icall (mono_get_addr_compiled_method, mono_icall_sig_ptr_ptr_object, FALSE);
                register_icall (mono_delegate_begin_invoke, mono_icall_sig_object_object_ptr, FALSE);
                register_icall (mono_delegate_end_invoke, mono_icall_sig_object_object_ptr, FALSE);
                register_icall (mono_gc_wbarrier_generic_nostore_internal, mono_icall_sig_void_ptr, TRUE);
@@ -2085,13 +2086,11 @@ free_signature_pointer_pair (SignaturePointerPair *pair)
 MonoMethod *
 mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt, gboolean static_method_with_first_arg_bound, MonoMethod *target_method)
 {
-       MonoMethodSignature *sig, *invoke_sig;
+       MonoMethodSignature *sig, *invoke_sig, *target_method_sig = NULL;
        MonoMethodBuilder *mb;
        MonoMethod *res;
        GHashTable *cache;
        gpointer cache_key = NULL;
-       SignaturePointerPair key = { NULL, NULL };
-       SignaturePointerPair *new_key;
        char *name;
        MonoClass *target_class = NULL;
        gboolean closed_over_null = FALSE;
@@ -2101,7 +2100,6 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
        WrapperInfo *info;
        WrapperSubtype subtype = WRAPPER_SUBTYPE_NONE;
        MonoMemoryManager *mem_manager = NULL;
-       gboolean found;
 
        g_assert (method && m_class_get_parent (method->klass) == mono_defaults.multicastdelegate_class &&
                  !strcmp (method->name, "Invoke"));
@@ -2129,6 +2127,11 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
                }
 
                closed_over_null = sig->param_count == mono_method_signature_internal (target_method)->param_count;
+
+               /*
+                * We don't want to use target_method's signature because it can be freed early
+                */
+               target_method_sig = mono_method_signature_internal (target_method);
        }
 
        if (static_method_with_first_arg_bound) {
@@ -2188,17 +2191,16 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
 
                cache_ptr = &mono_method_get_wrapper_cache (method)->delegate_abstract_invoke_cache;
 
-               /* We need to cache the signature+method pair */
+               /* We need to cache the signature */
                mono_marshal_lock ();
-               if (!*cache_ptr)
-                       *cache_ptr = g_hash_table_new_full (signature_pointer_pair_hash, (GEqualFunc)signature_pointer_pair_equal, (GDestroyNotify)free_signature_pointer_pair, NULL);
-               cache = *cache_ptr;
-               key.sig = invoke_sig;
-               key.pointer = target_method;
-               res = (MonoMethod *)g_hash_table_lookup (cache, &key);
+               cache = get_cache (cache_ptr,
+                                                  (GHashFunc)mono_signature_hash,
+                                                  (GCompareFunc)mono_metadata_signature_equal);
+               res = (MonoMethod *)g_hash_table_lookup (cache, invoke_sig);
                mono_marshal_unlock ();
                if (res)
                        return res;
+               cache_key = invoke_sig;
        } else {
                // Inflated methods should not be in this cache because it's not stored on the imageset.
                g_assert (!method->is_inflated);
@@ -2239,7 +2241,7 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
                /* FIXME: Other subtypes */
                mb->mem_manager = m_method_get_mem_manager (method);
 
-       get_marshal_cb ()->emit_delegate_invoke_internal (mb, sig, invoke_sig, static_method_with_first_arg_bound, callvirt, closed_over_null, method, target_method, target_class, ctx, container);
+       get_marshal_cb ()->emit_delegate_invoke_internal (mb, sig, invoke_sig, target_method_sig, static_method_with_first_arg_bound, callvirt, closed_over_null, method, target_method, target_class, ctx, container);
 
        get_marshal_cb ()->mb_skip_visibility (mb);
 
@@ -2251,13 +2253,6 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
 
                def = mono_mb_create_and_cache_full (cache, cache_key, mb, sig, sig->param_count + 16, info, NULL);
                res = cache_generic_delegate_wrapper (cache, orig_method, def, ctx);
-       } else if (callvirt) {
-               new_key = g_new0 (SignaturePointerPair, 1);
-               *new_key = key;
-
-               res = mono_mb_create_and_cache_full (cache, new_key, mb, sig, sig->param_count + 16, info, &found);
-               if (found)
-                       g_free (new_key);
        } else {
                res = mono_mb_create_and_cache_full (cache, cache_key, mb, sig, sig->param_count + 16, info, NULL);
        }
@@ -5481,6 +5476,49 @@ mono_struct_delete_old (MonoClass *klass, char *ptr)
        }
 }
 
+void*
+mono_get_addr_compiled_method (gpointer arg, MonoDelegate *del)
+{
+       ERROR_DECL (error);
+       MonoMethod *res, *method = del->method;
+       gpointer addr;
+       gboolean need_unbox = FALSE;
+
+       if (arg == NULL) {
+               mono_error_set_null_reference (error);
+               mono_error_set_pending_exception (error);
+               return NULL;
+       }
+
+       MonoClass *klass = del->object.vtable->klass;
+       MonoMethod *invoke = mono_get_delegate_invoke_internal (klass);
+       MonoMethodSignature *invoke_sig = mono_method_signature_internal (invoke);
+
+       MonoClass *arg_class = NULL;
+       if (m_type_is_byref (invoke_sig->params [0])) {
+               arg_class = mono_class_from_mono_type_internal (invoke_sig->params [0]);
+       } else {
+               MonoObject *object = (MonoObject*)arg;
+               arg_class = object->vtable->klass;
+       }
+
+       res = mono_class_get_virtual_method (arg_class, method, error);
+
+       if (!is_ok (error)) {
+               mono_error_set_pending_exception (error);
+               return NULL;
+       }
+
+       need_unbox = m_class_is_valuetype (res->klass) && !m_class_is_valuetype (method->klass);
+       addr = mono_get_runtime_callbacks ()->get_ftnptr (res, need_unbox, error);
+       if (!is_ok (error)) {
+               mono_error_set_pending_exception (error);
+               return NULL;
+       }
+
+       return addr;
+}
+
 void
 ves_icall_System_Runtime_InteropServices_Marshal_DestroyStructure (gpointer src, MonoReflectionTypeHandle type, MonoError *error)
 {
@@ -6255,8 +6293,6 @@ mono_marshal_free_dynamic_wrappers (MonoMethod *method)
         */
        if (image->wrapper_caches.runtime_invoke_method_cache)
                clear_runtime_invoke_method_cache (image->wrapper_caches.runtime_invoke_method_cache, method);
-       if (image->wrapper_caches.delegate_abstract_invoke_cache)
-               g_hash_table_foreach_remove (image->wrapper_caches.delegate_abstract_invoke_cache, signature_pointer_pair_matches_pointer, method);
        // FIXME: Need to clear the caches in other images as well
        if (image->wrapper_caches.delegate_bound_static_invoke_cache)
                g_hash_table_remove (image->wrapper_caches.delegate_bound_static_invoke_cache, mono_method_signature_internal (method));
index 6cd3377..e6ad315 100644 (file)
@@ -328,7 +328,7 @@ typedef struct {
        void (*emit_runtime_invoke_dynamic) (MonoMethodBuilder *mb);
        void (*emit_delegate_begin_invoke) (MonoMethodBuilder *mb, MonoMethodSignature *sig);
        void (*emit_delegate_end_invoke) (MonoMethodBuilder *mb, MonoMethodSignature *sig);
-       void (*emit_delegate_invoke_internal) (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoClass *target_class, MonoGenericContext *ctx, MonoGenericContainer *container);
+       void (*emit_delegate_invoke_internal) (MonoMethodBuilder *mb, MonoMethodSignature *sig, MonoMethodSignature *invoke_sig, MonoMethodSignature *target_method_sig, gboolean static_method_with_first_arg_bound, gboolean callvirt, gboolean closed_over_null, MonoMethod *method, MonoMethod *target_method, MonoClass *target_class, MonoGenericContext *ctx, MonoGenericContainer *container);
        void (*emit_synchronized_wrapper) (MonoMethodBuilder *mb, MonoMethod *method, MonoGenericContext *ctx, MonoGenericContainer *container, MonoMethod *enter_method, MonoMethod *exit_method, MonoMethod *gettypefromhandle_method);
        void (*emit_unbox_wrapper) (MonoMethodBuilder *mb, MonoMethod *method);
        void (*emit_array_accessor_wrapper) (MonoMethodBuilder *mb, MonoMethod *method, MonoMethodSignature *sig, MonoGenericContext *ctx);
@@ -624,6 +624,10 @@ ICALL_EXPORT
 void
 mono_struct_delete_old (MonoClass *klass, char *ptr);
 
+ICALL_EXPORT
+void*
+mono_get_addr_compiled_method (gpointer arg, MonoDelegate *del);
+
 int
 mono_emit_marshal (EmitMarshalContext *m, int argnum, MonoType *t,
              MonoMarshalSpec *spec, int conv_arg,
index 07ecbff..be4ce48 100644 (file)
@@ -695,7 +695,7 @@ typedef struct {
        void (*get_jit_stats)(gint64 *methods_compiled, gint64 *cil_code_size_bytes, gint64 *native_code_size_bytes, gint64 *jit_time);
        void (*get_exception_stats)(guint32 *exception_count);
        // Same as compile_method, but returns a MonoFtnDesc in llvmonly mode
-       gpointer (*get_ftnptr)(MonoMethod *method, MonoError *error);
+       gpointer (*get_ftnptr)(MonoMethod *method, gboolean need_unbox, MonoError *error);
        void (*interp_jit_info_foreach)(InterpJitInfoFunc func, gpointer user_data);
        gboolean (*interp_sufficient_stack)(gsize size);
        void (*init_class) (MonoClass *klass);
index 18a2370..cc20695 100644 (file)
@@ -2817,11 +2817,11 @@ mono_jit_compile_method_jit_only (MonoMethod *method, MonoError *error)
  * On llvmonly, this returns a MonoFtnDesc, otherwise it returns a normal function pointer.
  */
 static gpointer
-get_ftnptr_for_method (MonoMethod *method, MonoError *error)
+get_ftnptr_for_method (MonoMethod *method, gboolean need_unbox, MonoError *error)
 {
        if (!mono_llvm_only) {
                gpointer res = mono_jit_compile_method (method, error);
-               res = mini_add_method_trampoline (method, res, mono_method_needs_static_rgctx_invoke (method, TRUE), FALSE);
+               res = mini_add_method_trampoline (method, res, mono_method_needs_static_rgctx_invoke (method, TRUE), need_unbox);
                return res;
        } else {
                return mini_llvmonly_load_method_ftndesc (method, FALSE, FALSE, error);
diff --git a/src/tests/JIT/Methodical/delegate/GSDelegate.cs b/src/tests/JIT/Methodical/delegate/GSDelegate.cs
new file mode 100644 (file)
index 0000000..cbf80ad
--- /dev/null
@@ -0,0 +1,53 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Reflection;
+
+
+public interface IGetContents<T> {
+    (string, int, T) GetContents();
+}
+public struct MyStruct<T> : IGetContents<T> {
+    public string s;
+    public int a;
+    public T t;
+
+    public (string, int, T) GetContents()
+    {
+        return (s, a, t);
+    }
+}
+
+public class Program {
+
+    public delegate (string, int, T) MyDelegate<T>(IGetContents<T> arg);
+
+    public static int Main(string[] args)
+    {
+        int retVal = 100;
+
+        try {
+            MyStruct<string> myStruct = new MyStruct<string>();
+            myStruct.s = "test1";
+            myStruct.a = 42;
+            myStruct.t = "test2";
+
+            MethodInfo mi = typeof(IGetContents<string>).GetMethod("GetContents");
+            MyDelegate<string> func = (MyDelegate<string>)mi.CreateDelegate(typeof(MyDelegate<string>));
+
+            (string c1, int c2, string c3) = func(myStruct);
+            if (c1 != "test1")
+                retVal = 1;
+            if (c2 != 42)
+                retVal = 2;
+            if (c3 != "test2")
+                retVal = 3;
+        } catch (Exception e) {
+            Console.WriteLine(e);
+            retVal = 1;
+        }
+
+        return retVal;
+    }
+}
diff --git a/src/tests/JIT/Methodical/delegate/GSDelegate.csproj b/src/tests/JIT/Methodical/delegate/GSDelegate.csproj
new file mode 100644 (file)
index 0000000..96112c6
--- /dev/null
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <RequiresProcessIsolation>true</RequiresProcessIsolation>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>PdbOnly</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="GSDelegate.cs" />
+  </ItemGroup>
+</Project>
diff --git a/src/tests/JIT/Methodical/delegate/VirtualDelegate.cs b/src/tests/JIT/Methodical/delegate/VirtualDelegate.cs
new file mode 100644 (file)
index 0000000..5a28c68
--- /dev/null
@@ -0,0 +1,24 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+
+using System;
+using System.Runtime.InteropServices;
+
+public class VirtualDelegate
+{
+    public static int Main () {
+        int retVal = 100;
+        try {
+            var del = (Func<string, string>)Delegate.CreateDelegate (typeof (Func<string, string>), null, typeof (object).GetMethod ("ToString"));
+            if (del ("FOO") != "FOO")
+                retVal = 1;
+        } catch(Exception e) {
+            Console.WriteLine(e);
+            retVal = 1;
+        }
+        
+        return retVal;
+        
+    }
+}
diff --git a/src/tests/JIT/Methodical/delegate/VirtualDelegate.csproj b/src/tests/JIT/Methodical/delegate/VirtualDelegate.csproj
new file mode 100644 (file)
index 0000000..c8f15c2
--- /dev/null
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <RequiresProcessIsolation>true</RequiresProcessIsolation>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>PdbOnly</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="VirtualDelegate.cs" />
+  </ItemGroup>
+</Project>