[hot_reload] implement param reflection (#77563)

* Add new test ReflectionAddNewMethod * FIXME: get_param_names, get_marshal_info and custom_attrs need work * WIP - add a method param reverse lookup * look up params from added methods * Remove FIXMEs and unused field * remove writelines from test * fix test on coreclr * why does coreclr have 2 attributes here?? * There should be 2 attributes on the 4th param * one more place that looks at params * A couple more places where we look at the Params table * Check default values on params on added method * fix lookup if table is empty * add a gratuitious typeof assert otherwise the CancellationToken type is trimmed on wasm * Add a single mono_metadata_get_method_params function remove duplicated code
dotnet · Nov 2, 2022 · a27ecc5 · a27ecc5
1 parent 9a138bf
commit a27ecc5
Show file tree

Hide file tree

Showing 17 changed files with 325 additions and 70 deletions.
diff --git a/...tem.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod/ReflectionAddNewMethod.cs b/...tem.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod/ReflectionAddNewMethod.cs
@@ -0,0 +1,16 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+using System;
+
+
+namespace System.Reflection.Metadata.ApplyUpdate.Test
+{
+    public class ReflectionAddNewMethod
+    {
+        public string ExistingMethod(string u, double f)
+	{
+            return u + f.ToString();;
+        }
+
+    }
+}
diff --git a/....Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod/ReflectionAddNewMethod_v1.cs b/....Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod/ReflectionAddNewMethod_v1.cs
@@ -0,0 +1,21 @@
+// 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.CompilerServices;
+using CancellationToken = System.Threading.CancellationToken;
+
+namespace System.Reflection.Metadata.ApplyUpdate.Test
+{
+    public class ReflectionAddNewMethod
+    {
+        public string ExistingMethod(string u, double f)
+	{
+            return u + f.ToString();;
+        }
+
+	public double AddedNewMethod(char c, float h, string w, CancellationToken ct = default, [CallerMemberName] string callerName = "")
+	{
+	    return ((double)Convert.ToInt32(c)) + h;
+	}
+    }
+}
diff --git a/...ionAddNewMethod/System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod.csproj b/...ionAddNewMethod/System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod.csproj
@@ -0,0 +1,11 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <RootNamespace>System.Runtime.Loader.Tests</RootNamespace>
+    <TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
+    <TestRuntime>true</TestRuntime>
+    <DeltaScript>deltascript.json</DeltaScript>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="ReflectionAddNewMethod.cs" />
+  </ItemGroup>
+</Project>
diff --git a/...pdate/System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod/deltascript.json b/...pdate/System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod/deltascript.json
@@ -0,0 +1,6 @@
+{
+    "changes": [
+        {"document": "ReflectionAddNewMethod.cs", "update": "ReflectionAddNewMethod_v1.cs"},
+    ]
+}
+
diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs
@@ -3,6 +3,7 @@
 
 using System.Reflection;
 using System.Runtime.CompilerServices;
+using System.Runtime.InteropServices;
 using Xunit;
 
 namespace System.Reflection.Metadata
@@ -629,5 +630,87 @@ public static void TestReflectionAddNewType()
                 System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType.ZExistingClass.ExistingMethod ();
             });
         }
