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.

Sign up for GitHub

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

Guard members of MonoType union & fix related bugs #111645

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft

Guard members of MonoType union & fix related bugs #111645

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
71d52ae
Guard members of MonoType union behind type check helpers
kg Jan 20, 2025
55fdf8a
Reduce overhead of type access helpers
kg Jan 20, 2025
3ec7a60
Cleanup and fix CI failure
kg Jan 21, 2025
8ad069c
Expand allowed cases
kg Jan 21, 2025
2d41bb5
Fix actual bug in bulk_type_log_single_type
kg Jan 21, 2025
5fd3623
Steps to address PR feedback / cleanup
kg Jan 22, 2025
db37234
Cleanup and add comments
kg Jan 22, 2025
1145217
Add _unchecked to call sites that are obviously guarded correctly
kg Jan 22, 2025
31fa3c9
More unchecked and comment improvement for IDEs
kg Jan 22, 2025
ae64025
More unchecked and comments
kg Jan 22, 2025
fdf66d2
More unchecked
kg Jan 22, 2025
c8c6460
Attempt to fix szarray handler in eventpipe
kg Jan 22, 2025
41bf23c
Fix a bug and add comments
kg Jan 22, 2025
48e7a1a
Add comment
kg Jan 22, 2025
cb26e90
Fix typo
kg Jan 22, 2025
1fc1fe5
Revert whitespace changes
kg Jan 23, 2025
8153494
Fix android and iOS build
kg Jan 23, 2025
40e6dd9
Checkpoint
kg Jan 24, 2025
892e4fe
Hide the MonoType data union and introduce checked/unchecked setters
kg Jan 24, 2025
772a3fa
Fix genericinst fallthrough treating a MonoGenericClass ptr as a Mono…
kg Jan 24, 2025
dac61f3
Fix genericinst fallthrough treating a MonoGenericClass ptr as a Mono…
kg Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/mono/mono/eventpipe/ep-rt-mono-runtime-provider.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1554,8 +1554,15 @@ bulk_type_log_single_type (
// Sets val variable sized parameter type data, type_parameters_count, and mono_type_parameters associated
// with arrays or generics to be recursively batched in the same ep_rt_mono_log_type_and_parameters call
switch (mono_underlying_type->type) {
case MONO_TYPE_ARRAY:
case MONO_TYPE_SZARRAY:
{
// FIXME: Previously was handled by below ARRAY block but that is incorrect; this implementation is speculative -kg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you should be able to handle both cases similar, except that you can't assume mono_type is MonoArrayType it could also be MonoClass, so you would just need to handle the case where you get the type, rank and mono_type_parameter differently.

val->fixed_sized_data.flags |= TYPE_FLAGS_ARRAY;
// FIXME: Do we have to encode the rank of 1 explicitly?
// FIXME: Encode the mono_type_parameters and type_parameters_count
break;
}
case MONO_TYPE_ARRAY:
{
MonoArrayType *mono_array_type = mono_type_get_array_type (mono_type);
val->fixed_sized_data.flags |= TYPE_FLAGS_ARRAY;
Expand Down
6 changes: 3 additions & 3 deletions src/mono/mono/metadata/class-init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1887,7 +1887,7 @@ type_has_references (MonoClass *klass, MonoType *ftype)
if (MONO_TYPE_IS_REFERENCE (ftype) || IS_GC_REFERENCE (klass, ftype) || ((MONO_TYPE_ISSTRUCT (ftype) && class_has_references (mono_class_from_mono_type_internal (ftype)))))
return TRUE;
if (!m_type_is_byref (ftype) && (ftype->type == MONO_TYPE_VAR || ftype->type == MONO_TYPE_MVAR)) {
MonoGenericParam *gparam = ftype->data.generic_param;
MonoGenericParam *gparam = m_type_data_get_generic_param (ftype);

if (gparam->gshared_constraint)
return class_has_references (mono_class_from_mono_type_internal (gparam->gshared_constraint));
Expand Down Expand Up @@ -1935,7 +1935,7 @@ mono_class_is_gparam_with_nonblittable_parent (MonoClass *klass)
{
MonoType *type = m_class_get_byval_arg (klass);
g_assert (mono_type_is_generic_parameter (type));
MonoGenericParam *gparam = type->data.generic_param;
MonoGenericParam *gparam = m_type_data_get_generic_param (type);
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)
Expand Down Expand Up @@ -2699,7 +2699,7 @@ setup_generic_array_ifaces (MonoClass *klass, MonoClass *iface, MonoMethod **met
if (mono_class_is_gtd (iface)) {
MonoType *ty = mono_class_gtd_get_canonical_inst (iface);
g_assert (ty->type == MONO_TYPE_GENERICINST);
gclass = ty->data.generic_class;
gclass = m_type_data_get_generic_class (ty);
} else
gclass = mono_class_get_generic_class (iface);

Expand Down
4 changes: 2 additions & 2 deletions src/mono/mono/metadata/class-internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,13 +545,13 @@ mono_generic_param_name (MonoGenericParam *p)
static inline MonoGenericContainer *
mono_type_get_generic_param_owner (MonoType *t)
{
return mono_generic_param_owner (t->data.generic_param);
return mono_generic_param_owner (m_type_data_get_generic_param (t));
}

static inline guint16
mono_type_get_generic_param_num (MonoType *t)
{
return mono_generic_param_num (t->data.generic_param);
return mono_generic_param_num (m_type_data_get_generic_param (t));
}

/*
Expand Down
164 changes: 82 additions & 82 deletions src/mono/mono/metadata/class.c
View file
Open in desktop

Large diffs are not rendered by default.

52 changes: 26 additions & 26 deletions src/mono/mono/metadata/marshal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1347,15 +1347,15 @@ mono_type_to_ldind (MonoType *type)
case MONO_TYPE_R8:
return CEE_LDIND_R8;
case MONO_TYPE_VALUETYPE:
if (m_class_is_enumtype (type->data.klass)) {
type = mono_class_enum_basetype_internal (type->data.klass);
if (m_class_is_enumtype (m_type_data_get_klass (type))) {
type = mono_class_enum_basetype_internal (m_type_data_get_klass (type));
goto handle_enum;
}
return CEE_LDOBJ;
case MONO_TYPE_TYPEDBYREF:
return CEE_LDOBJ;
case MONO_TYPE_GENERICINST:
type = m_class_get_byval_arg (type->data.generic_class->container_class);
type = m_class_get_byval_arg (m_type_data_get_generic_class (type)->container_class);
goto handle_enum;
default:
g_error ("unknown type 0x%02x in type_to_ldind", type->type);
Expand Down Expand Up @@ -1401,15 +1401,15 @@ mono_type_to_stind (MonoType *type)
case MONO_TYPE_R8:
return CEE_STIND_R8;
case MONO_TYPE_VALUETYPE:
if (m_class_is_enumtype (type->data.klass)) {
type = mono_class_enum_basetype_internal (type->data.klass);
if (m_class_is_enumtype (m_type_data_get_klass (type))) {
type = mono_class_enum_basetype_internal (m_type_data_get_klass (type));
goto handle_enum;
}
return CEE_STOBJ;
case MONO_TYPE_TYPEDBYREF:
return CEE_STOBJ;
case MONO_TYPE_GENERICINST:
type = m_class_get_byval_arg (type->data.generic_class->container_class);
type = m_class_get_byval_arg (m_type_data_get_generic_class (type)->container_class);
goto handle_enum;
default:
g_error ("unknown type 0x%02x in type_to_stind", type->type);
Expand Down Expand Up @@ -1602,7 +1602,7 @@ mono_marshal_need_free (MonoType *t, MonoMethodPInvoke *piinfo, MonoMarshalSpec
return TRUE;
case MONO_TYPE_OBJECT:
case MONO_TYPE_CLASS:
if (t->data.klass == mono_class_try_get_stringbuilder_class ()) {
if (m_type_data_get_klass (t) == mono_class_try_get_stringbuilder_class ()) {
gboolean need_free;
mono_marshal_get_ptr_to_stringbuilder_conv (piinfo, spec, &need_free);
return need_free;
Expand Down Expand Up @@ -2495,8 +2495,8 @@ get_runtime_invoke_type (MonoType *t, gboolean ret)
case MONO_TYPE_U:
return mono_get_int_type ();
case MONO_TYPE_VALUETYPE:
if (m_class_is_enumtype (t->data.klass)) {
t = mono_class_enum_basetype_internal (t->data.klass);
if (m_class_is_enumtype (m_type_data_get_klass (t))) {
t = mono_class_enum_basetype_internal (m_type_data_get_klass (t));
goto handle_enum;
}
return t;
Expand Down Expand Up @@ -5364,7 +5364,7 @@ mono_marshal_get_unsafe_accessor_wrapper (MonoMethod *accessor_method, MonoUnsaf
return res;
}
// printf ("Cache miss\n");

mb = mono_mb_new (accessor_method->klass, accessor_method->name, MONO_WRAPPER_OTHER);
if (generic_wrapper) {
// If the accessor method was generic, make the wrapper generic, too.
Expand Down Expand Up @@ -6325,7 +6325,7 @@ mono_marshal_asany_impl (MonoObjectHandle o, MonoMarshalNative string_encoding,
case MONO_TYPE_CLASS:
case MONO_TYPE_VALUETYPE: {

MonoClass *klass = t->data.klass;
MonoClass *klass = m_type_data_get_klass (t);

if (mono_class_is_auto_layout (klass))
break;
Expand All @@ -6349,7 +6349,7 @@ mono_marshal_asany_impl (MonoObjectHandle o, MonoMarshalNative string_encoding,
}
case MONO_TYPE_SZARRAY: {
//TODO: Implement structs and in-params for all value types
MonoClass *klass = t->data.klass;
MonoClass *klass = m_type_data_get_klass (t);
MonoClass *eklass = m_class_get_element_class (klass);
MonoArray *arr = (MonoArray *) MONO_HANDLE_RAW (o);

Expand Down Expand Up @@ -6412,7 +6412,7 @@ mono_marshal_free_asany_impl (MonoObjectHandle o, gpointer ptr, MonoMarshalNativ
break;
case MONO_TYPE_CLASS:
case MONO_TYPE_VALUETYPE: {
MonoClass *klass = t->data.klass;
MonoClass *klass = m_type_data_get_klass (t);

if (m_class_is_valuetype (klass) && (mono_class_is_explicit_layout (klass) || m_class_is_blittable (klass) || m_class_is_enumtype (klass)))
break;
Expand All @@ -6437,7 +6437,7 @@ mono_marshal_free_asany_impl (MonoObjectHandle o, gpointer ptr, MonoMarshalNativ
break;
}
case MONO_TYPE_SZARRAY: {
MonoClass *klass = t->data.klass;
MonoClass *klass = m_type_data_get_klass (t);
MonoClass *eklass = m_class_get_element_class (klass);
MonoArray *arr = (MonoArray *) MONO_HANDLE_RAW (o);

Expand Down Expand Up @@ -6751,7 +6751,7 @@ typedef enum {
SWIFT_DOUBLE,
} SwiftPhysicalLoweringKind;

static int get_swift_lowering_alignment (SwiftPhysicalLoweringKind kind)
static int get_swift_lowering_alignment (SwiftPhysicalLoweringKind kind)
{
switch (kind) {
case SWIFT_INT64:
Expand All @@ -6764,19 +6764,19 @@ static int get_swift_lowering_alignment (SwiftPhysicalLoweringKind kind)
}
}

static void set_lowering_range(guint8* lowered_bytes, guint32 offset, guint32 size, SwiftPhysicalLoweringKind kind)
static void set_lowering_range(guint8* lowered_bytes, guint32 offset, guint32 size, SwiftPhysicalLoweringKind kind)
{
bool force_opaque = false;

if (offset != ALIGN_TO(offset, get_swift_lowering_alignment(kind))) {
// If the start of the range is not aligned, we need to force the entire range to be opaque.
force_opaque = true;
}

// Check if any of the range is non-empty.
// If so, we need to force this range to be opaque
// and extend the range to the existing tag's range and mark as opaque in addition to the requested range.

for (guint32 i = 0; i < size; ++i) {
SwiftPhysicalLoweringKind current = (SwiftPhysicalLoweringKind)lowered_bytes[offset + i];
if (current != SWIFT_EMPTY && current != kind) {
Expand All @@ -6796,7 +6796,7 @@ static void set_lowering_range(guint8* lowered_bytes, guint32 offset, guint32 si

static void record_struct_field_physical_lowering (guint8* lowered_bytes, MonoType* type, guint32 offset);

static void record_inlinearray_struct_physical_lowering (guint8* lowered_bytes, MonoClass* klass, guint32 offset)
static void record_inlinearray_struct_physical_lowering (guint8* lowered_bytes, MonoClass* klass, guint32 offset)
{
int align;
int type_offset = MONO_ABI_SIZEOF (MonoObject);
Expand Down Expand Up @@ -6841,7 +6841,7 @@ static void record_struct_physical_lowering (guint8* lowered_bytes, MonoClass* k
}
}

static void record_struct_field_physical_lowering (guint8* lowered_bytes, MonoType* type, guint32 offset)
static void record_struct_field_physical_lowering (guint8* lowered_bytes, MonoType* type, guint32 offset)
{
int align;

Expand All @@ -6864,7 +6864,7 @@ static void record_struct_field_physical_lowering (guint8* lowered_bytes, MonoTy
else if (type->type == MONO_TYPE_PTR || type->type == MONO_TYPE_FNPTR
|| type->type == MONO_TYPE_I || type->type == MONO_TYPE_U) {
kind = SWIFT_INT64;
}
}
#endif
else if (type->type == MONO_TYPE_R4) {
kind = SWIFT_FLOAT;
Expand Down Expand Up @@ -6913,7 +6913,7 @@ mono_marshal_get_swift_physical_lowering (MonoType *type, gboolean native_layout
}

guint8 lowered_bytes[TARGET_SIZEOF_VOID_P * 4] = { 0 };

// Loop through all fields and get the physical lowering for each field
record_struct_physical_lowering(lowered_bytes, klass, 0);

Expand Down Expand Up @@ -6943,7 +6943,7 @@ mono_marshal_get_swift_physical_lowering (MonoType *type, gboolean native_layout
|| (i == ALIGN_TO(i, 8) && (current == SWIFT_DOUBLE || current == SWIFT_INT64))
// We've changed interval types
|| current != lowered_bytes[i - 1];

if (start_new_interval) {
struct _SwiftInterval interval = { i, 1, current };
g_array_append_val(intervals, interval);
Expand Down Expand Up @@ -6973,7 +6973,7 @@ mono_marshal_get_swift_physical_lowering (MonoType *type, gboolean native_layout
MonoType *lowered_types[4];
guint32 offsets[4];
guint32 num_lowered_types = 0;

for (int i = 0; i < intervals->len; ++i) {
if (num_lowered_types == 4) {
// We can't handle more than 4 fields
Expand All @@ -6983,7 +6983,7 @@ mono_marshal_get_swift_physical_lowering (MonoType *type, gboolean native_layout
}

struct _SwiftInterval interval = g_array_index(intervals, struct _SwiftInterval, i);

offsets[num_lowered_types] = interval.start;

switch (interval.kind) {
Expand Down
92 changes: 89 additions & 3 deletions src/mono/mono/metadata/metadata-internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ struct _MonoType {
MonoMethodSignature *method;
MonoGenericParam *generic_param; /* for VAR and MVAR */
MonoGenericClass *generic_class; /* for GENERICINST */
} data;
} data; /* don't access directly, use m_type_data_get_<name> */
unsigned int attrs : 16; /* param attributes or field flags */
MonoTypeEnum type : 8;
unsigned int has_cmods : 1;
Expand Down Expand Up @@ -1296,6 +1296,92 @@ m_type_is_byref (const MonoType *type)
return type->byref__;
}

MONO_NEVER_INLINE void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to have this in production than I believe we should adopt a similar pattern as we have when using MONO_CLASS_DEF_PRIVATE, either extend that or make a similar thing for MonoType, that way we would run all these additional checks when running ENABLE_CHECKED_BUILD_PRIVATE_TYPES (we should have CI legs doing that), but use inline wrappers with zero overhead on release builds.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to do actual full checks in Release unless benchmarks show that it meaningfully regresses performance, at least during the preview period, in order to flush out any remaining issues of this type

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still add the same support for types as we have for class so we could do a MONO_TYPE_DEF_PRIVATE under ENABLED_CHECKED_BUILD_PRIVATE_TYPES to catch errors bypassing the getters on CI. We could also add support for the full check vs zero overhead wrappers as part of that and then just decide to initially opt into full checks during preview (and monitor perf hit during that time) and then have the option to move back to zero wrappers closer to release, but keep the full checks on CI builds running with the ENABLED_CHECKED_BUILD_PRIVATE_TYPES.

m_type_invalid_access (const char *fn_name, MonoTypeEnum actual_type);

static inline MonoClass *
m_type_data_get_klass (const MonoType *type)
{
switch (type->type) {
// list based on class.c mono_class_from_mono_type_internal cases
case MONO_TYPE_OBJECT:
case MONO_TYPE_VOID:
case MONO_TYPE_BOOLEAN:
case MONO_TYPE_CHAR:
case MONO_TYPE_I1:
case MONO_TYPE_U1:
case MONO_TYPE_I2:
case MONO_TYPE_U2:
case MONO_TYPE_I4:
case MONO_TYPE_U4:
case MONO_TYPE_I:
case MONO_TYPE_U:
case MONO_TYPE_I8:
case MONO_TYPE_U8:
case MONO_TYPE_R4:
case MONO_TYPE_R8:
case MONO_TYPE_STRING:
case MONO_TYPE_TYPEDBYREF:
case MONO_TYPE_CLASS:
case MONO_TYPE_VALUETYPE:
case MONO_TYPE_SZARRAY:
return type->data.klass;
default:
m_type_invalid_access (__func__, type->type);
return NULL;
}
}

static inline MonoGenericParam *
m_type_data_get_generic_param (const MonoType *type)
{
if (G_LIKELY((type->type == MONO_TYPE_VAR) || (type->type == MONO_TYPE_MVAR)))
return type->data.generic_param;

m_type_invalid_access (__func__, type->type);
return NULL;
}

static inline MonoArrayType *
m_type_data_get_array (const MonoType *type)
{
if (G_LIKELY(type->type == MONO_TYPE_ARRAY))
return type->data.array;

m_type_invalid_access (__func__, type->type);
return NULL;
}

static inline MonoType *
m_type_data_get_type (const MonoType *type)
{
if (G_LIKELY(type->type == MONO_TYPE_PTR))
return type->data.type;

m_type_invalid_access (__func__, type->type);
return NULL;
}

static inline MonoMethodSignature *
m_type_data_get_method (const MonoType *type)
{
if (G_LIKELY(type->type == MONO_TYPE_FNPTR))
return type->data.method;

m_type_invalid_access (__func__, type->type);
return NULL;
}

static inline MonoGenericClass *
m_type_data_get_generic_class (const MonoType *type)
{
if (G_LIKELY(type->type == MONO_TYPE_GENERICINST))
return type->data.generic_class;

m_type_invalid_access (__func__, type->type);
return NULL;
}

/**
* mono_type_get_class_internal:
* \param type the \c MonoType operated on
Expand All @@ -1308,7 +1394,7 @@ static inline MonoClass*
mono_type_get_class_internal (MonoType *type)
{
/* FIXME: review the runtime users before adding the assert here */
return type->data.klass;
return m_type_data_get_klass (type);
}

/**
Expand All @@ -1322,7 +1408,7 @@ mono_type_get_class_internal (MonoType *type)
static inline MonoArrayType*
mono_type_get_array_type_internal (MonoType *type)
{
return type->data.array;
return m_type_data_get_array (type);
}

static inline int
Expand Down
Loading