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

[Ppc64le] Added memory patch thunking for function calls #73616

Merged
merged 8 commits into from
Aug 12, 2022

Conversation

alhad-deshpande
Copy link
Contributor

@alhad-deshpande alhad-deshpande commented Aug 9, 2022

This PR implements memory patch thunking for function calls atomically. This fixes sporadic crashes due to code being patched non-atomically including crashes observed in System.Text.RegularExpressions.Tests. This is similar to what was implemeted for s390x in PR 52783

@dotnet-issue-labeler
Copy link

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

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 9, 2022
@alhad-deshpande alhad-deshpande marked this pull request as draft August 9, 2022 11:11
@alhad-deshpande alhad-deshpande marked this pull request as ready for review August 9, 2022 12:48
@alhad-deshpande alhad-deshpande marked this pull request as draft August 9, 2022 15:20
@alhad-deshpande
Copy link
Contributor Author

alhad-deshpande commented Aug 10, 2022

@vargaz
The PR #73616 raised for memory patch thunking fixes crashes observed in System.Text.RegularExpressions.Tests due to code being patched non-atomically.

The assertion mentioned in issue 71080 - "Assertion at /home/directhex/Projects/runtime/src/mono/mono/mini/mini-ppc.c:5080, condition `ppc_is_imm16 (inst->inst_offset)' not met" is observed only in Release mode which we are looking at.

Also, there are new test cases added in RegexPcreTests.cs file which are also failing on Power and hence looking into it as well.

We are targeting this PR for RC1 release. Can you please review it?

@Sapana-Khemkar
Copy link
Contributor

@vargaz can you please review this PR? We are targeting it for RC1 which is planned on 12th August .

@akoeplinger
Copy link
Member

/azp run runtime-community

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@akoeplinger
Copy link
Member

@vargaz is on vacation right now, I've triggered a CI run to see if the test is fixed and we can merge the PR and he can review when he is back.

@alhad-deshpande
Copy link
Contributor Author

@vargaz is on vacation right now, I've triggered a CI run to see if the test is fixed and we can merge the PR and he can review when he is back.

@akoeplinger
The System.Text.RegularExpressions.Tests is not fully fixed as mentioned above. But this PR fixes a large part of it where sporadic crashes occurs due to code being patched non-atomically. This has been fixed by memory patch thunking for function calls atomically. Please merge this PR as such crashes can occur anytime during code patching especially in multithreading environment and this PR fixes the same. We are targeting this PR for RC1.

@akoeplinger
Copy link
Member

Ok

@akoeplinger akoeplinger merged commit 80aeaa0 into dotnet:main Aug 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-ppc64le area-Codegen-JIT-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants