Skip to content

Commit

Permalink
[release/7.0] [hot_reload] Fix unresolved token when new nested struc…
Browse files Browse the repository at this point in the history
…ts are used (#76625)

* [test] Calling the ctor of a class with a nested struct

This fails with

```
       Stack Trace:

          Child exception:
            System.BadImageFormatException: Could not resolve field token 0x04000014
          File name: 'System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType'
          /Users/alklig/work/dotnet-runtime/runtime/src/libraries/System.Runtime.Loader/tests/ApplyUpdate/System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType/ReflectionAddNewType.cs(29,0):
          at
          System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType.ZExistingClass.ExistingMethod()

```

* Don't resolve the types of added fields too early

Co-authored-by: Aleksey Kliger <[email protected]>
  • Loading branch information
github-actions[bot] and lambdageek authored Oct 5, 2022
1 parent d8b3db4 commit 44734a5
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ public class PreviousNestedClass {
public event EventHandler<string> E;
public void R() { E(this,"123"); }
}

public static void ExistingMethod () {}
}

[AttributeUsage(AttributeTargets.All, AllowMultiple=true, Inherited=false)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ public class NewNestedClass {};
public static DateTime NewStaticField;

public static double NewProp { get; set; }

public static void ExistingMethod ()
{
// modified
NewStaticField2 = new AnotherAddedClass();
}

public static AnotherAddedClass NewStaticField2;
}

[AttributeUsage(AttributeTargets.All, AllowMultiple=true, Inherited=false)]
Expand Down Expand Up @@ -87,3 +95,22 @@ public interface INewInterface : IExistingInterface {
public enum NewEnum {
Red, Yellow, Green
}

public class AnotherAddedClass
{
public struct NewNestedStruct
{
public double D;
public object O;
}

public NewNestedStruct S;

public AnotherAddedClass()
{
S = new NewNestedStruct {
D = 1234.0,
O = "1234",
};
}
}
2 changes: 2 additions & 0 deletions src/libraries/System.Runtime.Loader/tests/ApplyUpdateTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,8 @@ public static void TestReflectionAddNewType()
var i = (System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType.IExistingInterface)o;
Assert.Equal("123", i.ItfMethod(123));
System.Reflection.Metadata.ApplyUpdate.Test.ReflectionAddNewType.ZExistingClass.ExistingMethod ();
});
}
}
Expand Down
22 changes: 13 additions & 9 deletions src/mono/mono/component/hot_reload.c
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ static const char *
hot_reload_get_capabilities (void);

static MonoClassMetadataUpdateField *
metadata_update_field_setup_basic_info_and_resolve (MonoImage *image_base, BaselineInfo *base_info, uint32_t generation, DeltaInfo *delta_info, MonoClass *parent_klass, uint32_t fielddef_token, uint32_t field_flags, MonoError *error);
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);

static MonoComponentHotReload fn_table = {
{ MONO_COMPONENT_ITF_VERSION, &hot_reload_available },
Expand Down Expand Up @@ -2189,11 +2189,11 @@ apply_enclog_pass2 (Pass2Context *ctx, MonoImage *image_base, BaselineInfo *base

add_field_to_baseline (base_info, delta_info, add_member_klass, log_token);

/* This actually does more than mono_class_setup_basic_field_info and
* resolves MonoClassField:type and sets MonoClassField:offset to -1 to make
* it easier to spot that the field is special.
/* This actually does slightly more than
* mono_class_setup_basic_field_info and sets MonoClassField:offset
* to -1 to make it easier to spot that the field is special.
*/
metadata_update_field_setup_basic_info_and_resolve (image_base, base_info, generation, delta_info, add_member_klass, log_token, field_flags, error);
metadata_update_field_setup_basic_info (image_base, base_info, generation, delta_info, add_member_klass, log_token, field_flags);
if (!is_ok (error)) {
mono_trace (G_LOG_LEVEL_WARNING, MONO_TRACE_METADATA_UPDATE, "Could not setup field (token 0x%08x) due to: %s", log_token, mono_error_get_message (error));
return FALSE;
Expand Down Expand Up @@ -2884,7 +2884,7 @@ hot_reload_get_field (MonoClass *klass, uint32_t fielddef_token) {


static MonoClassMetadataUpdateField *
metadata_update_field_setup_basic_info_and_resolve (MonoImage *image_base, BaselineInfo *base_info, uint32_t generation, DeltaInfo *delta_info, MonoClass *parent_klass, uint32_t fielddef_token, uint32_t field_flags, MonoError *error)
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)
{
if (!m_class_is_inited (parent_klass))
mono_class_init_internal (parent_klass);
Expand All @@ -2903,9 +2903,13 @@ metadata_update_field_setup_basic_info_and_resolve (MonoImage *image_base, Basel
uint32_t name_idx = mono_metadata_decode_table_row_col (image_base, MONO_TABLE_FIELD, mono_metadata_token_index (fielddef_token) - 1, MONO_FIELD_NAME);
field->field.name = mono_metadata_string_heap (image_base, name_idx);

mono_field_resolve_type (&field->field, error);
if (!is_ok (error))
return NULL;
/* It's important not to try and resolve field->type at this point. If the field's type is a
* newly-added struct, we don't want to resolve it early here if we're going to add fields
* and methods to it. It seems that for nested structs, the field additions come after the
* field addition to the enclosing struct. So if the enclosing struct has a field of the
* nested type, resolving the field type here will make it look like the nested struct has
* no fields.
*/

parent_info->added_fields = g_slist_prepend_mem_manager (m_class_get_mem_manager (parent_klass), parent_info->added_fields, field);

Expand Down
10 changes: 8 additions & 2 deletions src/mono/mono/metadata/class.c
Original file line number Diff line number Diff line change
Expand Up @@ -6604,8 +6604,14 @@ mono_field_resolve_type (MonoClassField *field, MonoError *error)
static guint32
mono_field_resolve_flags (MonoClassField *field)
{
/* Fields in metadata updates are pre-resolved, so this method should not be called. */
g_assert (!m_field_is_from_update (field));
if (G_UNLIKELY (m_field_is_from_update (field))) {
/* metadata-update: Just resolve the whole field, for simplicity. */
ERROR_DECL (error);
mono_field_resolve_type (field, error);
mono_error_assert_ok (error);
g_assert (field->type);
return field->type->attrs;
}

MonoClass *klass = m_field_get_parent (field);
MonoImage *image = m_class_get_image (klass);
Expand Down

0 comments on commit 44734a5

Please sign in to comment.