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

Jump to bottom

Mono: handle structs larger than 256 bytes correctly #58190

Closed
akoeplinger opened this issue Aug 26, 2021 · 4 comments · Fixed by #58978
Closed

Mono: handle structs larger than 256 bytes correctly #58190

akoeplinger opened this issue Aug 26, 2021 · 4 comments · Fixed by #58978
Assignees
@lambdageek
Labels
area-Codegen-JIT-mono
Milestone
6.0.0

Comments

@akoeplinger
Copy link
Member

akoeplinger commented Aug 26, 2021 •
edited
Loading

We hardcode the size of structs to 256 bytes here:

runtime/src/mono/mono/mini/mini-runtime.c

Line 3400 in 3f61d42

guint8 retval [256];

and here:

runtime/src/mono/mono/mini/mini-runtime.c

Line 3160 in 3f61d42

guint8 retval [256];

This causes an out of bounds check to abort the app when running the System.Text.Json tests on iOS arm64 devices because of this large struct: https://github.com/dotnet/runtime/blob/7b19ccefccb4d116a64bf09c9bb1db3dd1df35e8/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.SimpleTestStruct.cs

Note: we probably should also port #52501 to mono_jit_runtime_invoke.
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Infrastructure-mono untriaged New issue has not been triaged by the area owner labels Aug 26, 2021
@ghost
Copy link

ghost commented Aug 26, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

We hardcode the size of structs to 256 bytes here:

runtime/src/mono/mono/mini/mini-runtime.c

Line 3400 in 3f61d42

guint8 retval [256];

and here:

runtime/src/mono/mono/mini/mini-runtime.c

Line 3160 in 3f61d42

guint8 retval [256];

This causes an out of bounds check to abort the app when running the System.Text.Json tests on iOS arm64 devices.

Note: we probably should also port #52501 to mono_jit_runtime_invoke.

Author: akoeplinger
Assignees: -
Labels:

untriaged, area-Infrastructure-mono
Milestone: -

@ghost ghost added this to Untriaged in Infrastructure Backlog Aug 26, 2021
@ghost ghost removed this from Untriaged in Infrastructure Backlog Aug 26, 2021
@lambdageek lambdageek self-assigned this Aug 26, 2021
@lambdageek lambdageek added this to the 6.0.0 milestone Aug 26, 2021
@lambdageek lambdageek removed the untriaged New issue has not been triaged by the area owner label Aug 26, 2021
@lambdageek
Copy link
Member

lambdageek commented Aug 26, 2021 •
edited
Loading

Probably if the struct is bigger than out bugger we should not use the "dyn call" path, but go to a dedicated trampoline That turns out to be a can of worms.

Instead we should just malloc a buffer. PR shortly.

@lambdageek lambdageek mentioned this issue Aug 26, 2021
Merged
lambdageek added a commit that referenced this issue Aug 30, 2021
@lambdageek
[mini] Dynamically allocate a buffer for large runtime invoke results (…
b6cc822 
…#58215)

* [mini] Dynamically allocate a buffer for large runtime invoke results

   If the return type is a struct that's bigger than our buffer, malloc a buffer for it instead of using a fixed-size stack buffer

   Also take the ref-return logic from #52501 and add it to the non-LLVM runtime-invoke

   This makes the `System.Runtime` testsuite (particularly `InvokeRefReturnNetcoreTests`) pass on M1 MacCatalyst FullAOT.

  Related to #58190

* Throw nullbyrefreturn exception for non-LLVM dyn invoke

   Fixes various tests in InvokeRefReturnNetcoreTests
@SamMonoRT
Copy link
Member

@lambdageek - we can close this right ? Fixed in main and backport also merged

@lambdageek
Copy link
Member

Yes, that's right.

lambdageek added a commit to lambdageek/runtime that referenced this issue Sep 11, 2021
@lambdageek
[mini] Use alloca for runtime_invoke retval buffer
670dbf6 
Fixes dotnet#58957
Related to dotnet#58215 which was attempting to
fix dotnet#58190
@lambdageek lambdageek mentioned this issue Sep 11, 2021
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 11, 2021
github-actions bot pushed a commit that referenced this issue Sep 12, 2021
@lambdageek
[mini] Use alloca for runtime_invoke retval buffer
8856d3d 
Fixes #58957
Related to #58215 which was attempting to
fix #58190
@lambdageek lambdageek mentioned this issue Sep 12, 2021
Merged
lambdageek added a commit that referenced this issue Sep 12, 2021
@lambdageek
[mini] Use alloca for runtime_invoke retval buffer (#58978)
206dba7 
* [mini] Use alloca for runtime_invoke retval buffer

Fixes #58957
Related to #58215 which was attempting to
fix #58190

* Set initial return buffer size to TARGET_SIZEOF_VOID_P

In the common case we use the return buffer just to hold a pointer to
the return value
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 12, 2021
Anipik pushed a commit that referenced this issue Sep 13, 2021
@github-actions @lambdageek
[release/6.0] [mini] Use alloca for runtime_invoke retval buffer (#59006
8fe648d 
)

* [mini] Use alloca for runtime_invoke retval buffer

Fixes #58957
Related to #58215 which was attempting to
fix #58190

* Set initial return buffer size to TARGET_SIZEOF_VOID_P

In the common case we use the return buffer just to hold a pointer to
the return value

Co-authored-by: Aleksey Kliger <alklig@microsoft.com>
Co-authored-by: Aleksey Kliger <aleksey@lambdageek.org>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@lambdageek lambdageek

Labels
area-Codegen-JIT-mono
Projects
None yet
Milestone
6.0.0
Development

Successfully merging a pull request may close this issue.

[mini] Use alloca for runtime_invoke retval buffer lambdageek/runtime
3 participants
@lambdageek @akoeplinger @SamMonoRT