Uh oh!
There was an error while loading. Please reload this page.
JIT: protect delegate invoke with a NoGC region - #133077
Conversation
|
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 |
|
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 behavioral JIT fix lacks regression coverage for the GC-liveness invariant.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes a JIT GC-liveness issue by keeping collectible delegates alive through invocation.
Changes:
- Exposes the delegate node during lowering.
- Inserts
GT_KEEPALIVEbefore fully interruptible delegate calls.
File summaries
| File | Review |
|---|---|
src/coreclr/jit/lower.h | Updates the lowering signature. |
src/coreclr/jit/lower.cpp | Extends delegate GC lifetime through invocation. A focused regression test is required. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Uh oh!
There was an error while loading. Please reload this page.
72a038f to
7518178CompareThere was a problem hiding this comment.
🟡 Changes recommended
The critical delegate-target coverage gap must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/coreclr/jit/lower.cpp:6550
- This GC-correctness change has no regression test even though the linked issue provides a compact repro and the failure can be exercised by making the caller fully interruptible under GC stress. Please add a JIT regression test using a collectible delegate so CI protects the target-load/call boundary and the excluded CFG/tailcall variants.
BlockRange().InsertBefore(call, new (m_compiler, GT_START_NONGC) GenTree(GT_START_NONGC, TYP_VOID));
call->gtCallMoreFlags |= GTF_CALL_M_ENDS_NOGC_REGION;
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| // would put its validation call inside the region, and for a tailcall via helper the target | ||
| // ends up in an argument rather than in the control expression. | ||
| // | ||
| if (m_compiler->GetInterruptible() && !m_compiler->opts.IsCFGEnabled() && !call->IsTailCallViaJitHelper()) |
Fixesdotnet#105082 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7518178 to
b2c6ce8Compare
Fixes#105082.
G_M4108_IG04: ; bbWeight=1, gcrefRegs=0004 {x2}, byrefRegs=0000 {}, byref mov w1, w0 + ;; size=4 bbWeight=1 PerfScore 0.50+G_M4108_IG05: ; bbWeight=1, nogc, extend ldp x0, x2, [x2, #0x10] ; gcrRegs -[x2] +[x0] blr x2 // code for System.Action`1[int]:Invoke(int):this ; gcrRegs -[x0] + ;; size=8 bbWeight=1 PerfScore 5.00+G_M4108_IG06: ; bbWeight=1, extend+ nop