fix: SCCP bug with null checks in object traversal loops

[charsleysa]

SCCP Loop Back-Edge Fix

Issue Summary

The SparseConditionalConstantPropagationStage was incorrectly removing null checks and return statements in methods containing while-loops that traverse linked objects, causing infinite loops and test failures.

Affected Tests (Before Fix)

Three unit tests in WhileTests.cs were failing with identical symptoms:

• WhileTests::WhileObjectTraversalNullCheck() - Expected: 6, Result: NULL (CRASHED)
• WhileTests::WhileObjectTraversalLongChain() - Expected: True, Result: NULL (CRASHED)
• WhileTests::WhileObjectTraversalSum() - Expected: 100, Result: NULL (CRASHED)

All tests involved the same pattern:

var current = head;
while (current != null)  // ← This null check was being removed!
{
    // ... process current ...
    current = current.Next;
}
return result;  // ← This return was being removed as "unreachable"!

Root Cause

The Bug

In the Phi() method of SparseConditionalConstantPropagation.cs, when analyzing phi nodes in loops:

1. Initial Analysis: The phi node v42 <= v51, v36 (L_0002B, L_00000) has two sources:

   • L_00000 (entry block): executable, contributes v36 (NewObject, not null)
   • L_0002B (loop back-edge): not yet marked executable during first iteration

2. Incorrect Assumption: SCCP only saw the executable predecessor with a non-null value (v36) and concluded the phi result (v42) is always not null

3. Aggressive Optimization: Based on this incorrect conclusion:

   • Null check IR.BranchObject [!=] v42, null was removed as "always false"
   • Branch to epilogue was eliminated as "dead code"
   • Unconditional jump to loop body created infinite loop

IR Transformation (Before Fix)

BEFORE SCCP (Correct):
L_0003D:
    IR.PhiObject    v42 <= v51, v36 (L_0002B, L_00000)
    IR.Phi32        v43 <= v48, 0 (L_0002B, L_00000)
    IR.BranchObject [!=] L_0002B v42, null  // ← Null check
    IR.Jmp          L_FFFFF                 // ← Exit to epilogue

L_FFFFF:
    IR.SetReturn32  v43
    IR.Epilogue

AFTER SCCP (Bug - Infinite Loop):
L_0003D:
    IR.PhiObject    v42 <= v51, v36 (L_0002B, L_00000)
    IR.Phi32        v43 <= v48, 0 (L_0002B, L_00000)
    IR.Jmp          L_0002B  // ← Unconditional jump (null check removed!)

L_FFFFF:
    // EMPTY - Epilogue removed as "unreachable"!

The Fix

Solution

Modified the Phi() method to be conservative when analyzing phi nodes with unanalyzed predecessors (loop back-edges):

private void Phi(Node node)
{
    // ... setup code ...
    
    // Check if any source blocks haven't been analyzed yet (back-edges in loops)
    var hasUnanalyzedPredecessors = false;
    var analyzedCount = 0;

    for (var index = 0; index < currentBlock.PreviousBlocks.Count; index++)
    {
        var predecessor = sourceBlocks[index];
        var executable = blockStates[predecessor.Sequence];
        
        if (executable)
            analyzedCount++;
        else
            hasUnanalyzedPredecessors = true;
    }

    // CRITICAL FIX: If we have unanalyzed predecessors (back-edges in loops),
    // and we're dealing with object references, mark as OverDefined to prevent
    // incorrect null-check elimination
    if (hasUnanalyzedPredecessors && analyzedCount > 0 && result.IsReferenceType)
    {
        // We have a partial view - some blocks analyzed, some not (loop back-edge case)
        // Mark both value and reference state as OverDefined to be conservative
        UpdateToOverDefined(result);
        SetReferenceOverdefined(result);
        return;
    }
    
    // ... rest of phi processing ...
}

Key Changes

1. Detect Partial Analysis: Track when some predecessors are analyzed but others aren't (loop back-edge scenario)

2. Conservative Handling: When a phi node has:

   • At least one analyzed predecessor
   • At least one unanalyzed predecessor (back-edge)
   • Result is a reference type (object pointer)

   → Mark as OverDefined to prevent assumptions about null state

3. Preserve Correctness: This ensures:

   • Null checks are preserved in loops
   • Exit conditions remain functional
   • Return/epilogue blocks are not eliminated

Results

After Fix

Unit Test Results:
  Passed:     89867  (↑ +7 from 89860)
  Skipped:    2616
  Incomplete: 0
  Failures:   0      (↓ -3 from 3)
  Total:      92483

All unit tests passed successfully!

Previously Failing Tests Now Pass

• ✅ WhileTests::WhileObjectTraversalNullCheck() - Returns: 6
• ✅ WhileTests::WhileObjectTraversalLongChain() - Returns: True
• ✅ WhileTests::WhileObjectTraversalSum() - Returns: 100

