Skip to content

[WIP] Fix GC hole in JIT due to if-conversion - #133905

Draft
EgorBo with Copilot wants to merge 4 commits into
mainfrom
copilot/fix-gc-hole-issue
Draft

EgorBo with Copilot wants to merge 4 commits into
mainfrom
copilot/fix-gc-hole-issue

Conversation

Copilot AI commented Sep 14, 2026

Copy link
Copy Markdown
Contributor
  • Inspect explicit-init dependencies and complete the baseline build.
  • Remove explicit-init flags and dependent logic; add focused initialization coverage.
  • Build, run targeted tests, and check codegen impact: host/cross-target Debug JIT builds and six Linux x64 test runners passed; the normal regression wrapper fails on baseline and passes with the fix. Focused reference initialization adds 2 instructions / 6 bytes; existing large-struct zeroing codegen is unchanged. The crashing GC-stress repro was not run.
  • Complete formatting, code review, and security validation: clang-format 17 and secret scans passed; manual review feedback addressed. Full jit-format and the automated review tool were unavailable; CodeQL analysis was skipped because of database-size limits.
  • Resolves JIT: (bug) GC hole. If-conversion sinks the explicit init of an untracked GC local past a safepoint, leaving an uninitialized reported GC slot #133754

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
16 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@github-actions github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 14, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copilot AI and others added 2 commits September 14, 2026 19:30
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
@EgorBo

EgorBo commented Sep 14, 2026

Copy link
Copy Markdown
Member

@copilot remove changes to StructWithGC_Zeroing tests, no tests needed.

Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>

Copilot AI commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@copilot remove changes to StructWithGC_Zeroing tests, no tests needed.

Reverted both test files to their pre-PR versions in 735995d. The JIT fix is unchanged.

@EgorBo

EgorBo commented Sep 15, 2026

Copy link
Copy Markdown
Member

@jakobbotsch @AndyAyersMS @dotnet/jit-contrib so this PR removes hasExplicitInit entirely (with the flag) - see PR changes with "ignore whitespaces" mode.

Diffs - not that bad for non-tests collections, but still a regression. @jakobbotsch pointed out that the current change is not correct - it assumes at the SSA liveness stage we know exactly what parts of code are no-gc areas which is not correct, so presumably there might be a scenario when we have a GC local, it's not zeroed yet and we hit an unexpected gc safe point (e.g. fully interruptible part) - it will crash. Should we take it as is given it's a correctness fix?

I prototyped a fix that does the same analysis at the codegen phase that recovers the regressions, but it was a bit too complex for this PR IMO.

@EgorBo

EgorBo commented Sep 15, 2026

Copy link
Copy Markdown
Member

It seems that regression happen moslty in small methods where the existing opt managed to avoid any zeroing in prolog, e.g.

nuint Test(ref int value)
{
    fixed (int* p = &value)
        return (nuint)p & 1;
}

Main:

; Method WriteBarrierBenchmarks:Test(byref):nuint (FullOpts)
G_M49291_IG01:
       push     rax
G_M49291_IG02:
       mov      bword ptr [rsp], rcx
       mov      rax, rcx
       and      rax, 1
G_M49291_IG03:
       add      rsp, 8
       ret      
; Total bytes of code: 17

PR:

; Method WriteBarrierBenchmarks:Test(byref):nuint (FullOpts)
G_M49291_IG01:
       push     rax
       xor      eax, eax
       mov      qword ptr [rsp], rax
G_M49291_IG02:
       mov      bword ptr [rsp], rcx
       mov      rax, rcx
       and      rax, 1
G_M49291_IG03:
       add      rsp, 8
       ret      
; Total bytes of code: 23

(in the real world, it's likely inlined to somewhere where we already zero locals with SIMD anyway)

@AndyAyersMS

Copy link
Copy Markdown
Member

I'm a bit worried about the potential impact of this change. Wondering if we should consider something more surgical.

Is this a regression in .NET 11?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JIT: (bug) GC hole. If-conversion sinks the explicit init of an untracked GC local past a safepoint, leaving an uninitialized reported GC slot

3 participants