+
+        [ConditionalFact(typeof(ApplyUpdateUtil), nameof(ApplyUpdateUtil.IsSupported))]
+        public static void TestReflectionAddNewMethod()
+        {
+            ApplyUpdateUtil.TestCase(static () =>
+            {
+                var ty = typeof(System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod);
+                var assm = ty.Assembly;
+
+		var bindingFlags = BindingFlags.Instance | BindingFlags.Public;
+                var allMethods = ty.GetMethods(bindingFlags);
+
+                int objectMethods = typeof(object).GetMethods(bindingFlags).Length;
+                Assert.Equal (objectMethods + 1, allMethods.Length);
+
+                ApplyUpdateUtil.ApplyUpdate(assm);
+                ApplyUpdateUtil.ClearAllReflectionCaches();
+
+                ty = typeof(System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod);
+
+                allMethods = ty.GetMethods(bindingFlags);
+                Assert.Equal (objectMethods + 2, allMethods.Length);
+
+                var mi = ty.GetMethod ("AddedNewMethod");
+
+                Assert.NotNull (mi);
+
+                var retParm = mi.ReturnParameter;
+                Assert.NotNull (retParm);
+                Assert.NotNull (retParm.ParameterType);
+                Assert.Equal (-1, retParm.Position);
+
+                var retCas = retParm.GetCustomAttributes(false);
+                Assert.NotNull(retCas);
+                Assert.Equal(0, retCas.Length);
+
+                var parms = mi.GetParameters();
+                Assert.Equal (5, parms.Length);
+
+                int parmPos = 0;
+                foreach (var parm in parms)
+                {
+                    Assert.NotNull(parm);
+                    Assert.NotNull(parm.ParameterType);
+                    Assert.Equal(parmPos, parm.Position);
+                    Assert.NotNull(parm.Name);
+                    
+                    var cas = parm.GetCustomAttributes(false);
+                    foreach (var ca in cas) {
+                        Assert.NotNull (ca);
+                    }
+
+                    parmPos++;
+                }
+
+		var parmAttrs = parms[4].GetCustomAttributes(false);
+                Assert.Equal (2, parmAttrs.Length);
+		bool foundCallerMemberName = false;
+		bool foundOptional = false;
+		foreach (var pa in parmAttrs) {
+		    if (typeof (CallerMemberNameAttribute).Equals(pa.GetType()))
+		    {
+			foundCallerMemberName = true;
+		    }
+		    if (typeof (OptionalAttribute).Equals(pa.GetType()))
+		    {
+			foundOptional = true;
+		    }
+		}
+		Assert.True(foundCallerMemberName);
+		Assert.True(foundOptional);
+
+		// n.b. this typeof() also makes the rest of the test work on Wasm with aggressive trimming.
+		Assert.Equal (typeof(System.Threading.CancellationToken), parms[3].ParameterType);
+
+                Assert.True(parms[3].HasDefaultValue);
+		Assert.True(parms[4].HasDefaultValue);
+
+		Assert.Null(parms[3].DefaultValue);
+		Assert.Equal(string.Empty, parms[4].DefaultValue);
+            });
+	} 
     }
 }
diff --git a/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj b/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj
@@ -62,6 +62,7 @@
     <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticLambda\System.Reflection.Metadata.ApplyUpdate.Test.AddStaticLambda.csproj" />
     <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression\System.Reflection.Metadata.ApplyUpdate.Test.StaticLambdaRegression.csproj" />
     <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType\System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType.csproj" />
+    <ProjectReference Include="ApplyUpdate\System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod\System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewMethod.csproj" />
   </ItemGroup>
   <ItemGroup Condition="'$(TargetOS)' == 'Browser'">
     <WasmFilesToIncludeFromPublishDir Include="$(AssemblyName).dll" />

diff --git a/src/mono/mono/component/hot_reload-internals.h b/src/mono/mono/component/hot_reload-internals.h
@@ -86,4 +86,9 @@ typedef struct _MonoClassMetadataUpdateEvent {
 	uint32_t token; /* the Event table token where this event was defined. */
 } MonoClassMetadataUpdateEvent;
 
+typedef struct _MonoMethodMetadataUpdateParamInfo {
+	uint32_t first_param_token; /* a Param token */
+	uint32_t param_count;
+} MonoMethodMetadataUpdateParamInfo;
+
 #endif/*_MONO_COMPONENT_HOT_RELOAD_INTERNALS_H*/
diff --git a/src/mono/mono/component/hot_reload-stub.c b/src/mono/mono/component/hot_reload-stub.c
@@ -110,6 +110,9 @@ hot_reload_get_num_methods_added (MonoClass *klass);
 static const char *
 hot_reload_get_capabilities (void);
 
+static uint32_t
+hot_reload_stub_get_method_params (MonoImage *base_image, uint32_t methoddef_token, uint32_t *out_param_count_opt);
+
 static MonoComponentHotReload fn_table = {
 	{ MONO_COMPONENT_ITF_VERSION, &hot_reload_stub_available },
 	&hot_reload_stub_set_fastpath_data,
@@ -142,7 +145,8 @@ static MonoComponentHotReload fn_table = {
 	&hot_reload_stub_added_fields_iter,
 	&hot_reload_get_num_fields_added,
 	&hot_reload_get_num_methods_added,
-	&hot_reload_get_capabilities
+	&hot_reload_get_capabilities,
+	&hot_reload_stub_get_method_params,
 };
 
 static bool
@@ -343,6 +347,12 @@ hot_reload_get_capabilities (void)
 	return "";
 }
 
+static uint32_t
+hot_reload_stub_get_method_params (MonoImage *base_image, uint32_t methoddef_token, uint32_t *out_param_count_opt)
+{
+	return 0;
+}
+
 MONO_COMPONENT_EXPORT_ENTRYPOINT
 MonoComponentHotReload *
 mono_component_hot_reload_init (void)

diff --git a/src/mono/mono/component/hot_reload.c b/src/mono/mono/component/hot_reload.c
@@ -144,6 +144,9 @@ hot_reload_get_num_methods_added (MonoClass *klass);
 static const char *
 hot_reload_get_capabilities (void);
 
+static uint32_t
+hot_reload_get_method_params (MonoImage *base_image, uint32_t methoddef_token, uint32_t *out_param_count_opt);
+
 static MonoClassMetadataUpdateField *
 metadata_update_field_setup_basic_info (MonoImage *image_base, BaselineInfo *base_info, uint32_t generation, DeltaInfo *delta_info, MonoClass *parent_klass, uint32_t fielddef_token, uint32_t field_flags);
 
@@ -179,7 +182,8 @@ static MonoComponentHotReload fn_table = {
 	&hot_reload_added_fields_iter,
 	&hot_reload_get_num_fields_added,
 	&hot_reload_get_num_methods_added,
-	&hot_reload_get_capabilities
+	&hot_reload_get_capabilities,
+	&hot_reload_get_method_params,
 };
 
 MonoComponentHotReload *
@@ -272,6 +276,9 @@ struct _BaselineInfo {
 	/* Parents for added methods, fields, etc */
 	GHashTable *member_parent; /* maps added methoddef or fielddef tokens to typedef tokens */
 
+	/* Params for added methods */
+	GHashTable *method_params; /* maps methoddef tokens to a MonoClassMetadataUpdateMethodParamInfo* */
+
 	/* Skeletons for all newly-added types from every generation. Accessing the array requires the image lock. */
 	GArray *skeletons;
 };
@@ -412,6 +419,12 @@ baseline_info_destroy (BaselineInfo *info)
 	if (info->skeletons)
 		g_array_free (info->skeletons, TRUE);
 
+	if (info->member_parent)
+		g_hash_table_destroy (info->member_parent);
+
+	if (info->method_params)
+		g_hash_table_destroy (info->method_params);
+
 	g_free (info);
 }
 
@@ -627,6 +640,11 @@ add_method_to_baseline (BaselineInfo *base_info, DeltaInfo *delta_info, MonoClas
 static void
 add_field_to_baseline (BaselineInfo *base_info, DeltaInfo *delta_info, MonoClass *klass, uint32_t field_token);
 
+/* Add a method->params lookup for new methods in existing classes */
+static void
+add_param_info_for_method (BaselineInfo *base_info, uint32_t param_token, uint32_t method_token);
+
+
 void
 hot_reload_init (void)
 {
@@ -2019,6 +2037,7 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 	uint32_t add_member_typedef = 0;
 	uint32_t add_property_propertymap = 0;
 	uint32_t add_event_eventmap = 0;
+	uint32_t add_field_method = 0;
 
 	gboolean assemblyref_updated = FALSE;
 	for (guint32 i = 0; i < rows ; ++i) {
@@ -2049,6 +2068,7 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 
 		case ENC_FUNC_ADD_PARAM: {
 			g_assert (token_table == MONO_TABLE_METHOD);
+			add_field_method = log_token;
 			break;
 		}
 		case ENC_FUNC_ADD_FIELD: {
@@ -2115,8 +2135,11 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 		}
 		case MONO_TABLE_METHOD: {
 			/* if adding a param, handle it with the next record */
-			if (func_code == ENC_FUNC_ADD_PARAM)
+			if (func_code == ENC_FUNC_ADD_PARAM) {
+				g_assert (is_addition);
 				break;
+			}
+			g_assert (func_code == ENC_FUNC_DEFAULT);
 
 			if (!base_info->method_table_update)
 				base_info->method_table_update = g_hash_table_new (g_direct_hash, g_direct_equal);
@@ -2366,9 +2389,21 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 			 *
 			 * So by the time we see the param additions, the methods are already in.
 			 *
-			 * FIXME: we need a lookaside table (like member_parent) for every place
-			 * that looks at MONO_METHOD_PARAMLIST
 			 */
+			if (is_addition) {
+				g_assert (add_field_method != 0);
+				uint32_t parent_type_token = hot_reload_method_parent (image_base, add_field_method);
+				g_assert (parent_type_token != 0); // we added a parameter to a method that was added
+				if (pass2_context_is_skeleton (ctx, parent_type_token)) {
+					// it's a parameter on a new method in a brand new class
+					// FIXME: need to do something here?
+				} else {
+					// it's a parameter on a new method in an existing class
+					add_param_info_for_method (base_info, log_token, add_field_method);
+				}
+				add_field_method = 0;
+				break;
+			}
 			break;
 		}
 		case MONO_TABLE_INTERFACEIMPL: {
@@ -2819,6 +2854,28 @@ hot_reload_field_parent (MonoImage *base_image, uint32_t field_token)
 }
 
 
+static void
+add_param_info_for_method (BaselineInfo *base_info, uint32_t param_token, uint32_t method_token)
+{
+	if (!base_info->method_params) {
+		base_info->method_params = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_free);
+	}
+	MonoMethodMetadataUpdateParamInfo* info = NULL;
+	info = g_hash_table_lookup (base_info->method_params, GUINT_TO_POINTER (method_token));
+	if (!info) {
+		// FIXME locking
+		info = g_new0 (MonoMethodMetadataUpdateParamInfo, 1);
+		g_hash_table_insert (base_info->method_params, GUINT_TO_POINTER (method_token), info);
+		info->first_param_token = param_token;
+		info->param_count = 1;
+	} else {
+		uint32_t param_index = mono_metadata_token_index (param_token);
+		// expect params for a single method to be a contiguous sequence of rows
+		g_assert (mono_metadata_token_index (info->first_param_token) + info->param_count == param_index);
+		info->param_count++;
+	}
+}
+
 /* HACK - keep in sync with locator_t in metadata/metadata.c */
 typedef struct {
 	guint32 idx;			/* The index that we are trying to locate */
@@ -3148,6 +3205,32 @@ hot_reload_get_num_methods_added (MonoClass *klass)
 	return count;
 }
 
+static uint32_t
+hot_reload_get_method_params (MonoImage *base_image, uint32_t methoddef_token, uint32_t *out_param_count_opt)
+{
+	BaselineInfo *base_info = baseline_info_lookup (base_image);
+	g_assert (base_info);
+
+	/* FIXME: locking in case the hash table grows */
+
+	if (!base_info->method_params)
+		return 0;
+
+	MonoMethodMetadataUpdateParamInfo* info = NULL;
+	info = g_hash_table_lookup (base_info->method_params, GUINT_TO_POINTER (methoddef_token));
+	if (!info) {
+		if (out_param_count_opt)
+			*out_param_count_opt = 0;
+		return 0;
+	}
+
+	if (out_param_count_opt)
+		*out_param_count_opt = info->param_count;
+
+	return mono_metadata_token_index (info->first_param_token);
+}
+
+
 static const char *
 hot_reload_get_capabilities (void)
 {

diff --git a/src/mono/mono/component/hot_reload.h b/src/mono/mono/component/hot_reload.h
@@ -48,6 +48,7 @@ typedef struct _MonoComponentHotReload {
 	uint32_t (*get_num_fields_added) (MonoClass *klass);
 	uint32_t (*get_num_methods_added) (MonoClass *klass);
 	const char* (*get_capabilities) (void);
+	uint32_t (*get_method_params) (MonoImage *base_image, uint32_t methoddef_token, uint32_t *out_param_count_opt);
 } MonoComponentHotReload;
 
 MONO_COMPONENT_EXPORT_ENTRYPOINT