Skip to content

Commit

Permalink
[Mono] Add mono hot reload support for updating parameter name (dotne…
Browse files Browse the repository at this point in the history 
…t#85796)

Fixes dotnet#56626
Fixes dotnet#50978

* Add mono hot reload support for updating parameter name

* Add a debugger test

* Don't invalidate the whole assembly when using hot reload

* don't ignore reflection cache entries when the "no invalidate" flag is set

---------

Co-authored-by: Aleksey Kliger <alklig@microsoft.com>
  • Loading branch information
@fanyang-mono @lambdageek
fanyang-mono and lambdageek committed Jun 16, 2023
1 parent e47c4ab commit cc7448c
Show file tree
Hide file tree
Showing 8 changed files with 155 additions and 49 deletions.
26 changes: 2 additions & 24 deletions src/mono/mono/component/hot_reload.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1600,29 +1600,7 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, DeltaInfo *de
}
case MONO_TABLE_PARAM: {
*should_invalidate_transformed_code = true;
if (!is_addition) {
/* We only allow modifications where the parameter name doesn't change. */
uint32_t base_param [MONO_PARAM_SIZE];
uint32_t upd_param [MONO_PARAM_SIZE];
int mapped_token = hot_reload_relative_delta_index (image_dmeta, delta_info, mono_metadata_make_token (token_table, token_index));
g_assert (mapped_token != -1);
mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x PARAM update. mapped index = 0x%08x\n", i, log_token, mapped_token);

mono_metadata_decode_row (&image_dmeta->tables [MONO_TABLE_PARAM], mapped_token - 1, upd_param, MONO_PARAM_SIZE);
mono_metadata_decode_row (&image_base->tables [MONO_TABLE_PARAM], token_index - 1, base_param, MONO_PARAM_SIZE);

const char *base_name = mono_metadata_string_heap (image_base, base_param [MONO_PARAM_NAME]);
const char *upd_name = mono_metadata_string_heap (image_base, upd_param [MONO_PARAM_NAME]);
mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x: 0x%08x PARAM update: seq = %d (base = %d), name = '%s' (base = '%s')\n", i, log_token, upd_param [MONO_PARAM_SEQUENCE], base_param [MONO_PARAM_SEQUENCE], upd_name, base_name);
if (strcmp (base_name, upd_name) != 0 || base_param [MONO_PARAM_SEQUENCE] != upd_param [MONO_PARAM_SEQUENCE]) {
mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing PARAM table cols.", i, log_token);
mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing PARAM table cols. token=0x%08x", log_token);
unsupported_edits = TRUE;
continue;
}
break;
} else
break; /* added a row. ok */
break;
}
case MONO_TABLE_TYPEDEF: {
*should_invalidate_transformed_code = true;
Expand Down Expand Up @@ -3493,7 +3471,7 @@ hot_reload_get_method_params (MonoImage *base_image, uint32_t methoddef_token, u
static const char *
hot_reload_get_capabilities (void)
{
return "Baseline AddMethodToExistingType AddStaticFieldToExistingType NewTypeDefinition ChangeCustomAttributes AddInstanceFieldToExistingType GenericAddMethodToExistingType GenericUpdateMethod";
return "Baseline AddMethodToExistingType AddStaticFieldToExistingType NewTypeDefinition ChangeCustomAttributes AddInstanceFieldToExistingType GenericAddMethodToExistingType GenericUpdateMethod UpdateParameters";
}

static GENERATE_GET_CLASS_WITH_CACHE_DECL (hot_reload_instance_field_table);
Expand Down
15 changes: 10 additions & 5 deletions src/mono/mono/metadata/metadata.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5126,12 +5126,17 @@ mono_metadata_custom_attrs_from_index (MonoImage *meta, guint32 index)
/* FIXME: Index translation */

gboolean found = tdef->base && mono_binary_search (&loc, tdef->base, table_info_get_rows (tdef), tdef->row_size, table_locator) != NULL;
if (!found && !meta->has_updates)
return 0;

if (G_UNLIKELY (meta->has_updates)) {
if (!found && !mono_metadata_update_metadata_linear_search (meta, tdef, &loc, table_locator))
if (!found) {
if (G_LIKELY (!meta->has_updates)) {
return 0;
} else {
if ((mono_metadata_table_num_rows (meta, MONO_TABLE_CUSTOMATTRIBUTE) > table_info_get_rows (tdef))) {
if (!mono_metadata_update_metadata_linear_search (meta, tdef, &loc, table_locator))
return 0;
} else {
return 0;
}
}
}

/* Find the first entry by searching backwards */
Expand Down
58 changes: 48 additions & 10 deletions src/mono/mono/metadata/reflection-cache.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <mono/metadata/mono-hash.h>
#include <mono/metadata/mempool.h>
#include <mono/utils/mono-error-internals.h>
#include <mono/metadata/metadata-update.h>

/*
* We need to return always the same object for MethodInfo, FieldInfo etc..
Expand All @@ -22,8 +23,14 @@
typedef struct {
gpointer item;
MonoClass *refclass;
uint32_t generation; /* 0 is normal; hot reload may change it */
} ReflectedEntry;

enum {
MONO_REFL_CACHE_DEFAULT = 0,
MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE = 1,
};

gboolean
mono_reflected_equal (gconstpointer a, gconstpointer b);

Expand All @@ -46,7 +53,7 @@ free_reflected_entry (ReflectedEntry *entry)
}

static inline MonoObject*
cache_object (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer item, MonoObject* o)
cache_object (MonoMemoryManager *mem_manager, int flags, MonoClass *klass, gpointer item, MonoObject* o)
{
MonoObject *obj;
ReflectedEntry pe;
Expand All @@ -59,6 +66,10 @@ cache_object (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer item, M
ReflectedEntry *e = alloc_reflected_entry (mem_manager);
e->item = item;
e->refclass = klass;
if (G_UNLIKELY(mono_metadata_has_updates()) && ((flags & MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE) == 0))
e->generation = mono_metadata_update_get_thread_generation();
else
e->generation = 0;
mono_conc_g_hash_table_insert (mem_manager->refobject_hash, e, o);
obj = o;
}
Expand All @@ -67,7 +78,7 @@ cache_object (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer item, M
}

static inline MonoObjectHandle
cache_object_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer item, MonoObjectHandle o)
cache_object_handle (MonoMemoryManager *mem_manager, int flags, MonoClass *klass, gpointer item, MonoObjectHandle o)
{
MonoObjectHandle obj;
ReflectedEntry pe;
Expand All @@ -83,6 +94,10 @@ cache_object_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer
ReflectedEntry *e = alloc_reflected_entry (mem_manager);
e->item = item;
e->refclass = klass;
if (G_UNLIKELY(mono_metadata_has_updates()) && ((flags & MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE) == 0))
e->generation = mono_metadata_update_get_thread_generation();
else
e->generation = 0;
mono_conc_g_hash_table_insert (mem_manager->refobject_hash, e, MONO_HANDLE_RAW (o));
MONO_HANDLE_ASSIGN (obj, o);
}
Expand All @@ -92,6 +107,10 @@ cache_object_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer
ReflectedEntry *e = alloc_reflected_entry (mem_manager);
e->item = item;
e->refclass = klass;
if (G_UNLIKELY(mono_metadata_has_updates()) && ((flags & MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE) == 0))
e->generation = mono_metadata_update_get_thread_generation();
else
e->generation = 0;
mono_weak_hash_table_insert (mem_manager->weak_refobject_hash, e, MONO_HANDLE_RAW (o));
MONO_HANDLE_ASSIGN (obj, o);
}
Expand All @@ -100,13 +119,14 @@ cache_object_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer
return obj;
}

