Skip to content

Commit

Permalink
[metadata] Fields whose types are gparams with a reference type const…
Browse files Browse the repository at this point in the history
…raint aren't blittlable. (mono#15761)

* [metadata] Fields whose types are gparams with a reference type constraint
aren't blittlable.
Don't try to layout the field to find out if it's blittable.
For gshared gparams, follow the blittability of the constraint.

Fixes certain recursive examples.

```
using System;

namespace TestRecursiveType
{
    class Program
    {
        static void Main(string[] args)
        {
            SubClass subC = new SubClass();
            Console.WriteLine(subC.GetTest());
        }
    }

    public struct ValueTest<U>
    {
        // When U is instantiated with T, from BaseClass, we know it'll be a
	// reference field, so we know the instantiation ValueTest<T> won't
	// be blittable.
        public readonly U value;
    }

    public abstract class BaseClass<T> where T : BaseClass<T>
    {
        public ValueTest<T> valueTest = default(ValueTest<T>);
    }

    public class SubClass : BaseClass<SubClass>
    {
        private String test = "test";

        public string GetTest()
        {
            return test;
        }
    }
}
```

Fixes mono#15760

---

The failure is happening when we are doing mono_class_setup_fields ("BaseClass<T>") which needs to decide for each field whether it is blittable or not. So therefore we are trying to decide if ValueTest<T> (that is: the ValueTest<U> field inside BaseClass<T>) is blittable or not.

So we instantiate U with T.
Now to decide whether VaueTest<T> is blittable or not, we look at every field.
So then we look at T value.
To decide if T is blittable we first check if it's a reference type.

That check is currently inadequate for generic parameters - what the PR adds is the ability to see if theres a T : class constraint or a T : C constraint - where C is some class. As soon as we know that T's constraint will force it to be a reference type we can definitely say that T won't be blittable without having to initialize C, at all.

Previously, Mono would see that T is some kind of type for which it couldn't definitively decide that it's a reference type and it would call: mono_class_setup_fields (field_class) which would then try to setup the fields of the parent class BaseClass<T>. And that would hit the recursion check.

Unity cherry-pick note: Needed to bring MONO_CLASS_IS_INTERFACE_INTERNAL
and mono_class_get_valuetype_class forward
  • Loading branch information
lambdageek authored and UnityAlex committed Jul 30, 2019
1 parent 7a3ac72 commit 8e9be33
Show file tree
Hide file tree
Showing 6 changed files with 169 additions and 0 deletions.
4 changes: 4 additions & 0 deletions mono/metadata/class-internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -1186,6 +1186,7 @@ GENERATE_GET_CLASS_WITH_CACHE_DECL (variant)
#endif

GENERATE_GET_CLASS_WITH_CACHE_DECL (appdomain_unloaded_exception)
GENERATE_GET_CLASS_WITH_CACHE_DECL (valuetype)

extern MonoDefaults mono_defaults;

Expand Down Expand Up @@ -1371,6 +1372,9 @@ mono_class_vtable_full (MonoDomain *domain, MonoClass *klass, MonoError *error);
gboolean
mono_class_is_assignable_from_slow (MonoClass *target, MonoClass *candidate);

MonoClass*
mono_generic_param_get_base_type (MonoClass *klass);

gboolean
mono_class_has_variant_generic_params (MonoClass *klass);

