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

Constructing MD arrays in R2R code does not work on OSX/ARM64 #62747

Closed
VSadov opened this issue Dec 13, 2021 · 7 comments · Fixed by #62855
Closed

Constructing MD arrays in R2R code does not work on OSX/ARM64 #62747

VSadov opened this issue Dec 13, 2021 · 7 comments · Fixed by #62855
Labels
area-ReadyToRun-coreclr bug untriaged New issue has not been triaged by the area owner
Milestone

Comments

@VSadov
Copy link
Member

VSadov commented Dec 13, 2021

#61938 exposed this issue.

We had other issues that could cause related test scenario (TestGenericMDArrayBehavior) to never run in R2R mode on OSX.
#61938 makes R2R enabled more reliably on OSX, but now we see the test failing, so I disabled the scenario as a part of #61938.

I beleive the root cause is variadic JIT helper - HCIMPL2VA(Object*, JIT_NewMDArr, CORINFO_CLASS_HANDLE classHnd, unsigned dwNumArgs) .
It looks like OSX/ARM64 has special ABI for variadic calls, but we call it just as a cdecl with extra arguments. As a result the helper reads garbage and allocating MD arrays fails.

We do not use this helper in regular JIT code. There is a comment that it may be worth switching R2R to non-variadic helper as well since it would be more portable. Perhaps it is time?

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Dec 13, 2021
@VSadov
Copy link
Member Author

VSadov commented Dec 13, 2021

@dotnet/jit-contrib - is this a JIT issue?

@filipnavara
Copy link
Member

For reference, the general marshalling of varargs is tracked in #48796 with specific discussion about the macOS/iOS ABI situation.

@jkoritzinsky
Copy link
Member

I think this is a JIT issue.

It looks like there's two JIT helpers for creating MDArrays. One that uses varargs and one that doesn't.

For some reason it looks like the one that doesn't use varargs is only used in non-R2R or CoreRT scenarios:

Usage of the non-varag helper:

node = gtNewHelperCallNode(CORINFO_HELP_NEW_MDARR_NONVARARG, TYP_REF, args);

Condition for the block containing the non-vararg helper call setup:

if (!opts.IsReadyToRun() || IsTargetAbi(CORINFO_CORERT_ABI))

Usage of the vararg helper (in the else block for the above if condition):

node = gtNewHelperCallNode(CORINFO_HELP_NEW_MDARR, TYP_REF, args);

The comment above the if block already says that the vararg helper is deprecated and should be removed in the future and that all usages should use the non-vararg method, but the non-vararg variant is only used in CoreRT for now as it requires an R2R version bump. As we're already bumping the R2R version with #61759, I think it would be okay to switch things over, but I'd want @jkotas to chime in here as well.

@VSadov
Copy link
Member Author

VSadov commented Dec 13, 2021

Especially if we are already bumping R2R version.
Definitely a good time to get rid of a deprecated helper with poor portability.

@jkotas
Copy link
Member

jkotas commented Dec 14, 2021

Especially if we are already bumping R2R version.
Definitely a good time to get rid of a deprecated helper with poor portability.

+100

@jkoritzinsky
Copy link
Member

We're using a multidimensional array in ConsolePal.Unix.cs, so I think this is higher priority than we thought:

private static readonly string[,] s_fgbgAndColorStrings = new string[2, 16]; // 2 == fg vs bg, 16 == ConsoleColor values

As a result, this is breaking all of the crossgen2 outerloop runs on macOS ARM64. I'll take a stab at fixing this to unblock the legs.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 15, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ReadyToRun-coreclr bug untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants