Don't hoist potentially invalid byrefs#124833
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical GC crash issue where if-conversion was speculatively hoisting potentially invalid managed references (byrefs). The issue occurred when if-conversion transformed ternary operations involving null references (like Unsafe.NullRef<byte>()) into unconditionally evaluated code, leading to GC crashes.
Changes:
- Added
gtTreeMayHaveInvalidByrefsmethod to detect trees with potentially invalid byrefs - Integrated the check into if-conversion logic to skip conversion when invalid byrefs are detected
- The fix prevents speculative hoisting of GC-typed nodes that could be invalid
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/ifconversion.cpp | Added check to skip if-conversion when potentially invalid byrefs are detected in then or else branches |
| src/coreclr/jit/gentree.cpp | Implemented gtTreeMayHaveInvalidByrefs method with tree visitor to detect potentially invalid GC-typed nodes |
| src/coreclr/jit/compiler.h | Added declaration for the new static method gtTreeMayHaveInvalidByrefs |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if (Compiler::gtTreeMayHaveInvalidByrefs(m_compiler, m_thenOperation.node) || | ||
| (m_doElseConversion && Compiler::gtTreeMayHaveInvalidByrefs(m_compiler, m_elseOperation.node))) | ||
| { | ||
| JITDUMP("Skipping if-conversion that may have invalid byrefs in the then or else block\n"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The invalid-byref guard only inspects the original then/else statement trees. In the no-else case, if-conversion synthesizes a new GT_LCL_VAR read of the destination local (see later selectTrueInput = gtNewLclVarNode(...)), which can introduce an unconditionally-evaluated byref that wasn’t present before. Consider bailing out when !m_doElseConversion and the destination local is TYP_BYREF, or run the invalid-byref check on the final select inputs (including the synthesized one) before proceeding.
1 participant
Fixes #124807