From 207c2e4a28d2a1109296160d9465ca65ad15cc52 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Thu, 8 Jul 2021 13:38:47 -0400 Subject: [PATCH 1/7] [hot_reload] Add test for updating custom attribute ctor values Just changing the arguments of a custom attribute constructor should generate an update to the CustomAttributes table with the same parent and .ctor. That kind of change should be allowed by Mono and CoreCLR --- .../CustomAttributeUpdate.cs | 35 +++++++++++++++++++ .../CustomAttributeUpdate_v1.cs | 35 +++++++++++++++++++ ...lyUpdate.Test.CustomAttributeUpdate.csproj | 12 +++++++ .../deltascript.json | 6 ++++ .../tests/ApplyUpdateTest.cs | 16 +++++++++ .../tests/System.Runtime.Loader.Tests.csproj | 4 +++ 6 files changed, 108 insertions(+) create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate.cs create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate_v1.cs create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj create mode 100644 src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/deltascript.json diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate.cs new file mode 100644 index 00000000000000..f1590848bea591 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate.cs @@ -0,0 +1,35 @@ +// 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 +{ + [AttributeUsage (AttributeTargets.Method, AllowMultiple=true)] + public class MyAttribute : Attribute + { + public MyAttribute (string stringValue) { StringValue = stringValue; } + + public MyAttribute (Type typeValue) { TypeValue = typeValue; } + + public MyAttribute (int x) { IntValue = x; } + + public string StringValue { get; set; } + public Type TypeValue {get; set; } + public int IntValue {get; set; } + } + + public class ClassWithCustomAttributeUpdates + { + [MyAttribute ("abcd")] + public static string Method1 () => null; + + [MyAttribute (typeof(Exception))] + public static string Method2 () => null; + + [MyAttribute (42, StringValue = "hijkl", TypeValue = typeof(Type))] + [MyAttribute (17, StringValue = "", TypeValue = typeof(object))] + public static string Method3 () => null; + + } +} diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate_v1.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate_v1.cs new file mode 100644 index 00000000000000..516914d76d8e15 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/CustomAttributeUpdate_v1.cs @@ -0,0 +1,35 @@ +// 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 +{ + [AttributeUsage (AttributeTargets.Method, AllowMultiple=true)] + public class MyAttribute : Attribute + { + public MyAttribute (string stringValue) { StringValue = stringValue; } + + public MyAttribute (Type typeValue) { TypeValue = typeValue; } + + public MyAttribute (int x) { IntValue = x; } + + public string StringValue { get; set; } + public Type TypeValue {get; set; } + public int IntValue {get; set; } + } + + public class ClassWithCustomAttributeUpdates + { + [MyAttribute ("rstuv")] + public static string Method1 () => null; + + [MyAttribute (typeof(ArgumentException))] + public static string Method2 () => null; + + [MyAttribute (2042, StringValue = "qwerty", TypeValue = typeof(byte[]))] + [MyAttribute (17, StringValue = "", TypeValue = typeof(object))] + public static string Method3 () => null; + + } +} diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj new file mode 100644 index 00000000000000..fbfcc8e6267040 --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj @@ -0,0 +1,12 @@ + + + System.Runtime.Loader.Tests + $(NetCoreAppCurrent) + true + deltascript.json + disable + + + + + diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/deltascript.json b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/deltascript.json new file mode 100644 index 00000000000000..3dcb95912dbe5e --- /dev/null +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/deltascript.json @@ -0,0 +1,6 @@ +{ + "changes": [ + {"document": "CustomAttributeUpdate.cs", "update": "CustomAttributeUpdate_v1.cs"}, + ] +} + diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs index c2f06b86b93866..1ea996e2588275 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs @@ -83,6 +83,22 @@ void ClassWithCustomAttributes() }); } + [ConditionalFact(typeof(ApplyUpdateUtil), nameof (ApplyUpdateUtil.IsSupported))] + public void CustomAttributeUpdates() + { + // Test that _modifying_ custom attribute constructor/property argumments works as expected. + // For this test, we don't change which constructor is called, or how many custom attributes there are. + ApplyUpdateUtil.TestCase(static () => + { + var assm = typeof(System.Reflection.Metadata.ApplyUpdate.Test.ClassWithCustomAttributeUpdates).Assembly; + + ApplyUpdateUtil.ApplyUpdate(assm); + ApplyUpdateUtil.ClearAllReflectionCaches(); + + Assert.True(true); // just check that the update loaded + }); + } + class NonRuntimeAssembly : Assembly { } 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 index eeae81e75cc682..a5f2b3b0cce7bc 100644 --- a/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj +++ b/src/libraries/System.Runtime.Loader/tests/System.Runtime.Loader.Tests.csproj @@ -39,6 +39,7 @@ + @@ -58,6 +59,9 @@ + From 4a643f17d91ba89d1c79cab2be240389342f8eb0 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 9 Jul 2021 15:49:40 -0400 Subject: [PATCH 2/7] [hot_reload] Allow custom attribute row modifications if Parent and Type unchanged. Allows updates to the constructor arguments (or property values) --- src/mono/mono/component/hot_reload.c | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/mono/mono/component/hot_reload.c b/src/mono/mono/component/hot_reload.c index 0997e8f859c232..f6665ef0035750 100644 --- a/src/mono/mono/component/hot_reload.c +++ b/src/mono/mono/component/hot_reload.c @@ -1041,6 +1041,7 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, gconstpointer continue; } case MONO_TABLE_METHODSEMANTICS: { + /* FIXME: this should get the current table size, not the base stable size */ if (token_index > table_info_get_rows (&image_base->tables [token_table])) { /* new rows are fine, as long as they point at existing methods */ guint32 sema_cols [MONO_METHOD_SEMA_SIZE]; @@ -1073,6 +1074,35 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, gconstpointer continue; } } + case MONO_TABLE_CUSTOMATTRIBUTE: { + /* FIXME: this should get the current table size, not the base stable size */ + if (token_index <= table_info_get_rows (&image_base->tables [token_table])) { + /* modifying existing rows is ok, as long as the parent and ctor are the same */ + guint32 ca_upd_cols [MONO_CUSTOM_ATTR_SIZE]; + guint32 ca_base_cols [MONO_CUSTOM_ATTR_SIZE]; + int mapped_token = hot_reload_relative_delta_index (image_dmeta, 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 CUSTOM_ATTR update. mapped index = 0x%08x\n", i, log_token, mapped_token); + + mono_metadata_decode_row (&image_dmeta->tables [MONO_TABLE_CUSTOMATTRIBUTE], mapped_token - 1, ca_upd_cols, MONO_CUSTOM_ATTR_SIZE); + mono_metadata_decode_row (&image_base->tables [MONO_TABLE_CUSTOMATTRIBUTE], token_index - 1, ca_base_cols, MONO_CUSTOM_ATTR_SIZE); + + /* compare the ca_upd_cols [MONO_CUSTOM_ATTR_PARENT] to ca_base_cols [MONO_CUSTOM_ATTR_PARENT]. */ + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x CUSTOM_ATTR update. Old Parent 0x%08x New Parent 0x%08x\n", i, log_token, ca_base_cols [MONO_CUSTOM_ATTR_PARENT], ca_upd_cols [MONO_CUSTOM_ATTR_PARENT]); + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x CUSTOM_ATTR update. Old ctor 0x%08x New ctor 0x%08x\n", i, log_token, ca_base_cols [MONO_CUSTOM_ATTR_TYPE], ca_upd_cols [MONO_CUSTOM_ATTR_TYPE]); + + if (ca_base_cols [MONO_CUSTOM_ATTR_PARENT] != ca_upd_cols [MONO_CUSTOM_ATTR_PARENT] || + ca_base_cols [MONO_CUSTOM_ATTR_TYPE] != ca_upd_cols [MONO_CUSTOM_ATTR_TYPE]) { + mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing CA table cols with a different Parent or Type. token=0x%08x", log_token); + unsupported_edits = TRUE; + } + } else { + mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token); + mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token); + unsupported_edits = TRUE; + } + continue; + } default: /* FIXME: this bounds check is wrong for cumulative updates - need to look at the DeltaInfo:count.prev_gen_rows */ if (token_index <= table_info_get_rows (&image_base->tables [token_table])) { @@ -1211,6 +1241,10 @@ apply_enclog_pass2 (MonoImage *image_base, BaselineInfo *base_info, uint32_t gen /* assuming that property attributes and type haven't changed. */ break; } + case MONO_TABLE_CUSTOMATTRIBUTE: { + /* ok, pass1 checked for disallowed modifications */ + break; + } default: { g_assert (token_index > table_info_get_rows (&image_base->tables [token_table])); if (mono_trace_is_traced (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE)) From 8a437bd050568c973895f6c010725f1003f4e7d3 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 9 Jul 2021 22:18:15 -0400 Subject: [PATCH 3/7] [hot_reload] Implement table lookup of modified rows Add a bool array on the base image to keep track of whether each table had any row modifications (as opposed to row additions) in any generation. If there was a modification, take the slow path in mono_image_effective_table even if the index we're looking at is in the base image. Update hot_reload_effective_table_slow so that if there was a modified row, we look at all the deltas to see if there's an even later update to that row. (When there are only additions, keep same behavior as before - only look as far as the generation that added the row we wanted to find). Also refine the assertion in hot_reload_relative_delta_index to properly account for EnCMap entries that correspond to modifications - in that case we might stop searching a metadata delta before we hit the end of the table if the EnCmap entries start pointing to rows that are past the one we wanted to look up. --- src/mono/mono/component/hot_reload-stub.c | 9 ++ src/mono/mono/component/hot_reload.c | 100 ++++++++++++++++---- src/mono/mono/component/hot_reload.h | 2 + src/mono/mono/metadata/metadata-internals.h | 5 +- src/mono/mono/metadata/metadata-update.c | 5 + 5 files changed, 102 insertions(+), 19 deletions(-) diff --git a/src/mono/mono/component/hot_reload-stub.c b/src/mono/mono/component/hot_reload-stub.c index 718ce3e453a10b..48033c1c7a0cfc 100644 --- a/src/mono/mono/component/hot_reload-stub.c +++ b/src/mono/mono/component/hot_reload-stub.c @@ -59,6 +59,9 @@ hot_reload_stub_table_bounds_check (MonoImage *base_image, int table_index, int static gboolean hot_reload_stub_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc get_heap, uint32_t orig_index, MonoImage **image_out, uint32_t *index_out); +static gboolean +hot_reload_stub_has_modified_rows (const MonoTableInfo *table); + static MonoComponentHotReload fn_table = { { MONO_COMPONENT_ITF_VERSION, &hot_reload_stub_available }, &hot_reload_stub_set_fastpath_data, @@ -75,6 +78,7 @@ static MonoComponentHotReload fn_table = { &hot_reload_stub_get_updated_method_rva, &hot_reload_stub_table_bounds_check, &hot_reload_stub_delta_heap_lookup, + &hot_reload_stub_has_modified_rows, }; static bool @@ -172,6 +176,11 @@ hot_reload_stub_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc g_assert_not_reached (); } +static gboolean +hot_reload_stub_has_modified_rows (const MonoTableInfo *table){ + return FALSE; +} + 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 index f6665ef0035750..a1f692f090683a 100644 --- a/src/mono/mono/component/hot_reload.c +++ b/src/mono/mono/component/hot_reload.c @@ -73,6 +73,9 @@ hot_reload_table_bounds_check (MonoImage *base_image, int table_index, int token static gboolean hot_reload_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc get_heap, uint32_t orig_index, MonoImage **image_out, uint32_t *index_out); +static gboolean +hot_reload_has_modified_rows (const MonoTableInfo *table); + static MonoComponentHotReload fn_table = { { MONO_COMPONENT_ITF_VERSION, &hot_reload_available }, &hot_reload_set_fastpath_data, @@ -89,6 +92,7 @@ static MonoComponentHotReload fn_table = { &hot_reload_get_updated_method_rva, &hot_reload_table_bounds_check, &hot_reload_delta_heap_lookup, + &hot_reload_has_modified_rows, }; MonoComponentHotReload * @@ -161,6 +165,9 @@ typedef struct _BaselineInfo { /* Maps MethodDef token indices to a boolean flag that there's an update for the method */ GHashTable *method_table_update; + + /* TRUE if any published update modified an existing row */ + gboolean any_modified_rows [MONO_TABLE_NUM]; } BaselineInfo; #define DOTNET_MODIFIABLE_ASSEMBLIES "DOTNET_MODIFIABLE_ASSEMBLIES" @@ -724,9 +731,6 @@ dump_update_summary (MonoImage *image_base, MonoImage *image_dmeta) void hot_reload_effective_table_slow (const MonoTableInfo **t, int *idx) { - if (G_LIKELY (*idx < table_info_get_rows (*t))) - return; - /* FIXME: don't let any thread other than the updater thread see values from a delta image * with a generation past update_published */ @@ -738,17 +742,22 @@ hot_reload_effective_table_slow (const MonoTableInfo **t, int *idx) if (!info) return; - GList *list = info->delta_image; - MonoImage *dmeta; - int ridx; - MonoTableInfo *table; - /* Invariant: `*t` must be a `MonoTableInfo` of the base image. */ g_assert (base->tables < *t && *t < &base->tables [MONO_TABLE_LAST]); size_t s = ALIGN_TO (sizeof (MonoTableInfo), sizeof (gpointer)); int tbl_index = ((intptr_t) *t - (intptr_t) base->tables) / s; + if (G_LIKELY (*idx < table_info_get_rows (*t) && !info->any_modified_rows[tbl_index])) + return; + + gboolean any_modified = info->any_modified_rows[tbl_index]; + + GList *list = info->delta_image; + MonoImage *dmeta; + int ridx; + MonoTableInfo *table; + /* FIXME: I don't understand how ReplaceMethodOften works - it always has a * EnCMap entry 2: 0x06000002 (MethodDef) for every revision. Shouldn't the number of methodDef rows be going up? @@ -772,19 +781,43 @@ hot_reload_effective_table_slow (const MonoTableInfo **t, int *idx) */ int g = 0; + /* Candidate: the last delta that had updates for the requested row */ + MonoImage *cand_dmeta = NULL; + MonoTableInfo *cand_table = NULL; + int cand_ridx = -1; + int cand_g = 0; + + gboolean cont; do { g_assertf (list, "couldn't find idx=0x%08x in assembly=%s", *idx, dmeta && dmeta->name ? dmeta->name : "unknown image"); dmeta = (MonoImage*)list->data; list = list->next; table = &dmeta->tables [tbl_index]; - ridx = hot_reload_relative_delta_index (dmeta, mono_metadata_make_token (tbl_index, *idx + 1)) - 1; + int rel_row = hot_reload_relative_delta_index (dmeta, mono_metadata_make_token (tbl_index, *idx + 1)); + g_assert (rel_row == -1 || (rel_row > 0 && rel_row <= table_info_get_rows (table))); g++; - } while (ridx < 0 || ridx >= table_info_get_rows (table)); + if (rel_row != -1) { + cand_dmeta = dmeta; + cand_table = table; + cand_ridx = rel_row - 1; + cand_g = g; + } + ridx = rel_row - 1; + if (!any_modified) { + /* if the table only got additions, not modifications, don't continue after we find the first image that has the right number of rows */ + cont = ridx < 0 || ridx >= table_info_get_rows (table); + } else { + /* otherwise, keep going in case a later generation modified the row again */ + cont = list != NULL; + } + } while (cont); - mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "effective table for %s: 0x%08x -> 0x%08x (rows = 0x%08x) (gen %d, g %d)", mono_meta_table_name (tbl_index), *idx, ridx, table_info_get_rows (table), metadata_update_local_generation (base, info, dmeta), g); + if (cand_ridx != -1) { + mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "effective table for %s: 0x%08x -> 0x%08x (rows = 0x%08x) (gen %d, g %d)", mono_meta_table_name (tbl_index), *idx, cand_ridx, table_info_get_rows (cand_table), metadata_update_local_generation (base, info, cand_dmeta), cand_g); - *t = table; - *idx = ridx; + *t = cand_table; + *idx = cand_ridx; + } } /* @@ -835,6 +868,11 @@ hot_reload_relative_delta_index (MonoImage *image_dmeta, int token) mono_metadata_decode_row (encmap, index_map - 1, cols, MONO_ENCMAP_SIZE); int map_entry = cols [MONO_ENCMAP_TOKEN]; + /* we're looking at the beginning of a sequence of encmap rows that are all the + * modifications+additions for the table we are looking for (or we're looking at an entry + * for the next table after the one we wanted). the map entries will have tokens in + * increasing order. skip over the rows where the tokens are not the one we want, until we + * hit the rows for the next table or we hit the end of the encmap */ while (mono_metadata_token_table (map_entry) == table && mono_metadata_token_index (map_entry) < index && index_map < encmap_rows) { mono_metadata_decode_row (encmap, ++index_map - 1, cols, MONO_ENCMAP_SIZE); map_entry = cols [MONO_ENCMAP_TOKEN]; @@ -848,12 +886,18 @@ hot_reload_relative_delta_index (MonoImage *image_dmeta, int token) mono_trace (G_LOG_LEVEL_DEBUG, MONO_TRACE_METADATA_UPDATE, "relative index for token 0x%08x -> table 0x%02x row 0x%08x", token, table, return_val); return return_val; } else { - /* otherwise the last entry in the encmap is for this table, but is still less than the index - the index is in the next generation */ - g_assert (mono_metadata_token_index (map_entry) < index && index_map == encmap_rows); + /* Otherwise we stopped either: because we saw an an entry for a row after + * the one we wanted - we were looking for a modification, but the encmap + * has an addition; or, because we saw the last entry in the encmap and it + * still wasn't for a row as high as the one we wanted. either way, the + * update we want is not in the delta we're looking at. + */ + g_assert ((mono_metadata_token_index (map_entry) > index) || (mono_metadata_token_index (map_entry) < index && index_map == encmap_rows)); return -1; } } else { - /* otherwise there are no more encmap entries for this table, and we didn't see the index, so there index is in the next generation */ + /* otherwise there are no more encmap entries for this table, and we didn't see the + * index, so there was no modification/addition for that index in this delta. */ g_assert (mono_metadata_token_table (map_entry) > table); return -1; } @@ -925,9 +969,10 @@ delta_info_compute_table_records (MonoImage *image_dmeta, MonoImage *image_base, g_assert (table != MONO_TABLE_ENCMAP); g_assert (table >= prev_table); /* FIXME: check bounds - is it < or <=. */ - if (rid < delta_info->count[table].prev_gen_rows) + if (rid < delta_info->count[table].prev_gen_rows) { + base_info->any_modified_rows[table] = TRUE; delta_info->count[table].modified_rows++; - else + } else delta_info->count[table].inserted_rows++; if (table == prev_table) continue; @@ -1512,3 +1557,22 @@ hot_reload_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc get_ return (cur != NULL); } +static gboolean +hot_reload_has_modified_rows (const MonoTableInfo *table) +{ + MonoImage *base = table_info_get_base_image (table); + if (!base) + return FALSE; + BaselineInfo *info = baseline_info_lookup (base); + if (!info) + return FALSE; + + /* Invariant: `*t` must be a `MonoTableInfo` of the base image. */ + g_assert (base->tables < table && table < &base->tables [MONO_TABLE_LAST]); + + size_t s = ALIGN_TO (sizeof (MonoTableInfo), sizeof (gpointer)); + int tbl_index = ((intptr_t) table - (intptr_t) base->tables) / s; + + return info->any_modified_rows[tbl_index]; +} + diff --git a/src/mono/mono/component/hot_reload.h b/src/mono/mono/component/hot_reload.h index 4994664103cc2e..0bdb63d121d01e 100644 --- a/src/mono/mono/component/hot_reload.h +++ b/src/mono/mono/component/hot_reload.h @@ -29,6 +29,8 @@ typedef struct _MonoComponentHotReload { gpointer (*get_updated_method_rva) (MonoImage *base_image, uint32_t idx); gboolean (*table_bounds_check) (MonoImage *base_image, int table_index, int token_index); gboolean (*delta_heap_lookup) (MonoImage *base_image, MetadataHeapGetterFunc get_heap, uint32_t orig_index, MonoImage **image_out, uint32_t *index_out); + gboolean (*has_modified_rows) (const MonoTableInfo *table); + } MonoComponentHotReload; MONO_COMPONENT_EXPORT_ENTRYPOINT diff --git a/src/mono/mono/metadata/metadata-internals.h b/src/mono/mono/metadata/metadata-internals.h index 545693ef12a584..20f36c60e8f5da 100644 --- a/src/mono/mono/metadata/metadata-internals.h +++ b/src/mono/mono/metadata/metadata-internals.h @@ -805,11 +805,14 @@ mono_metadata_has_updates (void) void mono_image_effective_table_slow (const MonoTableInfo **t, int *idx); +gboolean +mono_metadata_update_has_modified_rows (const MonoTableInfo *t); + static inline void mono_image_effective_table (const MonoTableInfo **t, int *idx) { if (G_UNLIKELY (mono_metadata_has_updates ())) { - if (G_UNLIKELY (*idx >= table_info_get_rows ((*t)))) { + if (G_UNLIKELY (*idx >= table_info_get_rows ((*t)) || mono_metadata_update_has_modified_rows (*t))) { mono_image_effective_table_slow (t, idx); } } diff --git a/src/mono/mono/metadata/metadata-update.c b/src/mono/mono/metadata/metadata-update.c index d1d646c49ba25b..8787b28bca658c 100644 --- a/src/mono/mono/metadata/metadata-update.c +++ b/src/mono/mono/metadata/metadata-update.c @@ -121,3 +121,8 @@ mono_metadata_update_delta_heap_lookup (MonoImage *base_image, MetadataHeapGette } +gboolean +mono_metadata_update_has_modified_rows (const MonoTableInfo *table) +{ + return mono_component_hot_reload ()->has_modified_rows (table); +} From e2dc8fbc7acbbd70fe92ab25d675ea2b104e36d1 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 9 Jul 2021 22:24:37 -0400 Subject: [PATCH 4/7] Update the CustomAttributeUpdates test to check attribute value Check that we get the updated custom attribute string property value. --- .../tests/ApplyUpdateTest.cs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs index 1ea996e2588275..6976f8a6f77729 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs @@ -95,7 +95,20 @@ public void CustomAttributeUpdates() ApplyUpdateUtil.ApplyUpdate(assm); ApplyUpdateUtil.ClearAllReflectionCaches(); - Assert.True(true); // just check that the update loaded + // Just check the updated value on one method + + Type attrType = typeof(System.Reflection.Metadata.ApplyUpdate.Test.MyAttribute); + Type ty = assm.GetType("System.Reflection.Metadata.ApplyUpdate.Test.ClassWithCustomAttributeUpdates"); + Assert.NotNull(ty); + MethodInfo mi = ty.GetMethod(nameof(System.Reflection.Metadata.ApplyUpdate.Test.ClassWithCustomAttributeUpdates.Method1), BindingFlags.Public | BindingFlags.Static); + Assert.NotNull(mi); + var cattrs = Attribute.GetCustomAttributes(mi, attrType); + Assert.NotNull(cattrs); + Assert.Equal(1, cattrs.Length); + Assert.NotNull(cattrs[0]); + Assert.Equal(attrType, cattrs[0].GetType()); + string p = (cattrs[0] as System.Reflection.Metadata.ApplyUpdate.Test.MyAttribute).StringValue; + Assert.Equal("rstuv", p); }); } From 01f6e69f9c6b88e0355b4d5b1d4635188ba145e7 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 9 Jul 2021 22:29:05 -0400 Subject: [PATCH 5/7] Re-enable nullability for hot reload tests Mono can now deal with the custom attributes modifications that Roslyn emits --- ...ection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj | 1 - ...ystem.Reflection.Metadata.ApplyUpdate.Test.MethodBody1.csproj | 1 - .../ApplyUpdateReferencedAssembly.csproj | 1 - 3 files changed, 3 deletions(-) diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj index fbfcc8e6267040..985424ab348619 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeUpdate.csproj @@ -4,7 +4,6 @@ $(NetCoreAppCurrent) true deltascript.json - disable diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1.csproj b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1.csproj index 87a4a28d000193..57ba4f3ec52983 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1.csproj +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1/System.Reflection.Metadata.ApplyUpdate.Test.MethodBody1.csproj @@ -5,7 +5,6 @@ true deltascript.json true - disable diff --git a/src/tests/FunctionalTests/WebAssembly/Browser/HotReload/ApplyUpdateReferencedAssembly/ApplyUpdateReferencedAssembly.csproj b/src/tests/FunctionalTests/WebAssembly/Browser/HotReload/ApplyUpdateReferencedAssembly/ApplyUpdateReferencedAssembly.csproj index 06190d7b86fc09..46e260f24e155f 100644 --- a/src/tests/FunctionalTests/WebAssembly/Browser/HotReload/ApplyUpdateReferencedAssembly/ApplyUpdateReferencedAssembly.csproj +++ b/src/tests/FunctionalTests/WebAssembly/Browser/HotReload/ApplyUpdateReferencedAssembly/ApplyUpdateReferencedAssembly.csproj @@ -11,7 +11,6 @@ false false - disable From 3bbf5108ecac360f1d99fa71fcca4821d29ef625 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 9 Jul 2021 22:44:04 -0400 Subject: [PATCH 6/7] fixup copy/paste mistake --- src/mono/mono/component/hot_reload.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/mono/mono/component/hot_reload.c b/src/mono/mono/component/hot_reload.c index a1f692f090683a..fd983584ad411c 100644 --- a/src/mono/mono/component/hot_reload.c +++ b/src/mono/mono/component/hot_reload.c @@ -1140,13 +1140,13 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, gconstpointer ca_base_cols [MONO_CUSTOM_ATTR_TYPE] != ca_upd_cols [MONO_CUSTOM_ATTR_TYPE]) { mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing CA table cols with a different Parent or Type. token=0x%08x", log_token); unsupported_edits = TRUE; + continue; } + break; } else { - mono_trace (G_LOG_LEVEL_INFO, MONO_TRACE_METADATA_UPDATE, "row[0x%02x]:0x%08x we do not support patching of existing table cols.", i, log_token); - mono_error_set_type_load_name (error, NULL, image_base->name, "EnC: we do not support patching of existing table cols. token=0x%08x", log_token); - unsupported_edits = TRUE; + /* Added a row. ok */ + break; } - continue; } default: /* FIXME: this bounds check is wrong for cumulative updates - need to look at the DeltaInfo:count.prev_gen_rows */ From 928ac23cd3c3e602072ce878804212edf0f6c7a0 Mon Sep 17 00:00:00 2001 From: Aleksey Kliger Date: Fri, 9 Jul 2021 23:05:43 -0400 Subject: [PATCH 7/7] Remove duplicated code; outdated comments --- src/mono/mono/component/hot_reload.c | 73 ++++++++++++---------------- 1 file changed, 31 insertions(+), 42 deletions(-) diff --git a/src/mono/mono/component/hot_reload.c b/src/mono/mono/component/hot_reload.c index fd983584ad411c..76861056eda155 100644 --- a/src/mono/mono/component/hot_reload.c +++ b/src/mono/mono/component/hot_reload.c @@ -426,6 +426,28 @@ table_info_get_base_image (const MonoTableInfo *t) return image; } +/* Given a table, find the base image that it came from and its table index */ +static gboolean +table_info_find_in_base (const MonoTableInfo *table, MonoImage **base_out, int *tbl_index) +{ + g_assert (base_out); + *base_out = NULL; + MonoImage *base = table_info_get_base_image (table); + if (!base) + return FALSE; + + *base_out = base; + + /* Invariant: `table` must be a `MonoTableInfo` of the base image. */ + g_assert (base->tables < table && table < &base->tables [MONO_TABLE_LAST]); + + if (tbl_index) { + size_t s = ALIGN_TO (sizeof (MonoTableInfo), sizeof (gpointer)); + *tbl_index = ((intptr_t) table - (intptr_t) base->tables) / s; + } + return TRUE; +} + static MonoImage* image_open_dmeta_from_data (MonoImage *base_image, uint32_t generation, gconstpointer dmeta_bytes, uint32_t dmeta_length); @@ -735,50 +757,23 @@ hot_reload_effective_table_slow (const MonoTableInfo **t, int *idx) * with a generation past update_published */ - MonoImage *base = table_info_get_base_image (*t); - if (!base) + MonoImage *base; + int tbl_index; + if (!table_info_find_in_base (*t, &base, &tbl_index)) return; BaselineInfo *info = baseline_info_lookup (base); if (!info) return; - /* Invariant: `*t` must be a `MonoTableInfo` of the base image. */ - g_assert (base->tables < *t && *t < &base->tables [MONO_TABLE_LAST]); - - size_t s = ALIGN_TO (sizeof (MonoTableInfo), sizeof (gpointer)); - int tbl_index = ((intptr_t) *t - (intptr_t) base->tables) / s; + gboolean any_modified = info->any_modified_rows[tbl_index]; - if (G_LIKELY (*idx < table_info_get_rows (*t) && !info->any_modified_rows[tbl_index])) + if (G_LIKELY (*idx < table_info_get_rows (*t) && !any_modified)) return; - gboolean any_modified = info->any_modified_rows[tbl_index]; - GList *list = info->delta_image; MonoImage *dmeta; int ridx; MonoTableInfo *table; - - /* FIXME: I don't understand how ReplaceMethodOften works - it always has a - * EnCMap entry 2: 0x06000002 (MethodDef) for every revision. Shouldn't the number of methodDef rows be going up? - - * Apparently not - because conceptually the EnC log is saying to overwrite the existing rows. - */ - - /* FIXME: so if the tables are conceptually mutated by each delta, we can't just stop at the - * first lookup that gets a relative index in the right range, can we? that will always be - * the oldest delta. - */ - - /* FIXME: the other problem is that the EnClog is a sequence of actions to MUTATE rows. So when looking up an existing row we have to be able to make it so that naive callers decoding that row see the updated data. - * - * That's the main thing that PAss1 should eb doing for us. - * - * I think we can't get away from mutating. The format is just too geared toward it. - * - * We should make the mutations atomic, though. (And I guess the heap extension is probably unavoidable) - * - * 1. Keep a table of inv - */ int g = 0; /* Candidate: the last delta that had updates for the requested row */ @@ -1560,19 +1555,13 @@ hot_reload_delta_heap_lookup (MonoImage *base_image, MetadataHeapGetterFunc get_ static gboolean hot_reload_has_modified_rows (const MonoTableInfo *table) { - MonoImage *base = table_info_get_base_image (table); - if (!base) - return FALSE; + MonoImage *base; + int tbl_index; + if (!table_info_find_in_base (table, &base, &tbl_index)) + return FALSE; BaselineInfo *info = baseline_info_lookup (base); if (!info) return FALSE; - - /* Invariant: `*t` must be a `MonoTableInfo` of the base image. */ - g_assert (base->tables < table && table < &base->tables [MONO_TABLE_LAST]); - - size_t s = ALIGN_TO (sizeof (MonoTableInfo), sizeof (gpointer)); - int tbl_index = ((intptr_t) table - (intptr_t) base->tables) / s; - return info->any_modified_rows[tbl_index]; }