Skip to content

Conversation

@lambdageek
Copy link
Member

Mono supports InlineArray

@ghost ghost assigned lambdageek Jun 28, 2023
@lambdageek
Copy link
Member Author

FYI @kotlarmilos

@ghost
Copy link

ghost commented Jun 28, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Mono supports InlineArray

Author: lambdageek
Assignees: -
Labels:

area-System.Reflection

Milestone: -

@lambdageek lambdageek requested a review from steveharter June 28, 2023 16:03
@jkotas
Copy link
Member

jkotas commented Jun 28, 2023

There is one more src\libraries\System.Private.CoreLib\src\System\ParamsArray.cs

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@uweigand
Copy link
Contributor

uweigand commented Jul 4, 2023

Hi @lambdageek, this seems to have caused a severe regression on s390x: all CI tests are currently failing with this error:

error: Could not set up field '_args' due to: Generic type definition failed, due to: Inline array struct size out of bounds, abnormally large.

I haven't looked into the root cause yet - it looks like this is failing only on s390x, not ppc64le, so maybe an endian issue?

@uweigand
Copy link
Contributor

uweigand commented Jul 4, 2023

I see the following suspicious code in has_inline_array_attribute_value_func:

                attr->value = *(gpointer*)decoded_attr->typed_args [0]->value.primitive;

and later

                        klass->inlinearray_value = GPOINTER_TO_INT32 (attr.value);

If this is actually a 4-byte value in memory, then the *(gpointer*) is broken on big-endian.

@uweigand
Copy link
Contributor

uweigand commented Jul 4, 2023

And indeed the following patch fixes the problem for me:

diff --git a/src/mono/mono/metadata/class-init.c b/src/mono/mono/metadata/class-init.c
index c41ea7b891d..ba5954e7963 100644
--- a/src/mono/mono/metadata/class-init.c
+++ b/src/mono/mono/metadata/class-init.c
@@ -816,7 +816,7 @@ has_inline_array_attribute_value_func (MonoImage *image, uint32_t method_token,
                MonoDecodeCustomAttr *decoded_attr = mono_reflection_create_custom_attr_data_args_noalloc (image, ctor, (guchar*)data, data_size, &error);
                mono_error_assert_ok (&error);
                g_assert (decoded_attr->named_args_num == 0 && decoded_attr->typed_args_num == 1);
-               attr->value = *(gpointer*)decoded_attr->typed_args [0]->value.primitive;
+               attr->value = (gpointer)*(guint32*)decoded_attr->typed_args [0]->value.primitive;
                g_free (decoded_attr);
        } else {
                g_warning ("Can't find custom attr constructor image: %s mtoken: 0x%08x due to: %s", image->name, method_token, mono_error_get_message (&error));

Does this look reasonable to you?

@lambdageek
Copy link
Member Author

Maybe we should use one of our READ32 macros (sorry, I'm on a phone, can't look up the exact name)

@uweigand
Copy link
Contributor

uweigand commented Jul 4, 2023

Looks like READ32 is only available/used inside the interpreter (src/mono/mini/interp), not in the rest of Mono.

@uweigand
Copy link
Contributor

uweigand commented Jul 5, 2023

I've now opened a PR including this fix: #88417

@ghost ghost locked as resolved and limited conversation to collaborators Aug 4, 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.

3 participants