Uh oh!
There was an error while loading. Please reload this page.
Fix discarding of NRE's when unboxing nullables in Mono and JIT - #133351
Fix discarding of NRE's when unboxing nullables in Mono and JIT#133351jakobbotsch wants to merge 3 commits into
Conversation
Retain throwing roots when their VN exception sets are not covered by their operands. Fold unused GetType operations to null checks through shared side-effect extraction, and add a targeted nullable GetType regression test. Fixesdotnet#133207. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9c8af88-4173-4ead-99f9-50375606ef4a
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Uh oh!
There was an error while loading. Please reload this page.
Restore the Mono interpreter GetType fix without duplicating the JitBlue regression. Remove debug destruction of roots that assertion propagation may retain for side effects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9c8af88-4173-4ead-99f9-50375606ef4a
jakobbotsch
commented
Sep 8, 2026
/azp run runtime-coreclr superpmi-diffs |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
jakobbotsch
commented
Sep 8, 2026
/ba-g Failure is #130439 |
jakobbotsch
commented
Sep 8, 2026
cc @dotnet/jit-contrib PTAL @EgorBo |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
The new exception-set subset logic unions operand VNs without guarding against undefined VN pairs, which can trigger debug asserts/crashes in some loop/PHI scenarios.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a JIT and Mono interpreter optimization bug where VN-based folding (notably GetType folding on boxed nullables) could drop required faulting behavior, leading to missing NullReferenceExceptions. It also adds a targeted JIT regression test to prevent recurrence.
Changes:
- Update JIT VN-based constant folding to retain “throwing roots” when their exception sets aren’t covered by operands, and improve side-effect extraction so unused
GetTypecan be reduced to a null-check. - Reapply the Mono interpreter fix to avoid optimizing
box Nullable<T>; call GetType()into a constant type lookup. - Add a regression test for
Nullable<T>.GetType()behavior (throws on null; returns underlying type when non-null).
File summaries
| File | Description |
|---|---|
| src/coreclr/jit/assertionprop.cpp | Adjust VN-based folding to decide when to preserve root side effects based on exception-set coverage. |
| src/coreclr/jit/gentree.cpp | Add helper to turn unused GetType into a NULLCHECK via shared side-effect extraction. |
| src/coreclr/jit/compiler.h | Declare the new side-effect extraction helper on Compiler. |
| src/mono/mono/mini/interp/transform.c | Disable the box; GetType optimization for nullable types in the interpreter. |
| src/tests/JIT/Regression/JitBlue/Runtime_133207/Runtime_133207.cs | Add xUnit regression coverage for nullable GetType throwing/returning behavior. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Include the new regression test in the JIT regression project. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes JIT optimization and side-effect extraction behavior in broadly-used code paths, so it warrants final maintainer review despite the added regression coverage.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
VN-based constant folding did not preserve exceptions from the root node. For
GetTypefolding, that results in discarding an explicit nullcheck.Retain throwing roots when their VN exception sets are not covered by their operands. Fold unused GetType operations to null checks through shared side-effect extraction, and add a targeted nullable GetType regression test.
Mono had a similar issue for nullables only, which was fixed in #132732 but later reverted. This PR includes that fix.
Fix#133207