[release/7.0] [mono][s390x] Fix wrong implementation of OP_CHECK_THIS #76920
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
* Only access a single byte in memory for OP_CHECK_THIS * Remove unnecessary ltgr instruction * Fixes #76915
github-actions
bot
requested review from
vargaz,
lambdageek and
SamMonoRT
as code owners
akoeplinger
approved these changes
Oct 12, 2022
vargaz
approved these changes
Oct 12, 2022
|
@akoeplinger please send the email to Tactics requesting approval if you haven't done so. @mmitche Do we still have runway to take this for GA? |
2 tasks
|
This isn't critical for 7.0 GA, we can take it as part of normal servicing. |
|
@akoeplinger can you please take a look at the CI failures? Let me know if this is good to merge (pending Tactics approval, of course). |
|
The failures are unrelated. |
|
Approved only if this can be merged by 10/13/2022 EOD |
rbhanda
added
Servicing-approved
|
CI failures investigated as unrelated. Approved by Tactics. Signed off. Ready to merge. |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #76916 to release/7.0
/cc @akoeplinger @uweigand
Customer Impact
There is a Mono codegen bug on
s390x.The accesses that span the page boundary turn out to be emitted from the implementation of the
OP_CHECK_THISopcode. This is intended to perform a memory access, just to see if this triggers a segmentation fault if the incoming pointer was invalid. However, theOP_CHECK_THISimplementation currently always emits an 8-byte memory access - potentially resulting in a false positive if the pointer was valid, but only to access less than 8 bytes.This caused crashes in the
SpanHelpers.IndexOfValueTypebefore the code was reverted to an earlier version, but it could be hit by user code as well.We don't need it for 7.0 GA release as it only triggers in certain cases. If we can't make the 7.0 GA, please include in the first 7.x servicing release.
Testing
Manual testing.
Risk
Very low, only affects s390x architecture.
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.