#define CACHE_OBJECT(t,mem_manager,p,o,k) ((t) (cache_object ((mem_manager), (k), (p), (o))))
#define CACHE_OBJECT_HANDLE(t,mem_manager,p,o,k) (MONO_HANDLE_CAST (t, cache_object_handle ((mem_manager), (k), (p), (o))))
#define CACHE_OBJECT(t,mem_manager,flags,p,o,k) ((t) (cache_object ((mem_manager), (flags), (k), (p), (o))))
#define CACHE_OBJECT_HANDLE(t,mem_manager,flags,p,o,k) (MONO_HANDLE_CAST (t, cache_object_handle ((mem_manager), (flags), (k), (p), (o))))

static inline MonoObjectHandle
check_object_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer item)
check_object_handle (MonoMemoryManager *mem_manager, int flags, MonoClass *klass, gpointer item)
{
MonoObjectHandle obj_handle;
gpointer orig_e, orig_value;
ReflectedEntry e;
e.item = item;
e.refclass = klass;
Expand All @@ -123,6 +143,24 @@ check_object_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer
MonoWeakHashTable *hash = mem_manager->weak_refobject_hash;
obj_handle = MONO_HANDLE_NEW (MonoObject, (MonoObject *)mono_weak_hash_table_lookup (hash, &e));
}

