[hot_reload] assorted hot reload fixes for cumulative property updates (

#85351) * [hot_reload] remove unneeded assertion `is_addition` is already defined as `token_index-1 >= delta_info->count[token_table].prev_gen_rows` so: 1. the assertion as written is wrong for cumulative updates that add new properties or events 2. the assertion would be trivially true if it was updated to use `delta_info->count[token_table].prev_gen_rows`. Fixes the ability to cumulatively add new properties * Update generation on MonoClassMetadataUpdateInfo when adding members Give recompute_ginst_update_info something to work with. * assert that generic instances don't call hot_reload_get_property token-based lookup should only happen for class defs or GTDs * update regression test
dotnet · Apr 25, 2023 · 0b30b3b · 0b30b3b
1 parent 0c17737
commit 0b30b3b
Show file tree

Hide file tree

Showing 5 changed files with 112 additions and 10 deletions.
diff --git a/...pdate/System.Reflection.Metadata.ApplyUpdate.Test.AddInstanceField/AddInstanceField_v1.cs b/...pdate/System.Reflection.Metadata.ApplyUpdate.Test.AddInstanceField/AddInstanceField_v1.cs
@@ -85,5 +85,6 @@ public double FireEvents() {
             return Accumulator;
         }
 
+        public double AddedFirstProp {get => 0.0; set { Console.WriteLine (value); } }
     }
 }
diff --git a/...pdate/System.Reflection.Metadata.ApplyUpdate.Test.AddInstanceField/AddInstanceField_v2.cs b/...pdate/System.Reflection.Metadata.ApplyUpdate.Test.AddInstanceField/AddInstanceField_v2.cs
@@ -0,0 +1,91 @@
+// 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 AddInstanceField
+    {
+        public AddInstanceField () {
+            _doubleField2 = 5.5;
+            _stringField2 = "New Initial Value";
+            NewStructField = new NewStruct {
+                D = -1985.0,
+                O = new int[2] { 15, 17 },
+            };
+            // a little bit ldflda testing
+            IncRefDouble (ref NewStructField.D);
+            IncRefDouble (ref _doubleField2);
+
+            AddedStringAutoProp = "abcd";
+
+            AddedEvent += MyHandler;
+
+            void MyHandler (object sender, double data) {
+            }
+        }
+
+        public void IncRefDouble (ref double d)
+        {
+            d += 1.0;
+        }
+
+        public string GetStringField => _stringField2;
+        public double GetDoubleField => _doubleField2;
+
+        private string _stringField;
+        private string _stringField2;
+        private double _doubleField;
+        private double _doubleField2;
+
+        private int[] _intArrayFieldWithInit = new[] { 2, 4, 6, 8, 10, 12 };
+        private int[] _intArrayFieldWithInit2 = new[] { 1, 3, 5, 7, 9, 11 };
+
+        public void TestMethod () {
+            _stringField = "spqr";
+            _stringField2 = "4567";
+            _doubleField = 2.71828;
+            _doubleField2 = 0.707106;
+            AddedStringAutoProp = AddedStringAutoProp + "Test";
+        }
+
+        public int GetIntArrayLength() => _intArrayFieldWithInit2?.Length ?? -1;
+        public int GetIntArrayElt(int i) => _intArrayFieldWithInit2[i];
+
+        public struct NewStruct
+        {
+            public double D;
+            public object O;
+        }
+
+        public NewStruct NewStructField;
+
+        public string GetStringProp => AddedStringAutoProp;
+
+        public string AddedStringAutoProp { get; set; }
+
+        public event EventHandler<double> ExistingEvent;
+        public event EventHandler<double> AddedEvent;
+
+        public double Accumulator;
+
+        private void AccumHandler (object sender, double value) => Accumulator += value;
+
+        public double FireEvents() {
+            Accumulator = 0.0;
+            ExistingEvent += AccumHandler;
+            ExistingEvent(this, 123.0);
+            ExistingEvent -= AccumHandler;
+
+            AddedEvent += AccumHandler;
+            AddedEvent(this, 123.0);
+            AddedEvent -= AccumHandler;
+
+            return Accumulator;
+        }
+
+        public double AddedFirstProp {get => 0.0; set { Console.WriteLine (value+value); } }
+        public short AddedSecondProp {get; set; }
+    }
+}
diff --git a/...ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddInstanceField/deltascript.json b/...ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.AddInstanceField/deltascript.json
@@ -1,6 +1,7 @@
 {
     "changes": [
         {"document": "AddInstanceField.cs", "update": "AddInstanceField_v1.cs"},
+        {"document": "AddInstanceField.cs", "update": "AddInstanceField_v2.cs"},
     ]
 }
 
diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs
@@ -409,6 +409,16 @@ public static void TestAddInstanceField()
                 Assert.True ((addedEventToken & 0x00ffffff) < 4);
                 
 
