From 88633ae045e7741fffa17710dc48e9032e519258 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Wed, 9 Aug 2023 01:47:42 -0400 Subject: [PATCH] [hot_reload] ignore modified MONO_TABLE_TYPEDEF rows in update (#90166) * Add test that deletes a custom attribute from a class * just ignore modified MONO_TABLE_TYPEDEF rows in updates We may want to validate that Parent, Interfaces and Attributes columns haven't changed, but it's tricky and might be overly restrictive --- .../CustomAttributeDelete.cs | 3 +- .../CustomAttributeDelete_v1.cs | 2 +- .../System.Runtime.Loader/tests/ApplyUpdateTest.cs | 21 +++++++++-- src/mono/mono/component/hot_reload.c | 44 +++++++++++++++++++--- 4 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete.cs index 1781dcf..750f328 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete.cs @@ -5,7 +5,7 @@ using System; namespace System.Reflection.Metadata.ApplyUpdate.Test { - [AttributeUsage (AttributeTargets.Method, AllowMultiple=true)] + [AttributeUsage (AttributeTargets.Method | AttributeTargets.Class, AllowMultiple=true)] public class MyDeleteAttribute : Attribute { public MyDeleteAttribute (string stringValue) { StringValue = stringValue; } @@ -19,6 +19,7 @@ namespace System.Reflection.Metadata.ApplyUpdate.Test public int IntValue {get; set; } } + [MyDeleteAttribute ("xyz")] public class ClassWithCustomAttributeDelete { [MyDeleteAttribute ("abcd")] diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete_v1.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete_v1.cs index db85640..ae6384e 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete_v1.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.CustomAttributeDelete/CustomAttributeDelete_v1.cs @@ -5,7 +5,7 @@ using System; namespace System.Reflection.Metadata.ApplyUpdate.Test { - [AttributeUsage (AttributeTargets.Method, AllowMultiple=true)] + [AttributeUsage (AttributeTargets.Method | AttributeTargets.Class, AllowMultiple=true)] public class MyDeleteAttribute : Attribute { public MyDeleteAttribute (string stringValue) { StringValue = stringValue; } diff --git a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs index 05b3736..8ca1268 100644 --- a/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs +++ b/src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs @@ -198,18 +198,33 @@ namespace System.Reflection.Metadata { var assm = typeof(System.Reflection.Metadata.ApplyUpdate.Test.ClassWithCustomAttributeDelete).Assembly; + Type attrType = typeof(System.Reflection.Metadata.ApplyUpdate.Test.MyDeleteAttribute); + + Type preUpdateTy = assm.GetType("System.Reflection.Metadata.ApplyUpdate.Test.ClassWithCustomAttributeDelete"); + Assert.NotNull(preUpdateTy); + + // before the update the type has a MyDeleteAttribute on it + Attribute[] cattrs = Attribute.GetCustomAttributes(preUpdateTy, attrType); + Assert.NotNull(cattrs); + Assert.Equal(1, cattrs.Length); + ApplyUpdateUtil.ApplyUpdate(assm); ApplyUpdateUtil.ClearAllReflectionCaches(); - // Just check the updated value on one method - Type attrType = typeof(System.Reflection.Metadata.ApplyUpdate.Test.MyDeleteAttribute); Type ty = assm.GetType("System.Reflection.Metadata.ApplyUpdate.Test.ClassWithCustomAttributeDelete"); Assert.NotNull(ty); + // After the update, the type has no MyDeleteAttribute on it anymore + cattrs = Attribute.GetCustomAttributes(ty, attrType); + Assert.NotNull(cattrs); + Assert.Equal(0, cattrs.Length); + + // Just check the updated value on one method + MethodInfo mi1 = ty.GetMethod(nameof(System.Reflection.Metadata.ApplyUpdate.Test.ClassWithCustomAttributeDelete.Method1), BindingFlags.Public | BindingFlags.Static); Assert.NotNull(mi1); - Attribute[] cattrs = Attribute.GetCustomAttributes(mi1, attrType); + cattrs = Attribute.GetCustomAttributes(mi1, attrType); Assert.NotNull(cattrs); Assert.Equal(0, cattrs.Length); diff --git a/src/mono/mono/component/hot_reload.c b/src/mono/mono/component/hot_reload.c index 1b0fccf..2245fcb 100644 --- a/src/mono/mono/component/hot_reload.c +++ b/src/mono/mono/component/hot_reload.c @@ -1668,7 +1668,13 @@ apply_enclog_pass1 (MonoImage *image_base, MonoImage *image_dmeta, DeltaInfo *de i++; /* skip the next record */ continue; } - /* fallthru */ + // don't expect to see any other func codes with this table + g_assert (func_code == ENC_FUNC_DEFAULT); + // If it's an addition, it's an added type definition, continue. + + // If it's a modification, Roslyn sometimes sends this when a custom + // attribute is deleted from a type definition. + break; } default: if (!is_addition) { @@ -2279,16 +2285,42 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base * especially since from the next generation's point of view * that's what adding a field/method will be. */ break; - case ENC_FUNC_ADD_EVENT: - g_assert_not_reached (); /* FIXME: implement me */ default: g_assert_not_reached (); /* unknown func_code */ } break; + } else { + switch (func_code) { + case ENC_FUNC_DEFAULT: + // Roslyn sends this sometimes when it deletes a custom + // attribute. In this case no rows of the table def have + // should have changed from the previous generation. + + // Note: we may want to check that Parent and Interfaces + // haven't changed. But note that's tricky to do: we can't + // just compare tokens: the parent could be a TypeRef token, + // and roslyn does send a new typeref row (that ends up + // referencing the same type definition). But we also don't + // want to convert to a `MonoClass*` too eagerly - if the + // class hasn't been used yet we don't want to kick off + // class initialization (which could mention the current + // class thanks to generic arguments - see class-init.c) + // Same with Interfaces. Fields and Methods in an EnC + // updated row are zero. So that only really leaves + // Attributes as the only column we can compare, which + // wouldn't tell us much (and perhaps in the future Roslyn + // could allow changing visibility, so we wouldn't want to + // compare for equality, anyway) So... we're done. + break; + case ENC_FUNC_ADD_METHOD: + case ENC_FUNC_ADD_FIELD: + /* modifying an existing class by adding a method or field, etc. */ + break; + default: + /* not expecting anything else */ + g_assert_not_reached (); + } } - /* modifying an existing class by adding a method or field, etc. */ - g_assert (!is_addition); - g_assert (func_code != ENC_FUNC_DEFAULT); break; } case MONO_TABLE_NESTEDCLASS: { -- 2.7.4