Fix GC overreporting with generics context + GS cookie #50544
Merged
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
This is a case of a generics context in `this` where we need to keep `this` alive (but aren't required to by the VM) with a GS cookie check (due to "JitStress: STRESS_UNSAFE_BUFFER_CHECKS"), and maybe other JitStress modes, where `genEmitGSCookieCheck` decides to use `rcx` to load the cookie, even though it is alive. `this` is also mostly in a register, but it reported live as a stack slot. When the GS cookie label gets created, the "codegen" gc info hasn�t been updated with the killed register, so the emitter label gets the wrong GC info and brings rcx back alive just for the `nop`. GC cookie checks kind of do their own register allocation since they�re part of the epilogs. The GS cookie code checks for keep alive using "lvRegister", but that isn't set, since the cookie is mostly alive in a stack slock, not always in a register. Use `lvIsInReg()` instead, so GS cookie check will use RDX to load the cookie instead. Fixes dotnet#50404
|
@AndyAyersMS @dotnet/jit-contrib PTAL |
AndyAyersMS
approved these changes
Apr 1, 2021
|
@BruceForstall Does this warrant a Pri1 test? |
|
crossgen2smoke failures are #50539 No interop failures in gcstress-extra
Well, it already is a Pri1 test, just run in stress modes. We generally don't like to have gcstress tests in normal Pri-1 runs. What we really need is to more aggressively keep our stress modes (especially gcstress) clean. |
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.
This is a case of a generics context in
thiswhere we need to keepthisalive (but aren't required to by the VM) with a GS cookie check(due to "JitStress: STRESS_UNSAFE_BUFFER_CHECKS"), and maybe other
JitStress modes, where
genEmitGSCookieCheckdecides to usercxto load the cookie, even though it is alive.
thisis also mostlyin a register, but it reported live as a stack slot. When the GS
cookie label gets created, the "codegen" gc info hasn't been updated
with the killed register, so the emitter label gets the wrong GC info
and brings rcx back alive just for the
nop. GC cookie checks kindof do their own register allocation since they're part of the epilogs.
The GS cookie code checks for keep alive using "lvRegister", but that
isn't set, since the cookie is mostly alive in a stack slock, not
always in a register. Use
lvIsInReg()instead, so GS cookie checkwill use RDX to load the cookie instead.
No SuperPMI asm diffs.
Fixes #50404