Fix delegates closed over null in Mono

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DelegateTests.cs

@@ -1276,6 +1276,24 @@ public static void CreateDelegate10_Nullable_ClosedDelegate()
             AssertExtensions.Throws<ArgumentException>(
                 () => Delegate.CreateDelegate(typeof(NullableIntToString), num, mi));
         }
+
+        [Fact]
+        public static void CreateDelegate_ClosedOverNull_InstanceMethod()
+        {
+            MethodInfo mi = typeof(C).GetMethod(nameof(C.IsThisNull));
+            Func<bool> del = (Func<bool>)Delegate.CreateDelegate(typeof(Func<bool>), null, mi);
+            Assert.Null(del.Target);
+            Assert.True(del());
+        }
+
+        [Fact]
+        public static void CreateDelegate_ClosedOverNull_InstanceMethodViaMethodInfoCreateDelegate()
+        {
+            MethodInfo mi = typeof(C).GetMethod(nameof(C.IsThisNull));
+            Func<bool> del = mi.CreateDelegate<Func<bool>>(null);
+            Assert.Null(del.Target);
+            Assert.True(del());
+        }
         #endregion Tests
 
         #region Test Setup
@@ -1370,6 +1388,11 @@ public static void S(C c)
             {
             }
 
+            public bool IsThisNull()
+            {
+                return this is null;
+            }
+
             private void PrivateInstance()
             {
             }

src/mono/mono/metadata/loader-internals.h