if (!mem_manager->collectible) {
MonoConcGHashTable *hash = mem_manager->refobject_hash;
if (mono_conc_g_hash_table_lookup_extended (hash, &e, &orig_e, &orig_value))
if (mono_metadata_has_updates() && ((flags & MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE) == 0) && ((ReflectedEntry *)orig_e)->generation < mono_metadata_update_get_thread_generation()) {
mono_conc_g_hash_table_remove (hash, &e);
free_reflected_entry ((ReflectedEntry *)orig_e);
obj_handle = MONO_HANDLE_NEW (MonoObject, NULL);
} else {
obj_handle = MONO_HANDLE_NEW (MonoObject, (MonoObject *)orig_value);
}
else {
obj_handle = MONO_HANDLE_NEW (MonoObject, NULL);
}
} else {
MonoWeakHashTable *hash = mem_manager->weak_refobject_hash;
obj_handle = MONO_HANDLE_NEW (MonoObject, (MonoObject *)mono_weak_hash_table_lookup (hash, &e));
}
mono_mem_manager_unlock (mem_manager);

return obj_handle;
Expand All @@ -131,22 +169,22 @@ check_object_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer
typedef MonoObjectHandle (*ReflectionCacheConstructFunc_handle) (MonoClass*, gpointer, gpointer, MonoError *);

static inline MonoObjectHandle
check_or_construct_handle (MonoMemoryManager *mem_manager, MonoClass *klass, gpointer item, gpointer user_data, MonoError *error, ReflectionCacheConstructFunc_handle construct)
check_or_construct_handle (MonoMemoryManager *mem_manager, int flags, MonoClass *klass, gpointer item, gpointer user_data, MonoError *error, ReflectionCacheConstructFunc_handle construct)
{
error_init (error);
MonoObjectHandle obj = check_object_handle (mem_manager, klass, item);
MonoObjectHandle obj = check_object_handle (mem_manager, flags, klass, item);
if (!MONO_HANDLE_IS_NULL (obj))
return obj;
MONO_HANDLE_ASSIGN (obj, construct (klass, item, user_data, error));
return_val_if_nok (error, NULL_HANDLE);
if (MONO_HANDLE_IS_NULL (obj))
return obj;
/* note no caching if there was an error in construction */
return cache_object_handle (mem_manager, klass, item, obj);
return cache_object_handle (mem_manager, flags, klass, item, obj);
}

#define CHECK_OR_CONSTRUCT_HANDLE(type,mem_manager, item,klass,construct,user_data) \
#define CHECK_OR_CONSTRUCT_HANDLE(type,mem_manager,flags,item,klass,construct,user_data) \
(MONO_HANDLE_CAST (type, check_or_construct_handle ( \
(mem_manager), (klass), (item), (user_data), error, (ReflectionCacheConstructFunc_handle) (construct))))
(mem_manager), (flags), (klass), (item), (user_data), error, (ReflectionCacheConstructFunc_handle) (construct))))

#endif /*__MONO_METADATA_REFLECTION_CACHE_H__*/
16 changes: 8 additions & 8 deletions src/mono/mono/metadata/reflection.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ MonoReflectionAssemblyHandle
mono_assembly_get_object_handle (MonoAssembly *assembly, MonoError *error)
{
error_init (error);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionAssembly, m_image_get_mem_manager (assembly->image), assembly, NULL, assembly_object_construct, NULL);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionAssembly, m_image_get_mem_manager (assembly->image), MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE, assembly, NULL, assembly_object_construct, NULL);
}

