-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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][interp] Fix handing of native types #61612
Conversation
Use a single g_print in bulk for every instruction. Otherwise, the instruction ends up being displayed on multiple lines.
These structures are valuetypes, but their mint_type is a primitive. This means that a LDFLD applied on the type would have attempted to do a pointer dereference, because it saw that the current top of stack is not STACK_TYPE_VT. This was fixed in the past by passing the managed pointer to the valuetype rather than the valuetype itself, against the normal call convention, which lead to inconsistencies in the code. This commit removes that hack and fixes the problem by ignoring LDFLD applied to nint/nfloat valuetypes in the first place.
Tagging subscribers to this area: @BrzVlad Issue DetailsFixes #61153
|
@marek-safar @rolfbjarne @lambdageek My understanding was that we used these native types to compensate for a lack of support within the C# language. However, if I'm not mistaken these types were added recently in the language and they are implemented differently. Do we still need these long term ? Seems like iOS is still using them. @rolfbjarne Is there any way to test my change on iOS. I'm not sure if we have tests for this functionality in dotnet/runtime |
@BrzVlad we'll need the support for our There are some tests here, not sure if those could apply to this scenario: |
Wasn't able to test this on 32bit so I just submitted this change to mono/mono for some additional test running : mono/mono#21317 |
@BrzVlad please correct me if I'm wrong here --- but all validation is complete now. the change was ported to mono\mono locally to run a few more tests, and it's all green. |
Fixes #61153