@@ -53,6 +53,7 @@ typedef struct {
 	GHashTable *runtime_invoke_signature_cache;
 	GHashTable *runtime_invoke_sig_cache;
 	GHashTable *delegate_abstract_invoke_cache;
+	GHashTable *delegate_closed_over_null_cache;
 
 	/*
 	 * indexed by SignaturePointerPair

src/mono/mono/metadata/marshal-lightweight.c

@@ -2056,10 +2056,21 @@ emit_delegate_invoke_internal_ilgen (MonoMethodBuilder *mb, MonoMethodSignature
 			mono_mb_emit_byte (mb, CEE_MONO_LDVIRTFTN_DELEGATE);
 			mono_mb_emit_op (mb, CEE_CALLI, target_method_sig);
 		} else {
+			/* closed_over_null: call the instance method with null as 'this'
+			 * Use an indirect call through method_ptr so the wrapper can be AOT-compiled
+			 * without embedding a specific target method.
+			 */
 			mono_mb_emit_byte (mb, CEE_LDNULL);
 			for (i = 0; i < sig->param_count; ++i)
 				mono_mb_emit_ldarg (mb, i + 1);
-			mono_mb_emit_op (mb, CEE_CALL, target_method);
+			mono_mb_emit_ldarg (mb, 0);
+			mono_mb_emit_ldflda (mb, MONO_STRUCT_OFFSET (MonoDelegate, extra_arg));
+			mono_mb_emit_byte (mb, CEE_LDIND_I);
+			mono_mb_emit_ldarg (mb, 0);
+			mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX);
+			mono_mb_emit_byte (mb, CEE_MONO_LD_DELEGATE_METHOD_PTR);
+			mono_mb_emit_byte (mb, MONO_CUSTOM_PREFIX);
+			mono_mb_emit_op (mb, CEE_MONO_CALLI_EXTRA_ARG, sig);
 		}
 	} else {
 		if (static_method_with_first_arg_bound) {

src/mono/mono/metadata/marshal.c

@@ -2164,15 +2164,14 @@ 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)
+mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt, gboolean static_method_with_first_arg_bound, gboolean closed_over_null, MonoMethod *target_method)
 {
 	MonoMethodSignature *sig, *invoke_sig, *target_method_sig = NULL;
 	MonoMethodBuilder *mb;
 	MonoMethod *res;
 	GHashTable *cache;
 	gpointer cache_key = NULL;
 	char *name;
-	gboolean closed_over_null = FALSE;
 	MonoGenericContext *ctx = NULL;
 	MonoGenericContainer *container = NULL;
 	MonoMethod *orig_method = method;
@@ -2223,18 +2222,34 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
 
 	if (subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL) {
 		/*
-		 * We don't want to use target_method's signature because it can be freed early
+		 * Determine closed_over_null if not already set by the caller.
+		 * When called from AOT, closed_over_null is set explicitly.
+		 * When called at runtime with a target_method, compute from param counts:
+		 * when closed over null, both have the same count; when not closed over
+		 * null, the invoke sig has one extra param that becomes 'this'.
 		 */
-		target_method_sig = mono_metadata_signature_dup_delegate_invoke_to_target (invoke_sig);
+		if (!closed_over_null && target_method) {
+			MonoMethodSignature *actual_target_sig = mono_method_signature_internal (target_method);
+			closed_over_null = sig->param_count == actual_target_sig->param_count;
+		}
 
-		closed_over_null = sig->param_count == target_method_sig->param_count;
+		if (closed_over_null) {
+			subtype = WRAPPER_SUBTYPE_DELEGATE_INVOKE_CLOSED_OVER_NULL;
+		} else {
+			/*
+			 * We don't want to use target_method's signature because it can be freed early
+			 */
+			target_method_sig = mono_metadata_signature_dup_delegate_invoke_to_target (invoke_sig);
+		}
 	}
 
 	/*
 	 * Check cache
 	 */
 	if (ctx) {
-		if (callvirt)
+		if (callvirt && closed_over_null)
+			cache = get_cache (&((MonoMethodInflated*)orig_method)->owner->wrapper_caches.delegate_closed_over_null_cache, mono_aligned_addr_hash, NULL);
+		else if (callvirt)
 			cache = get_cache (&((MonoMethodInflated*)orig_method)->owner->wrapper_caches.delegate_invoke_virtual_cache, mono_aligned_addr_hash, NULL);
 		else
 			cache = get_cache (&((MonoMethodInflated*)orig_method)->owner->wrapper_caches.delegate_invoke_cache, mono_aligned_addr_hash, NULL);
@@ -2260,7 +2275,10 @@ mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt
 	} else if (callvirt) {
 		GHashTable **cache_ptr;
 
-		cache_ptr = &mono_method_get_wrapper_cache (method)->delegate_abstract_invoke_cache;
+		if (closed_over_null)
+			cache_ptr = &mono_method_get_wrapper_cache (method)->delegate_closed_over_null_cache;
+		else
+			cache_ptr = &mono_method_get_wrapper_cache (method)->delegate_abstract_invoke_cache;
 
 		/* We need to cache the signature */
 		mono_marshal_lock ();
@@ -2385,7 +2403,7 @@ mono_marshal_get_delegate_invoke (MonoMethod *method, MonoDelegate *del)
 		target_method = del->method;
 	}
 
-	return mono_marshal_get_delegate_invoke_internal (method, callvirt, static_method_with_first_arg_bound, target_method);
+	return mono_marshal_get_delegate_invoke_internal (method, callvirt, static_method_with_first_arg_bound, FALSE, target_method);
 }
 
 typedef struct {

src/mono/mono/metadata/marshal.h

@@ -142,6 +142,7 @@ typedef enum {
 	/* Subtypes of MONO_WRAPPER_DELEGATE_INVOKE */
 	WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL,
 	WRAPPER_SUBTYPE_DELEGATE_INVOKE_BOUND,
+	WRAPPER_SUBTYPE_DELEGATE_INVOKE_CLOSED_OVER_NULL,
 	/* Subtypes of MONO_WRAPPER_OTHER */
 	WRAPPER_SUBTYPE_GSHAREDVT_IN_SIG,
 	WRAPPER_SUBTYPE_GSHAREDVT_OUT_SIG,
@@ -506,7 +507,7 @@ MonoMethod *
 mono_marshal_get_delegate_invoke (MonoMethod *method, MonoDelegate *del);
 
 MonoMethod *
-mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt, gboolean static_method_with_first_arg_bound, MonoMethod *target_method);
+mono_marshal_get_delegate_invoke_internal (MonoMethod *method, gboolean callvirt, gboolean static_method_with_first_arg_bound, gboolean closed_over_null, MonoMethod *target_method);
 
 WrapperSubtype
 mono_marshal_get_delegate_invoke_subtype (MonoMethod *method, MonoDelegate *del);

src/mono/mono/mini/aot-compiler.c

@@ -3983,7 +3983,7 @@ encode_method_ref (MonoAotCompile *acfg, MonoMethod *method, guint8 *buf, guint8
 				if (method->wrapper_type == MONO_WRAPPER_DELEGATE_INVOKE)
 					encode_value (info ? info->subtype : 0, p, &p);
 
-				if (info && info->subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL)
+				if (info && (info->subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL || info->subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_CLOSED_OVER_NULL))
 					encode_klass_ref (acfg, info->d.delegate_invoke.method->klass, p, &p);
 				else
 					encode_signature (acfg, sig, p, &p);
@@ -5098,7 +5098,7 @@ add_full_aot_wrappers (MonoAotCompile *acfg)
 				inst = mono_class_inflate_generic_method_checked (method, &ctx, error);
 				g_assert (is_ok (error)); /* FIXME don't swallow the error */
 
-				m = mono_marshal_get_delegate_invoke_internal (inst, TRUE, FALSE, NULL);
+				m = mono_marshal_get_delegate_invoke_internal (inst, TRUE, FALSE, FALSE, NULL);
 
 				gshared = mini_get_shared_method_full (m, SHARE_MODE_NONE, error);
 				mono_error_assert_ok (error);
@@ -5112,7 +5112,36 @@ add_full_aot_wrappers (MonoAotCompile *acfg)
 					inst = mono_class_inflate_generic_method_checked (method, &ctx, error);
 					g_assert (is_ok (error)); /* FIXME don't swallow the error */
 
-					m = mono_marshal_get_delegate_invoke_internal (inst, TRUE, FALSE, NULL);
+					m = mono_marshal_get_delegate_invoke_internal (inst, TRUE, FALSE, FALSE, NULL);
+
+					gshared = mini_get_shared_method_full (m, SHARE_MODE_GSHAREDVT, error);
+					mono_error_assert_ok (error);
+
+					add_extra_method (acfg, gshared);
+				}
+			}
+
+			/* closed_over_null variant for generic delegates */
+			{
+				create_ref_shared_inst (acfg, method, &ctx);
+
+				inst = mono_class_inflate_generic_method_checked (method, &ctx, error);
+				g_assert (is_ok (error));
+
+				m = mono_marshal_get_delegate_invoke_internal (inst, TRUE, FALSE, TRUE, NULL);
+
+				gshared = mini_get_shared_method_full (m, SHARE_MODE_NONE, error);
+				mono_error_assert_ok (error);
+
+				add_extra_method (acfg, gshared);
+
+				if (acfg->jit_opts & MONO_OPT_GSHAREDVT) {
+					create_gsharedvt_inst (acfg, method, &ctx);
+
+					inst = mono_class_inflate_generic_method_checked (method, &ctx, error);
+					g_assert (is_ok (error));
+
+					m = mono_marshal_get_delegate_invoke_internal (inst, TRUE, FALSE, TRUE, NULL);
 
 					gshared = mini_get_shared_method_full (m, SHARE_MODE_GSHAREDVT, error);
 					mono_error_assert_ok (error);
@@ -5129,10 +5158,14 @@ add_full_aot_wrappers (MonoAotCompile *acfg)
 
 			sig = mono_method_signature_internal (method);
 			if (sig->param_count && !m_class_is_byreflike (mono_class_from_mono_type_internal (sig->params [0])) && !m_type_is_byref (sig->params [0])) {
-				m = mono_marshal_get_delegate_invoke_internal (method, TRUE, FALSE, NULL);
+				m = mono_marshal_get_delegate_invoke_internal (method, TRUE, FALSE, FALSE, NULL);
 				add_method (acfg, m);
 			}
 
+			/* closed_over_null variant */
+			m = mono_marshal_get_delegate_invoke_internal (method, TRUE, FALSE, TRUE, NULL);
+			add_method (acfg, m);
+
 			method = try_get_method_nofail (klass, "BeginInvoke", -1, 0);
 			if (method)
 				add_method (acfg, mono_marshal_get_delegate_begin_invoke (method));
@@ -5163,7 +5196,7 @@ add_full_aot_wrappers (MonoAotCompile *acfg)
 					/* Add wrappers needed by mono_ftnptr_to_delegate () */
 					invoke = mono_get_delegate_invoke_internal (klass);
 					wrapper = mono_marshal_get_native_func_wrapper_aot (klass);
-					del_invoke = mono_marshal_get_delegate_invoke_internal (invoke, FALSE, TRUE, wrapper);
+					del_invoke = mono_marshal_get_delegate_invoke_internal (invoke, FALSE, TRUE, FALSE, wrapper);
 					add_method (acfg, wrapper);
 					add_method (acfg, del_invoke);
 				}

src/mono/mono/mini/aot-runtime.c

@@ -1320,7 +1320,13 @@ decode_method_ref_with_target (MonoAotModule *module, MethodRef *ref, MonoMethod
 					if (subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_VIRTUAL) {
 						klass = decode_klass_ref (module, p, &p, error);
 						invoke = mono_get_delegate_invoke_internal (klass);
-						ref->method = mono_marshal_get_delegate_invoke_internal(invoke, TRUE, FALSE, NULL);
+						ref->method = mono_marshal_get_delegate_invoke_internal(invoke, TRUE, FALSE, FALSE, NULL);
+						break;
+					}
+					if (subtype == WRAPPER_SUBTYPE_DELEGATE_INVOKE_CLOSED_OVER_NULL) {
+						klass = decode_klass_ref (module, p, &p, error);
+						invoke = mono_get_delegate_invoke_internal (klass);
+						ref->method = mono_marshal_get_delegate_invoke_internal(invoke, TRUE, FALSE, TRUE, NULL);
 						break;
 					}
 

src/mono/mono/mini/interp/interp.c

@@ -4355,6 +4355,9 @@ mono_interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClause
 					} else {
 						LOCAL_VAR (call_args_offset, MonoObject*) = this_arg;
 					}
+				} else if (!m_method_is_static (cmethod->method) && cmethod->param_count == param_count) {
+					// Closed over null: instance method bound with null this
+					LOCAL_VAR (call_args_offset, MonoObject*) = NULL;
 				} else {
 					// skip the delegate pointer for static calls
 					// FIXME we could avoid memmove