Additional Tests Added

To provide comprehensive coverage of the bug pattern, additional edge-case tests were added:

• ✅ WhileTests::WhileObjectTraversalCount() - Returns: 5
• ✅ WhileTests::WhileObjectTraversalWithModification() - Returns: True
• ✅ WhileTests::WhileObjectNullCheckAtStart() - Returns: 0
• ✅ WhileTests::WhileObjectSingleNode() - Returns: 42

Issue Category

• Component: Compiler optimization (SCCP stage)
• Pattern: Loop analysis with phi nodes and object references
• Severity: Critical (causes infinite loops)
• Scope: Any while-loop traversing object references with null checks

Testing

The fix was validated by:

1. Building successfully (no compilation errors)
2. Running full unit test suite (92,483 tests)
3. Verifying previously failing tests now pass
4. Confirming no regressions in other tests

Test Coverage

All tests in Mosa.UnitTests/FlowControl/WhileTests.cs that target the SCCP bug pattern:

1. WhileObjectTraversalNullCheck() - Simple 3-node linked list traversal with null check
2. WhileObjectTraversalCount() - Count nodes in a 5-node chain
3. WhileObjectTraversalLongChain() - Traverse 10-node chain and verify last value
4. WhileObjectTraversalSum() - Sum values from 4-node chain (10+20+30+40=100)
5. WhileObjectTraversalWithModification() - Modify values during traversal
6. WhileObjectNullCheckAtStart() - Edge case: null list
7. WhileObjectSingleNode() - Edge case: single-node list

These tests specifically target the SCCP bug pattern:

• Object reference traversal in while loops
• Null checks as loop exit conditions
• Phi nodes with loop back-edges
• Reference field access (.Next)

All tests pass, confirming the fix prevents the bug from recurring.

Files Modified

• Mosa.Compiler.Framework/Analysis/SparseConditionalConstantPropagation.cs

  • Modified: Phi() method (lines ~1060-1125)
  • Added: Conservative handling for phi nodes with unanalyzed loop back-edges

• Mosa.UnitTests/FlowControl/WhileTests.cs

  • Added: LinkedNode helper class
  • Added: 7 test methods targeting object traversal patterns

Technical Notes

SCCP Algorithm

Sparse Conditional Constant Propagation is an optimizing compiler transformation that:

• Tracks constant values through the program
• Eliminates dead code based on constant conditions
• Requires proper handling of loops and phi nodes

Phi Nodes in SSA

In Static Single Assignment (SSA) form:

• Phi nodes merge values from different control flow paths
• Loop headers have phi nodes for loop variables
• Back-edges (loop iteration) create cyclic dependencies
• Analysis must handle partial information correctly

Conservative vs. Aggressive Optimization

• Aggressive: Assume best case (led to the bug)
• Conservative: Assume unknown when uncertain (correct approach for soundness)

The fix applies conservative analysis to maintain correctness while still enabling optimizations where safe.

[Copilot]

Pull request overview

This PR fixes a critical bug in the Sparse Conditional Constant Propagation (SCCP) optimization stage where null checks in while-loops traversing linked objects were incorrectly eliminated, causing infinite loops and test crashes.

Key Changes:

• Added conservative handling for phi nodes with unanalyzed loop back-edges in SCCP to prevent premature null-check elimination
• Added 7 comprehensive unit tests targeting the specific bug pattern: object reference traversal in while loops with null checks
• Fixed spelling of method name from "Continuiously" to "Continuously"

Reviewed changes

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

File	Description
Source/Mosa.Compiler.Framework/Analysis/SparseConditionalConstantPropagation.cs	Added logic to detect phi nodes with unanalyzed predecessors and mark them as OverDefined for reference types; corrected method name spelling
Source/Mosa.UnitTests/FlowControl/WhileTests.cs	Added LinkedNode helper class and 7 regression tests covering various linked-list traversal scenarios

Comments suppressed due to low confidence (1)

Source/Mosa.Compiler.Framework/Analysis/SparseConditionalConstantPropagation.cs:1

• The first loop (lines 1404-1423) contains duplicate logic that can be simplified. The executable variable is checked at line 1410 with if (!executable) continue;, making the subsequent conditional at lines 1415-1422 unreachable for the else branch. The counting logic should be consolidated into a single pass without the early continue.

// Copyright (c) MOSA Project. Licensed under the New BSD License.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This second loop (starting at line 1436) duplicates the iteration logic from the first loop (line 1404). Consider refactoring to iterate once and collect both the analysis counts and perform the subsequent processing in the same loop to reduce code duplication.

[tgiphil]

I’m still reviewing the conservative logic for SCCP. From what I can tell, the special handling for reference types probably isn’t needed, and it might not yield the optimal disposition state of the variable.