[mono] Mono support for InlineArrayAttribute #83776
Conversation
…line-array-attribute
|
Inline array attribute tests have passed and the failure shouldn't be related. |
|
/cc @mangod9 |
|
@VSadov too. |
There was a problem hiding this comment.
LGTM.
The most important things to address:
- we should make sure that
[InlineArray(-1)]fails for the right reasons (because we correctly detect and disallow negative values) not because it wraps around to a huge unsigned value. - the callback that extracts the value shoudl be named more specifically for InlineArrayAttribute - I don't think it's a good idea to accidentally reuse it for another attribute that might be defined with more constructors, arguments or properties.
src/mono/mono/metadata/class-init.c
Outdated
| size *= m_class_inlinearray_value (klass); | ||
| inlined_fields++; | ||
| if(size <= 0 || size > struct_max_size) { | ||
| mono_class_set_type_load_failure (klass, "Inline array struct size out of bounds."); |
There was a problem hiding this comment.
Is this an error that we share with coreclr? Should we mention the maximum size?
There was a problem hiding this comment.
Should we mention the maximum size?
I am not sure the limit is very informative to the user.
The size is computed from element size and will be impacted by aligning and padding. The limit was rather arbitrarily chosen as there would be some limit anyways.
1 MiB seems to be enough for most uses. It can be increased if there is a need for more, but likely this error will be seen only in erroneous cases.
We basically want to say "Hey, the array is abnormally large. Is everything alright with the code?"
There was a problem hiding this comment.
It might be valuable for users to provide more information on the error, either maximum size or indication that the struct is abnormally large. Would it be wrong to mention maximum size as it could be impacted by aligning and padding?
src/mono/mono/metadata/object.c
Outdated
| @@ -931,6 +932,15 @@ compute_class_bitmap (MonoClass *klass, gsize *bitmap, int size, int offset, int | |||
| g_error ("compute_class_bitmap: Invalid type %x for field %s:%s\n", type->type, mono_type_get_full_name (m_field_get_parent (field)), field->name); | |||
| break; | |||
| } | |||
|
|
|||
| // If a struct has inline array attribute, consider it as an array | |||
There was a problem hiding this comment.
I am not familiar with how GC/pointer maps work in Mono. The rest of the change looks good to me.
There was a problem hiding this comment.
A struct with the inline array attribute is handled in the same way as an array.
...braries/System.Runtime.InteropServices.JavaScript/gen/JSImportGenerator/JSManagedTypeInfo.cs
Outdated
Show resolved
Hide resolved
|
Failures shouldn't be related. |
ghost
locked as resolved and limited conversation to collaborators
This PR Implements support for
InlineArrayAtributeas outlined in #61135 and tracked in #80798.It implements structure layout, updates GC class bitmap to consider the struct field as an array, disallows marshalling structs with the attribute in
JS Interop generatorand adds a corresponding test.