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.

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

[mono] Use InlineArrayAttribute for ArgumentData and StackAllocatedByRefs #88149

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

lambdageek
Copy link
Member

Mono supports InlineArray

@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: -

@jkotas
Copy link
Member

jkotas commented Jun 28, 2023

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

lambdageek and others added 2 commits June 28, 2023 13:44
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!

@lambdageek lambdageek merged commit dd04e01 into dotnet:main Jun 29, 2023
160 of 165 checks passed
@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