-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix optImpliedByTypeOfAssertions to correctly find a non-null assertion #120917
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
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
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.
Pull Request Overview
This PR fixes a bug in the JIT compiler's assertion propagation logic where optImpliedByTypeOfAssertions was incorrectly identifying non-null assertions. When searching for an implied obj != null assertion after finding obj->pMT == <cns_type>, the code was incorrectly accepting any obj != <constant> assertion instead of specifically verifying that the right-hand side was null.
Key Changes:
- Fixed the assertion validation logic to use
CanPropNonNull()to verify the assertion is actuallyobj != null - Simplified duplicate-checking logic using
TryAddElemD - Added regression test to prevent future occurrences
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/jit/assertionprop.cpp | Fixed bug by replacing manual operand checks with CanPropNonNull() call and simplified duplicate detection |
| src/tests/JIT/Regression/JitBlue/Runtime_120792/Runtime_120792.cs | Added regression test that triggers the bug scenario with type assertions and null checks |
| src/tests/JIT/Regression/JitBlue/Runtime_120792/Runtime_120792.csproj | Test project configuration with tiered compilation and PGO enabled |
|
cc @dotnet/jit-contrib |
|
/backport to release/10.0-staging |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/18679209211 |
|
Started backporting to release/10.0-staging: https://github.com/dotnet/runtime/actions/runs/18679216580 |
|
@EgorBo backporting to "release/8.0-staging" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: fix optImpliedByTypeOfAssertions to correctly find non-null assertion
Using index info to reconstruct a base tree...
M src/coreclr/jit/assertionprop.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/assertionprop.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/assertionprop.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 fix optImpliedByTypeOfAssertions to correctly find non-null assertion
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@EgorBo an error occurred while backporting to "release/10.0-staging", please check the run log for details! Error: The specified backport target branch "release/10.0-staging" wasn't found in the repo. |
Fixes #120792
This looks like some very old bug that was exposed by an unrelated change in .NET 10.0
Basically, we have an assertion
obj->pMT == <cns_type>we invokeoptImpliedByTypeOfAssertionsto see if we can find some existing assertion (anywhere) forobj != nullso we can duplicate it into currentASSERT_TP& activeAssertions, becauseobj->pMTassertion implies obj is not null.We do find such an assertion but for some reason we don't check that it's null on the right side (op2) -> obj != null. Instead, we pick up some random
obj != <frozen_object>assertion which then is incorrectly used.No diffs.