-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[release/8.0] JIT: Disallow mismatched GC-ness for physical promotions #90739
Conversation
Physical promotion was working under the assumption that reinterpreting GC pointers is undefined behavior, and would happily promote GC pointers as integers if it saw such accesses. However, physical promotion is function wide while the UB accesses can be happening in a restricted (dynamically unreachable) scope. This exact situation happens in MemoryExtensions.Contains. The issue was uncovered under jit stress where we did not fold away the guard early enough, meaning that promotion then saw a `TYP_LONG` access of a `struct { object, int }` and proceeded to promote it as such. Fix #90602
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsBackport of #90694 to release/8.0 /cc @jakobbotsch Customer ImpactTestingRiskIMPORTANT: If this backport is for a servicing release, please verify that:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved. please get a code review, you are good to merge
FYI this is going into RC2 (release/8.0). If that's your intention, @jakobbotsch , then I can merge right away, since it has been signed-off and approved by @jeffschwMSFT . If you intended to send it to RC1, please retarget the PR to release/8.0-rc1 and send an email to Tactics requesting approval. |
I think RC2 only is just fine. |
Backport of #90694 to release/8.0
/cc @jakobbotsch
Customer Impact
Code that involves illegal reinterpretations of GC pointers in unreachable code paths can cause the JIT to generate code with incorrect GC reporting, even in the reachable paths.
For example, MemoryMarshal.Contains is a case. When
value
is a GC pointer,RuntimeHelpers.IsBitwiseEquatable<T>()
will return false, but under some stress scenarios the JIT will still see the laterUnsafe.As<T, long>(ref value)
. This can cause it to mistakenly reinterpretvalue
as a long without any GC reporting, even outside theRuntimeHelpers.IsBitwiseEquatable<T>()
branch.For this particular case, in normal circumstances (no stress modes) the unreachable code is folded away early enough that the JIT does not see the reinterpretation, so no issue happens.
Testing
Verified that the failing test case now generates correct code and GC reporting, even when the stress variables are set.
Risk
Low. Targeted fix with no diffs in any of our own code (under non stress circumstances) that disables physical promotion when the case is detected.