JIT: handle adjacent no-GC regions (#115719)

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.

[AndyAyersMS]

Ensure the first instruction of each non-interruptible GC range is excluded from the range. This breaks up adjacent no-GC regions, where previously only the first instruction of the first range was excluded.

Fixes#115719.

[AndyAyersMS]

@EgorBo PTAL
fyi @dotnet/jit-contrib

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Updates CoreCLR JIT GC info encoding so that each IGF_NOGCINTERRUPT (non-interruptible / no-GC) region consistently has its first instruction treated as interruptible (except for prolog regions), even when two no-GC regions are adjacent. This improves the accuracy of interruptible range reporting used by the runtime/debugger.

Changes:

• Adjust InterruptibleRangeReporter to consider igOffs + firstInstrSize (non-prolog) when deciding whether to emit an interruptible range.
• This ensures an interruptible “gap” is emitted for the first instruction even when igOffs == m_uninterruptibleEnd, breaking up adjacent no-GC regions.

[jakobbotsch]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

[jakobbotsch]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

Fixes #115719.

What happened in the issue? We end up defining a 0-length interruptible GC range and that does something odd?

We fused together adjacent no-gc regions into one big region, and the user was unable to run funceval when broken in at the start of the second one (0x64), since it was considered no-gc.

;; interruptible ranges
Before: [0x27, 0x57) [0x76, 0x89) [0x96, 0x9E)
After: [0x27, 0x57) [0x64, 0x69) [0x76, 0x89) [0x96, 0x9E)

[AndyAyersMS]

GC stress failures are related.

[AndyAyersMS]

We can't distinguish between a single no-gc region broken up across IGs vs multiple no-gc regions that happen to be adjacent. Looking instead at adding an explicit NOP to clearly delineate the start of a second, separate no-gc region.

[tannergooding]

Is this one worth backporting?

[AndyAyersMS]

Is this one worth backporting?

It is fixing an impaired debugging experience. I don't know if that meets the servicing bar. The issue has been open a while now.

[tannergooding]

Personally I'd say its worth a bar check. Pretty sure I've hit this a few times and it would be nice to not have a degraded debug experience in the LTS

[AndyAyersMS]

Revising this to insert a NOP in lower instead, so that we have a clear gc safe point with appropriate debug info.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[AndyAyersMS]

GC stress failures are the tests fixed by #130974 and the known issue #130993

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
"version": 5,
"last_dispatched_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_reviewed_commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "49110af56a95223e88f9e540503c0f8a16e64e15",
"last_recorded_worker_run_id": "29688127987",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "ec4034b1140ce1658a1cc7e6cf7941cfd93af5ab",
"review_id": 4730810109
}
]
}

[github-actions]

Holistic Review

Motivation: Justified. Issue #115719 shows a real debugging failure where evaluation is disallowed because the debugger stops at an unsafe GC point in unoptimized code. The root cause is that adjacent no-GC regions get merged so only the very first instruction of the first region is excluded from no-GC, leaving stack-empty points between logical regions non-interruptible.

Approach: Sound. For debuggable codegen only, the change detects when a new no-GC region is being opened immediately after existing no-GC code at a stack-empty Normal IP mapping matching the current location, and inserts an interruptible INS_nop to break up the adjacent regions. It also introduces emitLastSavedIGWasNoGC tracking and renames the previously-unused emitGCDisabled() to emitLastCodeIsNoGC(), which correctly reports interruptibility of the current IG (if non-empty) or the last saved non-empty IG.

Summary: ✅ LGTM. The change is narrowly gated to compDbgCode, so codegen for optimized code is unaffected. Bookkeeping for emitLastSavedIGWasNoGC is updated consistently in emitSavIG (only for non-empty IGs), emitCreatePlaceholderIG (prolog/epilog placeholders, always no-GC), and initialized in emitBegFN. The renamed helper had no external callers, so the rename is safe. Already approved by two JIT area experts. No blocking issues found.

---

Detailed Findings

✅ Correctness — No-GC region bookkeeping is consistent

emitLastSavedIGWasNoGC is set from IGF_NOGCINTERRUPT only when the saved IG is non-empty (ig->igSize != 0), matching the emitCurIGsize != 0 guard in emitLastCodeIsNoGC. Placeholder IGs (prolog/epilog) are unconditionally marked no-GC, which is correct since those regions are non-interruptible. Initialization in emitBegFN prevents stale cross-method state.

✅ Scope — Debug-only, low risk

The NOP insertion is guarded by m_compiler->opts.compDbgCode, emitNoGCRequestCount == 0, a non-empty genIPmappings, and precise checks that the last mapping is a Normal, STACK_EMPTY, current-location mapping. This tightly limits the behavioral change to the intended debuggable scenario and avoids emitting spurious NOPs.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 62.3 AIC · ⌖ 14.7 AIC · ⊞ 10K

[AndyAyersMS]

@EgorBo@jakobbotsch reworked this, take another look.

GC stress in main has some known failures.

[JulieLeeMSFT]

LGTM.
In emitDisableGC(), conditions are tight and look correct.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

[AndyAyersMS]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

[jakobbotsch]

I'm worried about this case here:

runtime/src/coreclr/jit/codegenxarch.cpp

Lines 166 to 171 in 78fd36e

	// Because of the way the flowgraph is connected, the liveness info for this one instruction
	// after the call is not (can not be) correct in cases where a variable has a last use in the
	// handler. So turn off GC reporting once we execute the call and reenable after the jmp/nop
	GetEmitter()->emitDisableGC();
	GetEmitter()->emitIns_J(INS_call, block->GetTarget());

With this PR do we potentially end up allowing a GC in this position that has the wrong GC info described in the comment?

I don't think so? The concern there is that in a call finally; jmp continuation GC info can be wrong between the two instructions, so the jmp is a no-GC region.

That doesn't change with this PR.

Ah yes, I see that now

[AndyAyersMS]

/azp run runtime-coreclr gcstress0x3-gcstress0xc, runtime-coreclr gcstress-extra

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[AndyAyersMS]

Merged up and GC stress is all green.