Fix SIP (sweep-in-plan) plan_allocated corruption in process_remaining_regions

[cshung]

Fix SIP (sweep-in-plan) plan_allocated corruption in process_remaining_regions

Problem

With the regions GC, setting DOTNET_GCEnableSpecialRegions=1 trips a _DEBUG GC assert shortly after startup (SIGABRT in a Checked build). Two different asserts can fire depending on heap layout:

• fix_generation_bounds — plan_allocated == mem
• verify_regions — heap_segment_allocated <= heap_segment_reserved (FATAL_GC_ERROR)

Both trace back to a single root cause in process_remaining_regions.

Root cause

When the allocation region for a generation is itself swept-in-plan (SIP), heap_segment_non_sip() skips it and returns a different, empty region. The region chain is not address-ordered, so the returned region may be at a higher or lower address than the skipped SIP region.

The code then assigned that region's plan_allocated directly from the consing pointer:

heap_segment_plan_allocated (current_region) = generation_allocation_pointer (consing_gen);

But the consing pointer belongs to the skipped SIP region, not current_region, so it lies outside current_region's [mem, reserved] range:

• Below mem → the region can later become gen0's start region, tripping fix_generation_bounds and corrupting gen0 bounds.
• Above reserved → find_first_valid_region copies plan_allocated into allocated, making allocated > reserved and tripping verify_regions.

Fix

A region reached by skipping a SIP allocation region is empty regardless of address direction, so its plan_allocated is simply its mem. Clamp plan_allocated to mem whenever the consing pointer falls outside [mem, reserved]:

uint8_t* plan_alloc = generation_allocation_pointer (consing_gen);
if ((plan_alloc < heap_segment_mem (current_region)) ||
    (plan_alloc > heap_segment_reserved (current_region)))
{
    plan_alloc = heap_segment_mem (current_region);
}
heap_segment_plan_allocated (current_region) = plan_alloc;

The clamp only triggers on the SIP-skip path (consing pointer out of range), so the default path is unchanged.

Validation

Checked CLR, linux-x64.

• Functional: 56/56 GC tests that pass by default (GC/API, GC/Scenarios, GC/Coverage) also pass with DOTNET_GCEnableSpecialRegions=1 after the fix. No regressions with the switch off.
• Stress: DOTNET_GCStress=0xC and 0xF (the latter with a reduced gen0 size to force frequent compacting GCs) across allocation-heavy scenarios (linked-list, jagged-array, server-model, large-object). Results identical with the switch on and off.

[azure-pipelines]

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

[Copilot]

Pull request overview

This PR adjusts regions-GC planning in gc_heap::process_remaining_regions to prevent heap_segment_plan_allocated from being set to a pointer that falls outside the selected non-SIP region after SIP skipping, avoiding downstream state corruption and debug asserts.

Changes:

• Replace direct assignment of heap_segment_plan_allocated(current_region) from the consing allocation pointer with a guarded/clamped assignment.
• When the consing allocation pointer is outside [heap_segment_mem(current_region), heap_segment_reserved(current_region)], treat the region as empty and set plan_allocated to mem instead.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "56e2bc856fa13f22e6bac1d3151cfa690c14e146",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_reviewed_commit": "56e2bc856fa13f22e6bac1d3151cfa690c14e146",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "13c03a4a9a854291f77636b1e15038e65919e68b",
  "last_recorded_worker_run_id": "29689445229",
  "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": "56e2bc856fa13f22e6bac1d3151cfa690c14e146",
      "review_id": 4730870106
    }
  ]
}

[github-actions]

Holistic Review

Motivation: With regions GC and DOTNET_GCEnableSpecialRegions=1, a _DEBUG assert fires shortly after startup (fix_generation_bounds's plan_allocated == mem, or verify_regions's heap_segment_allocated <= heap_segment_reserved). The root cause is real and well-explained: in process_remaining_regions, when the generation's allocation region is itself swept-in-plan (SIP), heap_segment_non_sip() skips it and returns a different, empty region. Because the region chain is not address-ordered, that returned region may sit at a higher or lower address than the skipped SIP region. The pre-existing code assigned heap_segment_plan_allocated(current_region) = generation_allocation_pointer(consing_gen), but the consing pointer belongs to the skipped region, so it can fall below mem or above reserved of current_region, corrupting gen0 bounds or making allocated > reserved.

Approach: The fix clamps plan_allocated to heap_segment_mem(current_region) whenever the consing pointer lies outside [mem, reserved]. The reasoning is sound: a region reached only by skipping a SIP allocation region is empty, so mem is the correct plan_allocated. I verified the default (non-skip) path is unaffected: when no SIP region is skipped, current_region is the allocation segment itself and generation_allocation_pointer(consing_gen) is guaranteed to lie within [mem, committed] ⊆ [mem, reserved], so the clamp never triggers — matching the author's claim. The out-of-range check correctly handles both the below-mem and above-reserved directions described in the PR. The change is minimal (12/1), local, and gated behind an off-by-default switch, keeping blast radius small.

Summary: This is a correct, minimal, and well-documented fix for a genuine SIP-region bug, with a clear comment explaining the invariant and validation across functional and GCStress runs. The default code path is provably unchanged. I have no blocking concerns.

Detailed Findings

No actionable issues found.

💡 suggestion (optional, non-blocking): The out-of-range address check is an effective proxy for "a SIP region was skipped," but it detects the condition indirectly. Since the original allocation segment is still available at line 2961 (generation_allocation_segment(consing_gen)) before the heap_segment_non_sip reassignment at line 2975, an alternative would be to capture it and branch on current_region != <original> to make the SIP-skip intent explicit. The current range-based approach is equally correct and arguably more robust (it also guards against any future path that leaves the pointer out of range), so this is purely a readability preference — feel free to keep as-is.

Note

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

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