/**
Expand Down Expand Up @@ -311,7 +311,7 @@ MonoReflectionModuleHandle
mono_module_get_object_handle (MonoImage *image, MonoError *error)
{
error_init (error);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionModule, m_image_get_mem_manager (image), image, NULL, module_object_construct, NULL);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionModule, m_image_get_mem_manager (image), MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE, image, NULL, module_object_construct, NULL);
}

/**
Expand Down Expand Up @@ -670,7 +670,7 @@ mono_method_get_object_handle (MonoMethod *method, MonoClass *refclass, MonoErro
refclass = method->klass;

// FIXME: For methods/params etc., use the mem manager for refclass or a merged one ?
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionMethod, m_method_get_mem_manager (method), method, refclass, method_object_construct, NULL);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionMethod, m_method_get_mem_manager (method), MONO_REFL_CACHE_DEFAULT, method, refclass, method_object_construct, NULL);
}
/*
* mono_method_get_object_checked:
Expand Down Expand Up @@ -776,7 +776,7 @@ MonoReflectionFieldHandle
mono_field_get_object_handle (MonoClass *klass, MonoClassField *field, MonoError *error)
{
error_init (error);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionField, m_class_get_mem_manager (m_field_get_parent (field)), field, klass, field_object_construct, NULL);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionField, m_class_get_mem_manager (m_field_get_parent (field)), MONO_REFL_CACHE_DEFAULT, field, klass, field_object_construct, NULL);
}

/*
Expand Down Expand Up @@ -844,7 +844,7 @@ property_object_construct (MonoClass *klass, MonoProperty *property, gpointer us
MonoReflectionPropertyHandle
mono_property_get_object_handle (MonoClass *klass, MonoProperty *property, MonoError *error)
{
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionProperty, m_class_get_mem_manager (property->parent), property, klass, property_object_construct, NULL);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionProperty, m_class_get_mem_manager (property->parent), MONO_REFL_CACHE_DEFAULT, property, klass, property_object_construct, NULL);
}

/**
Expand Down Expand Up @@ -909,7 +909,7 @@ MonoReflectionEventHandle
mono_event_get_object_handle (MonoClass *klass, MonoEvent *event, MonoError *error)
{
error_init (error);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionEvent, m_class_get_mem_manager (event->parent), event, klass, event_object_construct, NULL);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionEvent, m_class_get_mem_manager (event->parent), MONO_REFL_CACHE_DEFAULT, event, klass, event_object_construct, NULL);
}


Expand Down Expand Up @@ -1167,7 +1167,7 @@ mono_param_get_objects_internal (MonoMethod *method, MonoClass *refclass, MonoEr
/* Note: the cache is based on the address of the signature into the method
* since we already cache MethodInfos with the method as keys.
*/
return CHECK_OR_CONSTRUCT_HANDLE (MonoArray, m_method_get_mem_manager (method), &method->signature, refclass, param_objects_construct, method);
return CHECK_OR_CONSTRUCT_HANDLE (MonoArray, m_method_get_mem_manager (method), MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE, &method->signature, refclass, param_objects_construct, method);
fail:
return MONO_HANDLE_NEW (MonoArray, NULL);
}
Expand Down Expand Up @@ -1392,7 +1392,7 @@ MonoReflectionMethodBodyHandle
mono_method_body_get_object_handle (MonoMethod *method, MonoError *error)
{
error_init (error);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionMethodBody, m_method_get_mem_manager (method), method, NULL, method_body_object_construct, NULL);
return CHECK_OR_CONSTRUCT_HANDLE (MonoReflectionMethodBody, m_method_get_mem_manager (method), MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE, method, NULL, method_body_object_construct, NULL);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/metadata/sre.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1188,13 +1188,13 @@ mono_image_create_token (MonoDynamicImage *assembly, MonoObjectHandle obj,
static gpointer
register_assembly (MonoReflectionAssembly *res, MonoAssembly *assembly)
{
return CACHE_OBJECT (MonoReflectionAssembly *, mono_mem_manager_get_ambient (), assembly, &res->object, NULL);
return CACHE_OBJECT (MonoReflectionAssembly *, mono_mem_manager_get_ambient (), MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE, assembly, &res->object, NULL);
}

static MonoReflectionModuleBuilderHandle
register_module (MonoReflectionModuleBuilderHandle res, MonoDynamicImage *module)
{
return CACHE_OBJECT_HANDLE (MonoReflectionModuleBuilder, mono_mem_manager_get_ambient (), module, MONO_HANDLE_CAST (MonoObject, res), NULL);
return CACHE_OBJECT_HANDLE (MonoReflectionModuleBuilder, mono_mem_manager_get_ambient (), MONO_REFL_CACHE_NO_HOT_RELOAD_INVALIDATE, module, MONO_HANDLE_CAST (MonoObject, res), NULL);
}

/*
Expand Down
38 changes: 38 additions & 0 deletions src/mono/wasm/debugger/DebuggerTestSuite/HotReloadTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,5 +599,43 @@ await SendCommandAndCheck (JObject.FromObject(new { }), "Debugger.resume", scrip
}, "c", num_fields: 2);
});
}

// Enable this test when https://github.com/dotnet/hotreload-utils/pull/264 flows into dotnet/runtime repo
// [ConditionalFact(nameof(RunningOnChrome))]
// public async Task DebugHotReloadMethod_ChangeParameterName()
// {
// string asm_file = Path.Combine(DebuggerTestAppPath, "ApplyUpdateReferencedAssembly.dll");
// string pdb_file = Path.Combine(DebuggerTestAppPath, "ApplyUpdateReferencedAssembly.pdb");
// string asm_file_hot_reload = Path.Combine(DebuggerTestAppPath, "../wasm/ApplyUpdateReferencedAssembly.dll");

// var bp_notchanged = await SetBreakpoint(".*/MethodBody1.cs$", 89, 12, use_regex: true);
// // var bp_invalid = await SetBreakpoint(".*/MethodBody1.cs$", 59, 12, use_regex: true);

// var pause_location = await LoadAssemblyAndTestHotReloadUsingSDBWithoutChanges(
// asm_file, pdb_file, "MethodBody9", "test", expectBpResolvedEvent: true, sourcesToWait: new string [] { "MethodBody0.cs", "MethodBody1.cs" });

// CheckLocation("dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 89, 12, scripts, pause_location["callFrames"]?[0]["location"]);
// await StepAndCheck(StepKind.Over, "dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 90, 12, "ApplyUpdateReferencedAssembly.MethodBody9.M1",
// locals_fn: async (locals) =>
// {
// CheckNumber(locals, "a", 1);
// await Task.CompletedTask;
// }
// );
// //apply first update
// pause_location = await LoadAssemblyAndTestHotReloadUsingSDB(
// asm_file_hot_reload, "MethodBody9", "test", 1);

// JToken top_frame = pause_location["callFrames"]?[0];
// AssertEqual("ApplyUpdateReferencedAssembly.MethodBody9.M1", top_frame?["functionName"]?.Value<string>(), top_frame?.ToString());
// CheckLocation("dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 89, 12, scripts, top_frame["location"]);
// await StepAndCheck(StepKind.Over, "dotnet://ApplyUpdateReferencedAssembly.dll/MethodBody1.cs", 90, 12, "ApplyUpdateReferencedAssembly.MethodBody9.M1",
// locals_fn: async (locals) =>
// {
// CheckNumber(locals, "x", 1);
// await Task.CompletedTask;
// }
// );
// }
}
}
37 changes: 37 additions & 0 deletions src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,41 @@ public static void StaticMethod1 () {
Console.WriteLine("original");
}
}




























// public class MethodBody9 {
// public static int M1(int a, int b) {
// return a + b;
// }

// public static int test() {
// return M1(1, 2);
// }
// }
}
10 changes: 10 additions & 0 deletions src/mono/wasm/debugger/tests/ApplyUpdateReferencedAssembly/MethodBody1_v1.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,14 @@ public void InstanceMethod () {
Console.WriteLine($"add a breakpoint the instance method of the new class");
}
}

// public class MethodBody9 {
// public static int M1(int x, int y) {
// return x + y;
// }

// public static int test() {
// return M1(1, 2);
// }
// }
}

0 comments on commit cc7448c

Please sign in to comment.