Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono] When creating an array type, don't do full initialization of the element type #85828

Merged
merged 4 commits into from
May 22, 2023

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented May 5, 2023

Consider code like this:

public class Node<T>
{
    public Node<T>[][] Children;
}


Console.WriteLine (typeof(Node<int>));

In this case, when we're JITing ldtoken class Node`1<int32> we first have to parse the type Node`1<int32> and initialize it.
When we're initializing it, we need to resolve the types of all the fields in the class in order to to figure out if its instance size.

To resolve the field types we have to parse the types of all the fields, and we eventually end up parsing the type Node`1<!0>[][] which ends up here:

MonoType *etype = mono_metadata_parse_type_checked (m, container, 0, transient, ptr, &ptr, error);
if (!etype)
return FALSE;
type->data.klass = mono_class_from_mono_type_internal (etype);

When we get to line 4027 the second time (recursively), etype is Node`1<!0> as a MonoType*. And we want to set type->data.klass to its corresponding MonoClass*. That ends up calling mono_class_from_mono_type_internal, which does this:

case MONO_TYPE_SZARRAY:
return mono_class_create_array (type->data.klass, 1);

When we call mono_class_create_array we end up here:

if (mono_class_is_ginst (eclass))
mono_class_init_internal (eclass);

with eclass equal to Node`1<!0>` which we try to initialize and we get a TLE because we're going to try to initialize the same generic type definition Node`1`` twice.

Compare with this other class:

public unsafe class Node2<T>
{
    public Node2<T>[] Children;
}

In this case we only end up calling mono_class_from_mono_type_internal on Node2`1<int32> (not an array). And that branch does not do any initialization - it just returns type->data.klass (for Node2`1<!0> - which is seen as a gtd at this point) or mono_class_create_generic_inst (type->data.generic_class) (for Node2`1<int32> which is seen later when ldtoken inflates the gtd)) - without any extra initialization.


It seems the reason we get into trouble is because mono_class_create_array does more work than other MonoClass creation functions - it tries to initialize the MonoClass a little bit (for example by setting MonoClass:has_references) which needs at least a little bit of the element class to be initialized.

But note that the code has this:

if (mono_class_is_ginst (eclass))
mono_class_init_internal (eclass);
if (!eclass->size_inited)
mono_class_setup_fields (eclass);

I feel fairly certain we don't need the full class initialization if we're going to immediately initialize the field size information.

Fixes #85821

@lambdageek lambdageek changed the title [DRAFT] try to init less when creating array types [mono] When creating an array type, don't do full initialization of the element type May 19, 2023
@lambdageek lambdageek added area-AssemblyLoader-mono and removed NO-REVIEW Experimental/testing PR, do NOT review it labels May 19, 2023
@lambdageek lambdageek marked this pull request as ready for review May 19, 2023 20:44
@ghost
Copy link

ghost commented May 19, 2023

Tagging subscribers to this area:
See info in area-owners.md if you want to be subscribed.

Issue Details

Consider code like this:

public class Node<T>
{
    public Node<T>[][] Children;
}


Console.WriteLine (typeof(Node<int>));

In this case, when we're JITing ldtoken class Node`1<int32> we first have to parse the type Node`1<int32> and initialize it.
When we're initializing it, we need to resolve the types of all the fields in the class in order to to figure out if its instance size.

To resolve the field types we have to parse the types of all the fields, and we eventually end up parsing the type Node`1<int32>[][] which ends up here:

MonoType *etype = mono_metadata_parse_type_checked (m, container, 0, transient, ptr, &ptr, error);
if (!etype)
return FALSE;
type->data.klass = mono_class_from_mono_type_internal (etype);

When we get to line 4027 the second time (recursively), etype is Node`1<int32> as a MonoType*. And we want to set type->data.klass to its corresponding MonoClass*. That ends up callin mono_class_from_mono_type_internal, which does this:

case MONO_TYPE_SZARRAY:
return mono_class_create_array (type->data.klass, 1);

When we call mono_class_create_array we end up here:

if (mono_class_is_ginst (eclass))
mono_class_init_internal (eclass);

with eclass equal to Node`1<int32>` which we try to initialize and we get a TLE because we're going to try to initialize the same generic type definition Node`1`` twice.

Compare with this other class:

public unsafe class Node2<T>
{
    public Node2<T>[] Children;
}

In this case we only end up calling mono_class_from_mono_type_internal on Node2`1<int32> (not an array). And that branch does not do any initialization - it just returns type->data.klass (for Node2`1<!0> - which is seen as a gtd at this point) or mono_class_create_generic_inst (type->data.generic_class) (for Node2`1<int32> which is seen later when ldtoken inflates the gtd)) - without any extra initialization.


It seems the reason we get into trouble is because mono_class_create_array does more work than other MonoClass creation functions - it tries to initialize the MonoClass a little bit (for example by setting MonoClass:has_references) which needs at least a little bit of the element class to be initialized.

But note that the code has this:

if (mono_class_is_ginst (eclass))
mono_class_init_internal (eclass);
if (!eclass->size_inited)
mono_class_setup_fields (eclass);

I feel fairly certain we don't need the full class initialization if we're going to immediately initialize the field size information.

Fixes #85821

Author: lambdageek
Assignees: lambdageek
Labels:

area-AssemblyLoader-mono

Milestone: -

@lambdageek
Copy link
Member Author

@thaystg @vargaz PTAL

@lambdageek lambdageek merged commit bdf5df3 into dotnet:main May 22, 2023
joshpeterson pushed a commit to Unity-Technologies/mono that referenced this pull request May 22, 2023
Mono was a bit too aggressive with its TypeLoadException processing, not
allowing some valid code in this case. This change applies the fix from:

dotnet/runtime#85828
Saiprasad945 pushed a commit to Unity-Technologies/mono that referenced this pull request Jun 6, 2023
Mono was a bit too aggressive with its TypeLoadException processing, not
allowing some valid code in this case. This change applies the fix from:

dotnet/runtime#85828
Saiprasad945 pushed a commit to Unity-Technologies/mono that referenced this pull request Jun 6, 2023
Mono was a bit too aggressive with its TypeLoadException processing, not
allowing some valid code in this case. This change applies the fix from:

dotnet/runtime#85828
Saiprasad945 pushed a commit to Unity-Technologies/mono that referenced this pull request Jun 6, 2023
Mono was a bit too aggressive with its TypeLoadException processing, not
allowing some valid code in this case. This change applies the fix from:

dotnet/runtime#85828
@ghost ghost locked as resolved and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mono incorrectly throws a TypeLoadException for a recursive type definition
2 participants