+                ApplyUpdateUtil.ApplyUpdate(assm);
+
+                var addedFirstPropInfo = x2.GetType().GetProperty("AddedFirstProp");
+                var firstPropSetter = addedFirstPropInfo.GetSetMethod();
+                Assert.NotNull (firstPropSetter);
+
+                var addedSecondPropInfo = x2.GetType().GetProperty("AddedSecondProp");
+                var secondPropGetter = addedSecondPropInfo.GetGetMethod();
+                Assert.NotNull (secondPropGetter);
+
             });
         }
 

diff --git a/src/mono/mono/component/hot_reload.c b/src/mono/mono/component/hot_reload.c
@@ -2305,10 +2305,7 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 		}
 		case MONO_TABLE_PROPERTY: {
 			/* allow updates to existing properties. */
-			if (!is_addition)
-				/* FIXME: use DeltaInfo:prev_gen_rows instead of image_base */
-				g_assert (token_index <= table_info_get_rows (&image_base->tables [token_table]));
-			else {
+			if (is_addition) {
 				g_assert (add_property_propertymap != 0);
 
 				uint32_t parent_type_token = mono_metadata_decode_row_col (&image_base->tables [MONO_TABLE_PROPERTYMAP], mono_metadata_token_index (add_property_propertymap) - 1, MONO_PROPERTY_MAP_PARENT);
@@ -2362,10 +2359,7 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base
 
 		}
 		case MONO_TABLE_EVENT: {
-			if (!is_addition)
-				/* FIXME: use DeltaInfo:prev_gen_rows instead of image_base */
-				g_assert (token_index <= table_info_get_rows (&image_base->tables [token_table]));
-			else {
+			if (is_addition) {
 				g_assert (add_event_eventmap != 0);
 
 				uint32_t parent_type_token = mono_metadata_decode_row_col (&image_base->tables [MONO_TABLE_EVENTMAP], mono_metadata_token_index (add_event_eventmap) - 1, MONO_EVENT_MAP_PARENT);
@@ -2869,6 +2863,7 @@ add_member_to_baseline (BaselineInfo *base_info, DeltaInfo *delta_info, MonoClas
 	GSList *members = klass_info->added_members;
 	klass_info->added_members = g_slist_prepend_mem_manager (m_class_get_mem_manager (klass), members, GUINT_TO_POINTER (member_token));
 	add_member_parent (base_info, m_class_get_type_token (klass), member_token);
+	klass_info->generation = delta_info->generation;
 }
 
 
@@ -3025,6 +3020,7 @@ hot_reload_get_field (MonoClass *klass, uint32_t fielddef_token) {
 static MonoProperty *
 hot_reload_get_property (MonoClass *klass, uint32_t property_token)
 {
+	g_assert (m_class_get_class_kind (klass) != MONO_CLASS_GINST);
 	MonoClassMetadataUpdateInfo *info = mono_class_get_or_add_metadata_update_info (klass);
 	g_assert (mono_metadata_token_table (property_token) == MONO_TABLE_PROPERTY);
 	GSList *added_props = info->added_props;
@@ -3040,6 +3036,7 @@ hot_reload_get_property (MonoClass *klass, uint32_t property_token)
 static MonoEvent *
 hot_reload_get_event (MonoClass *klass, uint32_t event_token)
 {
+	g_assert (m_class_get_class_kind (klass) != MONO_CLASS_GINST);
 	MonoClassMetadataUpdateInfo *info = mono_class_get_or_add_metadata_update_info (klass);
 	g_assert (mono_metadata_token_table (event_token) == MONO_TABLE_EVENT);
 	GSList *added_events = info->added_events;
@@ -3108,6 +3105,8 @@ add_property_to_existing_class (MonoImage *image_base, BaselineInfo *base_info,
 
 	parent_info->added_props = g_slist_prepend_mem_manager (m_class_get_mem_manager (parent_klass), parent_info->added_props, prop);
 
+	parent_info->generation = generation;
+
 	return prop;
 
 }
@@ -3134,6 +3133,8 @@ add_event_to_existing_class (MonoImage *image_base, BaselineInfo *base_info, uin
 
 	parent_info->added_events = g_slist_prepend_mem_manager (m_class_get_mem_manager (parent_klass), parent_info->added_events, evt);
 
+	parent_info->generation = generation;
+
 	return evt;
 
 }
@@ -3167,7 +3168,6 @@ add_semantic_method_to_existing_property (MonoImage *image_base, BaselineInfo *b
 
 
 	g_assert (dest != NULL);
-	g_assert (*dest == NULL);
 
 	MonoMethod *method = mono_get_method_checked (image_base, method_token, klass, NULL, error);
 	mono_error_assert_ok (error);
@@ -3207,7 +3207,6 @@ add_semantic_method_to_existing_event (MonoImage *image_base, BaselineInfo *base
 	}
 
 	g_assert (dest != NULL);
-	g_assert (*dest == NULL);
 
 	MonoMethod *method = mono_get_method_checked (image_base, method_token, klass, NULL, error);
 	mono_error_assert_ok (error);