Skip to content

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

@ghost ghost added the area-VM-meta-mono label May 5, 2023
@ghost ghost assigned lambdageek May 5, 2023
@lambdageek lambdageek added NO-REVIEW Experimental/testing PR, do NOT review it and removed area-VM-meta-mono labels May 5, 2023
@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
@lambdageek lambdageek requested review from thaystg and vargaz as code owners 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