diff --git a/docs/changes/2026-06-09-evm-ssa-lift-phi-crashes/README.md b/docs/changes/2026-06-09-evm-ssa-lift-phi-crashes/README.md new file mode 100644 index 000000000..f2fd6af82 --- /dev/null +++ b/docs/changes/2026-06-09-evm-ssa-lift-phi-crashes/README.md @@ -0,0 +1,115 @@ +# Change: Fix phi-materialization crashes in the EVM SSA stack-lift path + +- **Status**: Implemented +- **Date**: 2026-06-09 +- **Tier**: Light + +## Overview + +The EVM multipass JIT has an optional SSA stack-lifting path, gated by the +build flag `ZEN_ENABLE_EVM_STACK_SSA_LIFT` (default **OFF**, introduced in +#395, fed by the range analyzer from #493). When that flag is enabled the +JIT emitted invalid MIR that the verifier rejects and that aborts the +compiler (SIGABRT) on common real-world control flow. This change fixes +three independent defects in that path. All edits are reachable only when a +block is lifted, so the default (flag-off) build is unaffected. + +## Motivation + +With `ZEN_ENABLE_EVM_STACK_SSA_LIFT=ON`, compiling ordinary contracts trips +the MIR verifier's phi invariants and then aborts: + +1. **Phi contiguity.** `createPendingPhi` appended each phi at the block end. + A stack merge with two or more slots emits, per slot, four phi components + followed by four temp-store `dassign`s; a later slot's phis therefore + landed *after* an earlier slot's `dassign`s, violating the + "phis contiguous at block start" invariant + (`mir/pass/verifier.h`). Multi-slot merge blocks — e.g. any loop header + carrying more than one stack value — aborted. + +2. **Dynamic-jump-target phi undersize.** Codegen's indirect-jump dispatch + wires an edge from every block that performs a computed `JUMP`/`JUMPI` to + every `JUMPDEST`. The analyzer's shape-class predecessor enumeration + (`getPotentialEntryPredecessorsForBlock`), which sizes a merge block's + phis, is a strict subset of that set. So a dynamic-jump-target merge block + received phis with fewer incoming slots than its actual MIR predecessor + count; a later pass indexing phi incomings by predecessor then ran out of + bounds (silent SIGABRT in a release build). + +3. **Loop back-edge phi incoming block.** A loop header's merge phi froze its + back-edge incoming *block* before the `JUMPI` terminator wired the real CFG + edge into the header. The forward walk that resolves the incoming block + could not yet reach the true predecessor and recorded the source EVM + block's *entry* MIR basic block instead of the basic block that actually + branches back. The verifier rejected the phi + ("phi incoming block must be a predecessor of the current block"). + +These are latent correctness/robustness bugs in merged code. Fixing them is +worthwhile independent of whether the feature is enabled. + +## Approach + +1. **Phi contiguity** — `createPendingPhi` (evm_mir_compiler.cpp): create the + phi without appending, scan the block's leading run of phis, and insert the + new phi immediately after it. All phis stay grouped at the block start + regardless of how many slots or interleaved `dassign`s precede. + +2. **Dynamic-jump-target exclusion** — `finalizeLiftability` (evm_analyzer.h): + a new coverage check + (`dynamicJumpTargetPredecessorsCoverCodegenEdges`) sets + `CanLiftStack = false` for any dynamic-jump-target candidate whose static + predecessor enumeration does not account for every dispatch source codegen + will wire. Such blocks fall back to the existing memory-backed stack path — + the correct conservative behavior: never lift a block whose phi predecessor + set cannot be modeled exactly. + +3. **Deferred incoming-block re-resolution** — + `finalizeStackMergePhiIncomingBlocks` (evm_mir_compiler.cpp), called at the + end of `finalizeEVMBase` once the whole CFG exists: for each stack-merge phi, + any incoming block that is not yet a real predecessor of the owning block is + re-resolved by a forward walk to the actual predecessor. Only the block + pointer is corrected; the incoming **value** is preserved. + +The fixes touch only phi *placement* and *predecessor bookkeeping*; no +arithmetic lowering or value semantics change. + +## Impact + +- **Modules**: `src/compiler/evm_frontend/` only (`evm_mir_compiler.{h,cpp}`, + `evm_analyzer.h`). +- **Default build (flag OFF)**: unaffected. `createPendingPhi`, + `finalizeStackMergePhiIncomingBlocks`, and the new analyzer code are + reachable only on the lift path; with the flag off the bookkeeping vector + stays empty and the lift-only paths are never entered. +- **Lift build (flag ON)**: the three abort/verifier failures above no longer + occur on the validated workloads. + +## Validation + +- **Default-path neutrality** (build rebuilt from this change, flag OFF): + - multipass `evmone-unittests`: **223/223** + - multipass `evmone-statetest -k fork_Cancun` (EEST `state_tests`): + **2723/2723** +- **Lift path** (flag ON): + - multipass `evmone-unittests`: **223/223** (was 222/223 before fix 3; + `memory_grow_mstore8` now passes) + - 228-fixture mainnet-replay corpus: JIT-compiles crash-free, verifier-clean + (was SIGABRT before fixes 1–2) +- `tools/format.sh check`: passes. + +## Known limitation (disclosed) + +This change does **not** make SSA stack-lifting production-ready, and the flag +stays **OFF**. With the flag ON, at least one further crash remains: the EEST +`stEIP1153_transientStorage/transStorageOK` state test still aborts under +SSA-lift (confirmed in isolation; passes with the flag off). That defect is +independent of the three fixed here and will be addressed separately. CI does +not build with `ZEN_ENABLE_EVM_STACK_SSA_LIFT=ON`, so it does not exercise this +path; the lift-path results above were produced manually. + +## Checklist + +- [x] Implementation complete +- [x] Default-path tests pass (unittests 223/223, statetest 2723/2723) +- [x] Build and format checks pass +- [ ] SSA-lift made fully correct (tracked separately — see Known limitation) diff --git a/src/compiler/evm_frontend/evm_analyzer.h b/src/compiler/evm_frontend/evm_analyzer.h index c8748cf97..55c6e609f 100644 --- a/src/compiler/evm_frontend/evm_analyzer.h +++ b/src/compiler/evm_frontend/evm_analyzer.h @@ -15,6 +15,7 @@ #include #include #include +#include #include #include @@ -1090,6 +1091,56 @@ class EVMAnalyzer { return SourceBlockPCs; } + // Codegen's indirect-jump dispatch wires an edge from every block that emits + // a dynamic (computed) JUMP/JUMPI to every JUMPDEST in the global jump-dest + // table (see implementIndirectJump / getOrCreateIndirectJumpBB). The set of + // such dispatch source blocks is therefore independent of the analyzer's + // shape-class partitioning. Enumerate it so the lifter can size phis to match + // the edges codegen actually wires. + std::vector collectAllDynamicJumpDispatchSourceBlocks() const { + std::vector SourceBlockPCs; + for (const auto &[EntryPC, Info] : BlockInfos) { + // Codegen emits the indirect-jump dispatch for every block whose JUMP / + // JUMPI target is non-constant (Info.HasDynamicJump), independent of + // whether the analyzer could resolve that block's entry stack depth. The + // depth filter used elsewhere would drop sources codegen still wires, so + // it must not be applied here. + if (!Info.HasDynamicJump) { + continue; + } + appendUniqueBlockPC(SourceBlockPCs, EntryPC); + } + return SourceBlockPCs; + } + + // A dynamic-jump-target block is only safe to lift when the static + // predecessor enumeration that sizes its merge phis + // (getPotentialEntryPredecessorsForBlock) already accounts for every + // dynamic-jump dispatch source codegen will wire. Otherwise the phi is + // undersized relative to the block's actual MIR predecessor count and the + // verifier/regalloc path breaks. + // DispatchSources is the codegen dispatch-source set, computed once by the + // caller (collectAllDynamicJumpDispatchSourceBlocks) and reused across blocks + // to avoid rescanning all BlockInfos per block. + bool dynamicJumpTargetPredecessorsCoverCodegenEdges( + uint64_t BlockPC, const std::vector &DispatchSources) const { + auto It = BlockInfos.find(BlockPC); + if (It == BlockInfos.end() || !It->second.IsDynamicJumpTargetCandidate) { + return true; + } + + const std::vector EnumeratedSources = + collectDynamicJumpSourceBlocksForInfo(It->second); + const std::unordered_set EnumeratedSet(EnumeratedSources.begin(), + EnumeratedSources.end()); + for (uint64_t DispatchSourcePC : DispatchSources) { + if (EnumeratedSet.find(DispatchSourcePC) == EnumeratedSet.end()) { + return false; + } + } + return true; + } + bool getUniformDynamicJumpEntryDepthForRegion(uint64_t RegionEntryPC, int32_t &EntryDepth) const { bool SawDynamicJump = false; @@ -1321,6 +1372,10 @@ class EVMAnalyzer { } void finalizeLiftability() { + // Computed once and reused across blocks; independent of the per-block + // loop. + const std::vector DispatchSources = + collectAllDynamicJumpDispatchSourceBlocks(); for (auto &[EntryPC, Info] : BlockInfos) { (void)EntryPC; bool EntryKnown = Info.IsEntryStateCompatible; @@ -1344,6 +1399,16 @@ class EVMAnalyzer { getPotentialEntryPredecessorsForBlock(EntryPC).size() > 4) { Info.CanLiftStack = false; } + // Never lift a dynamic-jump-target block whose statically enumerated + // predecessor set (which sizes its stack-merge phis) cannot account for + // every dynamic-jump dispatch edge codegen wires. The shape-class source + // enumeration is a strict subset of codegen's "all JUMPDESTs reachable + // from every dynamic jump" wiring, so such a block's phi would be + // undersized relative to its actual MIR predecessor count. + if (Info.CanLiftStack && !dynamicJumpTargetPredecessorsCoverCodegenEdges( + EntryPC, DispatchSources)) { + Info.CanLiftStack = false; + } } } diff --git a/src/compiler/evm_frontend/evm_mir_compiler.cpp b/src/compiler/evm_frontend/evm_mir_compiler.cpp index 73c522f0f..adeb5ee65 100644 --- a/src/compiler/evm_frontend/evm_mir_compiler.cpp +++ b/src/compiler/evm_frontend/evm_mir_compiler.cpp @@ -165,7 +165,21 @@ MBasicBlock *EVMMirBuilder::resolveReachablePhiIncomingPredecessorBB( return CandidateBB; } - MBasicBlock *TargetBB = TargetIt->second; + return resolveReachablePredecessorBB(TargetIt->second, CandidateBB); +} + +// Resolve CandidateBB to a real predecessor of TargetBB. If CandidateBB is +// already a predecessor it is returned unchanged; otherwise the CFG is walked +// forward from CandidateBB to the first reachable block that is a predecessor +// of TargetBB. Returns CandidateBB when no such block is found, so callers can +// detect failure by checking predecessor membership of the result. +MBasicBlock * +EVMMirBuilder::resolveReachablePredecessorBB(MBasicBlock *TargetBB, + MBasicBlock *CandidateBB) const { + if (TargetBB == nullptr || CandidateBB == nullptr) { + return CandidateBB; + } + auto PredRange = TargetBB->predecessors(); if (std::find(PredRange.begin(), PredRange.end(), CandidateBB) != PredRange.end()) { @@ -196,6 +210,50 @@ MBasicBlock *EVMMirBuilder::resolveReachablePhiIncomingPredecessorBB( return CandidateBB; } +void EVMMirBuilder::finalizeStackMergePhiIncomingBlocks() { + // Stack-merge phi incoming blocks are resolved eagerly at the time each + // predecessor edge's stack state is assigned. For a loop back-edge that + // assignment runs before the predecessor block's terminator wires the real + // CFG edge into the loop header, so the recorded incoming block can be the + // predecessor EVM block's entry MIR block instead of the MIR block that + // actually branches into the loop header. Now that the full CFG is built, + // walk forward from each recorded incoming block to the real predecessor of + // the phi's owning block. Only the incoming-block pointer is corrected; the + // incoming value is preserved. + for (const auto &[Phi, OwnerBB] : StackMergePhiBlocks) { + if (Phi == nullptr || OwnerBB == nullptr) { + continue; + } + auto PredRange = OwnerBB->predecessors(); + for (size_t Index = 0; Index < Phi->getNumIncoming(); ++Index) { + MBasicBlock *IncomingBB = Phi->getIncomingBlock(Index); + if (IncomingBB == nullptr) { + continue; + } + // Already a real predecessor: nothing to fix. + if (std::find(PredRange.begin(), PredRange.end(), IncomingBB) != + PredRange.end()) { + continue; + } + // Walk forward from the recorded block to the actual predecessor of the + // phi's owning block, reusing the shared reachability resolver. + MBasicBlock *ResolvedBB = + resolveReachablePredecessorBB(OwnerBB, IncomingBB); + // The walk must land on a real predecessor; otherwise the phi would keep + // an incoming block that is not a predecessor and the verifier would + // reject it. Make that failure explicit in debug builds. + ZEN_ASSERT(std::find(PredRange.begin(), PredRange.end(), ResolvedBB) != + PredRange.end() && + "stack-merge phi incoming block did not resolve to a real " + "predecessor"); + if (ResolvedBB != IncomingBB) { + // Only the incoming-block pointer changes; the value is preserved. + Phi->setIncomingBlock(Index, ResolvedBB); + } + } + } +} + void EVMMirBuilder::loadEVMInstanceAttr() { InstanceAddr = createInstruction( false, OP_ptrtoint, &Ctx.I64Type, @@ -501,6 +559,10 @@ void EVMMirBuilder::finalizeEVMBase() { CurFunc->deleteMBasicBlock(ReturnBB); ReturnBB = nullptr; } + + // Correct loop back-edge merge phi incoming blocks against the now-complete + // CFG. No-op when no stack-merge phis were built (e.g. SSA stack-lift off). + finalizeStackMergePhiIncomingBlocks(); } LoadInstruction *EVMMirBuilder::getInstanceElement(MType *ValueType, @@ -1096,6 +1158,10 @@ typename EVMMirBuilder::Operand EVMMirBuilder::materializeStackMergeOperand( IncomingComponents[ComponentIndex]); } PhiComponents[ComponentIndex] = Phi; + // Record the phi against its loop-header block so its incoming blocks can + // be re-resolved once the full CFG (including back-edge terminators) is + // built. See finalizeStackMergePhiIncomingBlocks(). + StackMergePhiBlocks.emplace_back(Phi, CurBB); } for (size_t ComponentIndex = 0; ComponentIndex < EVM_ELEMENTS_COUNT; @@ -5234,7 +5300,24 @@ MInstruction *EVMMirBuilder::loadVariable(Variable *Var) { PhiInstruction *EVMMirBuilder::createPendingPhi(MType *Type, size_t NumIncoming) { - return createInstruction(true, Type, NumIncoming); + // Create the phi without appending it to the block end. Phi instructions + // must be contiguous at the block start (verified by the MIR verifier). + // When merging multiple stack slots, each slot emits phis followed by + // non-phi temp-store dassigns; appending at the end would interleave a + // later slot's phis after an earlier slot's dassigns and break the + // phi-contiguity invariant. Insert the phi right after the existing leading + // phis instead so all phis stay grouped at the front. + PhiInstruction *Phi = + createInstruction(false, Type, NumIncoming); + size_t InsertIdx = 0; + for (MInstruction *Inst : *CurBB) { + if (Inst->getKind() != MInstruction::PHI) { + break; + } + ++InsertIdx; + } + CurBB->addStatement(InsertIdx, Phi); + return Phi; } size_t EVMMirBuilder::getPhiIncomingSlot(PhiInstruction *Phi, diff --git a/src/compiler/evm_frontend/evm_mir_compiler.h b/src/compiler/evm_frontend/evm_mir_compiler.h index da6938610..7a0dffca5 100644 --- a/src/compiler/evm_frontend/evm_mir_compiler.h +++ b/src/compiler/evm_frontend/evm_mir_compiler.h @@ -1152,6 +1152,8 @@ class EVMMirBuilder final { MBasicBlock * resolveReachablePhiIncomingPredecessorBB(uint64_t TargetBlockPC, MBasicBlock *CandidateBB) const; + MBasicBlock *resolveReachablePredecessorBB(MBasicBlock *TargetBB, + MBasicBlock *CandidateBB) const; CompilerContext &Ctx; MFunction *CurFunc = nullptr; @@ -1202,6 +1204,17 @@ class EVMMirBuilder final { std::map> PhiIncomingSlotMap; std::map StackMergePhiVarMap; + // Stack-merge phis and the loop-header block they belong to. A merge phi's + // incoming block is resolved eagerly when each predecessor edge's stack + // state is assigned (materializeStackMergeOperand / assignStackMergeOperand). + // For a loop back-edge this assignment happens before the predecessor's + // terminator wires the real CFG edge into the loop header, so the resolved + // incoming block can be the predecessor EVM block's entry MIR block rather + // than its terminator MIR block. finalizeStackMergePhiIncomingBlocks() + // re-resolves every recorded incoming block against the now-complete CFG. + std::vector> StackMergePhiBlocks; + void finalizeStackMergePhiIncomingBlocks(); + struct MemoryCompileStats { uint64_t MLoadExpandCount = 0; uint64_t MStoreExpandCount = 0; diff --git a/src/compiler/mir/instructions.h b/src/compiler/mir/instructions.h index 3891d2a91..423585cbf 100644 --- a/src/compiler/mir/instructions.h +++ b/src/compiler/mir/instructions.h @@ -203,6 +203,14 @@ class PhiInstruction : public DynamicOperandInstruction { } } + // Update only the incoming block for an edge, leaving the incoming value + // unchanged. Used when the CFG edge an incoming block represents is resolved + // after the value has already been wired. + void setIncomingBlock(size_t Index, MBasicBlock *Block) { + ZEN_ASSERT(Index < Blocks.size()); + Blocks[Index] = Block; + } + private: friend class DynamicOperandInstruction;