Expand Down
112 changes: 112 additions & 0 deletions mono/metadata/class.c
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ static gboolean mono_class_set_failure (MonoClass *klass, MonoErrorBoxed *boxed_

static gboolean class_kind_may_contain_generic_instances (MonoTypeKind kind);

GENERATE_GET_CLASS_WITH_CACHE (valuetype, "System", "ValueType");


/*
We use gclass recording to allow recursive system f types to be referenced by a parent.
Expand Down Expand Up @@ -1760,6 +1762,59 @@ type_has_references (MonoClass *klass, MonoType *ftype)
return FALSE;
}

/**
* mono_class_is_gparam_with_nonblittable_parent:
* \param klass a generic parameter
*
* \returns TRUE if \p klass is definitely not blittable.
*
* A parameter is definitely not blittable if it has the IL 'reference'
* constraint, or if it has a class specified as a parent. If it has an IL
* 'valuetype' constraint or no constraint at all or only interfaces as
* constraints, we return FALSE because the parameter may be instantiated both
* with blittable and non-blittable types.
*
* If the paramter is a generic sharing parameter, we look at its gshared_constraint->blittable bit.
*/
static gboolean
mono_class_is_gparam_with_nonblittable_parent (MonoClass *klass)
{
MonoType *type = &klass->byval_arg;
g_assert (mono_type_is_generic_parameter (type));
MonoGenericParam *gparam = type->data.generic_param;
if (mono_generic_param_info (gparam) != NULL)

This comment has been minimized.

Copy link
@UnityAlex

UnityAlex Jul 30, 2019

Collaborator

I had to add this null check here. Upstream the MonoGenericParamInfo is stored within the MonoGenericParam struct and is possibly why this is never null?

{
if ((mono_generic_param_info (gparam)->flags & GENERIC_PARAMETER_ATTRIBUTE_REFERENCE_TYPE_CONSTRAINT) != 0)
return TRUE;
if ((mono_generic_param_info (gparam)->flags & GENERIC_PARAMETER_ATTRIBUTE_VALUE_TYPE_CONSTRAINT) != 0)
return FALSE;
}

if (gparam->gshared_constraint) {
MonoClass *constraint_class = mono_class_from_mono_type (gparam->gshared_constraint);
return !constraint_class->blittable;
}

if (mono_generic_param_owner (gparam)->is_anonymous)
return FALSE;

/* We could have: T : U, U : Base. So have to follow the constraints. */
MonoClass *parent_class = mono_generic_param_get_base_type (klass);
g_assert (!MONO_CLASS_IS_INTERFACE_INTERNAL (parent_class));
/* Parent can only be: System.Object, System.ValueType or some specific base class.
*
* If the parent_class is ValueType, the valuetype constraint would be set, above, so
* we wouldn't get here.
*
* If there was a reference constraint, the parent_class would be System.Object,
* but we would have returned early above.
*
* So if we get here, there is either no base class constraint at all,
* in which case parent_class would be set to System.Object, or there is none at all.
*/
return parent_class != mono_defaults.object_class;
}

/*
* mono_class_layout_fields:
* @class: a class
Expand Down Expand Up @@ -1877,6 +1932,9 @@ mono_class_layout_fields (MonoClass *klass, int base_instance_size, int packing_
if (blittable) {
if (field->type->byref || MONO_TYPE_IS_REFERENCE (field->type)) {
blittable = FALSE;
} else if (mono_type_is_generic_parameter (field->type) &&
mono_class_is_gparam_with_nonblittable_parent (mono_class_from_mono_type (field->type))) {
blittable = FALSE;
} else {
MonoClass *field_class = mono_class_from_mono_type (field->type);
if (field_class) {
Expand Down Expand Up @@ -8645,6 +8703,60 @@ mono_class_is_assignable_from_slow (MonoClass *target, MonoClass *candidate)
return FALSE;
}

/**
* mono_generic_param_get_base_type:
*
* Return the base type of the given generic parameter from its constraints.
*
* Could be another generic parameter, or it could be Object or ValueType.
*/
MonoClass*
mono_generic_param_get_base_type (MonoClass *klass)
{
MonoType *type = &klass->byval_arg;
g_assert (mono_type_is_generic_argument (type));

MonoGenericParam *gparam = type->data.generic_param;

g_assert (gparam->owner && !gparam->owner->is_anonymous);

MonoClass **constraints = mono_generic_container_get_param_info (gparam->owner, gparam->num)->constraints;

MonoClass *base_class = mono_defaults.object_class;

if (constraints) {
int i;
for (i = 0; constraints [i]; ++i) {
MonoClass *constraint = constraints[i];

if (MONO_CLASS_IS_INTERFACE_INTERNAL (constraint))
continue;

MonoType *constraint_type = &constraint->byval_arg;
if (mono_type_is_generic_argument (constraint_type)) {
MonoGenericParam *constraint_param = constraint_type->data.generic_param;
MonoGenericParamInfo *constraint_info = mono_generic_param_info (constraint_param);
if ((constraint_info->flags & GENERIC_PARAMETER_ATTRIBUTE_REFERENCE_TYPE_CONSTRAINT) == 0 &&
(constraint_info->flags & GENERIC_PARAMETER_ATTRIBUTE_VALUE_TYPE_CONSTRAINT) == 0)
continue;
}

base_class = constraint;
}

}

if (base_class == mono_defaults.object_class)
{
MonoGenericParamInfo *gparam_info = mono_generic_param_info (gparam);
if ((gparam_info->flags & GENERIC_PARAMETER_ATTRIBUTE_VALUE_TYPE_CONSTRAINT) != 0) {
base_class = mono_class_get_valuetype_class ();
}
}

return base_class;
}

/**
* mono_class_get_cctor:
* \param klass A MonoClass pointer
Expand Down
2 changes: 2 additions & 0 deletions mono/metadata/metadata-internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -974,5 +974,7 @@ mono_signature_get_managed_fmt_string (MonoMethodSignature *sig);
gboolean
mono_type_in_image (MonoType *type, MonoImage *image);

#define MONO_CLASS_IS_INTERFACE_INTERNAL(c) ((mono_class_get_flags (c) & TYPE_ATTRIBUTE_INTERFACE) || mono_type_is_generic_parameter (&c->byval_arg))

#endif /* __MONO_METADATA_INTERNALS_H__ */

5 changes: 5 additions & 0 deletions mono/metadata/metadata.c
Original file line number Diff line number Diff line change
Expand Up @@ -6815,6 +6815,11 @@ mono_type_is_pointer (MonoType *type)
mono_bool
mono_type_is_reference (MonoType *type)
{
/* NOTE: changing this function to return TRUE more often may have
* consequences for generic sharing in the AOT compiler. In
* particular, returning TRUE for generic parameters with a 'class'
* constraint may cause crashes.
*/
return (type && (((type->type == MONO_TYPE_STRING) ||
(type->type == MONO_TYPE_SZARRAY) || (type->type == MONO_TYPE_CLASS) ||
(type->type == MONO_TYPE_OBJECT) || (type->type == MONO_TYPE_ARRAY)) ||
Expand Down
1 change: 1 addition & 0 deletions mono/tests/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ TESTS_CS_SRC= \
generic-stack-traces2.2.cs \
bug-472600.2.cs \
recursive-generics.2.cs \
recursive-generics.3.cs \
bug-473482.2.cs \
bug-473999.2.cs \
bug-479763.2.cs \
Expand Down
45 changes: 45 additions & 0 deletions mono/tests/recursive-generics.3.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
using System;

class Program {
static void Main (string[] args)
{
// If this runs without a TLE, the test passed. A
// TypeLoadException due to recursion during type
// initialization is a failure.
var subC = new SubClass ();
Console.WriteLine (subC.GetTest ());
// same as above, but try to land in generic sharing code.
var genSubC = new GenericSubClass<object> ();
Console.WriteLine (genSubC.GetTest ());
}
}

public struct ValueTest<U> {
// When U is instantiated with T, from BaseClass, we know it'll be a
// reference field without having to fully initialize its parent
// (namely BaseClass<T> itself), so we know the instantiation
// ValueTest<T> won't be blittable.
public readonly U value;
}

public abstract class BaseClass<T> where T : BaseClass<T> {
public ValueTest<T> valueTest = default (ValueTest<T>);
}

public class SubClass : BaseClass<SubClass> {
private string test = "test";

public string GetTest()
{
return test;
}
}

public class GenericSubClass<T> : BaseClass<GenericSubClass<T>> {
private string test = "test";

public string GetTest()
{
return test;
}
}

0 comments on commit 8e9be33

Please sign in to comment.