diff --git a/docs/development/current/main/phases/phase-135/README.md b/docs/development/current/main/phases/phase-135/README.md index 887dee54..db3a07aa 100644 --- a/docs/development/current/main/phases/phase-135/README.md +++ b/docs/development/current/main/phases/phase-135/README.md @@ -1,8 +1,8 @@ # Phase 135: ConditionLoweringBox allocator SSOT（ValueId 衝突の根治） ## Status -- 状態: ✅ 実装完了（動作確認はローカルで実施） -- スコープ: JoinIR の条件 lowering が `JoinValueSpace` と同一の allocator（SSOT）を使うことを保証する +- 状態: ✅ P0 + P1 実装完了 +- スコープ: JoinIR の条件 lowering が `JoinValueSpace` と同一の allocator（SSOT）を使うことを保証 + contract_checks で Fail-Fast 強化 ## Problem `apps/tests/phase133_json_skip_whitespace_min.hako` などで `--verify` が失敗し、MIR に以下の SSA 破綻が出ることがあった: @@ -21,7 +21,60 @@ - `condition_lowerer::lower_condition_to_joinir` は `&mut dyn FnMut() -> ValueId` を受理する。 - `BoundaryInjector` は `condition_bindings` 注入を `dst` で重複排除し、異なる source が同一 dst に来る場合は Fail-Fast。 -## Acceptance +## Acceptance (P0) - `./target/release/hakorune --verify apps/tests/phase133_json_skip_whitespace_min.hako` が PASS - `./target/release/hakorune --dump-mir apps/tests/phase133_json_skip_whitespace_min.hako` のループ header で PHI dst の再定義がない +--- + +## P1: Contract Checks 強化（Fail-Fast） + +### 目的 +Phase 135 P0 の根治を「二度と破れない」ように、JoinIR merge 時の contract violation を早期検出する Fail-Fast を追加。 + +### 実装内容 + +#### Step 1: `verify_condition_bindings_consistent` +**場所**: `src/mir/builder/control_flow/joinir/merge/contract_checks.rs` + +**契約**: +- condition_bindings は alias（同一 join_value に複数名）を許容 +- ただし、同一 join_value が異なる host_value に紐付く場合は即座に Fail-Fast + +**検出例**: +```text +[JoinIRVerifier/Phase135-P1] condition_bindings conflict: + join_value ValueId(104) mapped to both ValueId(12) and ValueId(18) + Contract: Same join_value can have multiple names (alias) but must have same host_value. +``` + +#### Step 2: `verify_header_phi_dsts_not_redefined` +**場所**: `src/mir/builder/control_flow/joinir/merge/contract_checks.rs` + +**契約**: +- Loop header PHI dst ValueIds は、PHI 以外の命令で dst として再利用してはいけない +- 違反すると MIR SSA 破壊（PHI dst overwrite） + +**検出例**: +```text +[JoinIRVerifier/Phase135-P1] Header PHI dst ValueId(14) redefined by non-PHI instruction in block 3: + Instruction: Call { dst: Some(ValueId(14)), ... } + Contract: Header PHI dsts must not be reused as dst in other instructions. +``` + +### 統合 +`src/mir/builder/control_flow/joinir/merge/mod.rs` の `verify_joinir_contracts()` に統合： +1. Step 1 を merge 前に実行（boundary 検証） +2. Step 2 を merge 後に実行（func 検証、header PHI dst set を収集） + +### Acceptance (P1) +- ✅ `cargo build --release` 成功 +- ✅ `phase135_trim_mir_verify.sh` - PASS +- ✅ 回帰テスト: `phase132_exit_phi_parity.sh` - 3/3 PASS +- ✅ 回帰テスト: `phase133_json_skip_whitespace_llvm_exe.sh` - PASS + +### 効果 +- **予防**: 今後の Box 実装で allocator SSOT 違反や boundary 矛盾を即座に検出 +- **明示的エラー**: `--verify` の汎用的なエラーではなく、Phase 135 固有の契約違反メッセージを出力 +- **二度と破れない**: debug build で必ず検出されるため、CI/CD で確実に防げる + diff --git a/src/mir/builder/control_flow/joinir/merge/contract_checks.rs b/src/mir/builder/control_flow/joinir/merge/contract_checks.rs index fbab0943..4899af7f 100644 --- a/src/mir/builder/control_flow/joinir/merge/contract_checks.rs +++ b/src/mir/builder/control_flow/joinir/merge/contract_checks.rs @@ -2,6 +2,7 @@ use super::LoopHeaderPhiInfo; use crate::mir::join_ir::lowering::inline_boundary::JoinInlineBoundary; use crate::mir::join_ir::lowering::join_value_space::{LOCAL_MAX, PARAM_MAX, PARAM_MIN}; use crate::mir::{BasicBlockId, MirFunction, MirInstruction, ValueId}; +use std::collections::HashMap; #[cfg(debug_assertions)] pub(super) fn verify_loop_header_phis( @@ -279,3 +280,110 @@ pub(super) fn verify_valueid_regions(loop_info: &LoopHeaderPhiInfo, boundary: &J } } } + +/// Phase 135 P1 Step 1: Verify condition_bindings consistency (alias allowed, conflict fails) +/// +/// # Contract +/// +/// condition_bindings can have multiple names (aliases) pointing to the same join_value, +/// but if the same join_value appears with different host_value, it's a contract violation. +/// +/// This catches merge-time inconsistencies before BoundaryInjector tries to inject Copy +/// instructions, preventing MIR SSA breakage. +/// +/// # Example Valid (alias): +/// ```text +/// condition_bindings: [ +/// { name: "is_char_match", join_value: ValueId(104), host_value: ValueId(12) }, +/// { name: "char", join_value: ValueId(104), host_value: ValueId(12) } // Same host_value - OK +/// ] +/// ``` +/// +/// # Example Invalid (conflict): +/// ```text +/// condition_bindings: [ +/// { name: "is_char_match", join_value: ValueId(104), host_value: ValueId(12) }, +/// { name: "char", join_value: ValueId(104), host_value: ValueId(18) } // Different host_value - FAIL +/// ] +/// ``` +/// +/// # Panics +/// +/// Panics if the same join_value has conflicting host_value mappings. +#[cfg(debug_assertions)] +pub(super) fn verify_condition_bindings_consistent(boundary: &JoinInlineBoundary) { + let mut join_to_host: HashMap = HashMap::new(); + + for binding in &boundary.condition_bindings { + if let Some(&existing_host) = join_to_host.get(&binding.join_value) { + if existing_host != binding.host_value { + panic!( + "[JoinIRVerifier/Phase135-P1] condition_bindings conflict: join_value {:?} mapped to both {:?} and {:?}\n\ + Binding names with conflict: check all bindings with join_value={:?}\n\ + Contract: Same join_value can have multiple names (alias) but must have same host_value.\n\ + Fix: Ensure ConditionLoweringBox uses SSOT allocator (ConditionContext.alloc_value).", + binding.join_value, existing_host, binding.host_value, binding.join_value + ); + } + } else { + join_to_host.insert(binding.join_value, binding.host_value); + } + } +} + +/// Phase 135 P1 Step 2: Verify header PHI dsts are not redefined by non-PHI instructions +/// +/// # Contract +/// +/// Loop header PHI dst ValueIds must not be reused as dst in non-PHI instructions. +/// This prevents "PHI dst overwrite" where a Copy/BinOp/etc. instruction redefines +/// the PHI result, breaking MIR SSA. +/// +/// # Example Invalid: +/// ```text +/// bb3 (header): +/// %14 = phi [%2, bb1], [%28, bb8] // Header PHI +/// %16 = copy %0 +/// %14 = call %16.length() // INVALID: Redefines PHI dst %14 +/// ``` +/// +/// This typically happens when: +/// - ConditionLoweringBox bypasses SSOT allocator and reuses PHI dst ValueIds +/// - JoinIR merge incorrectly remaps values to PHI dst range +/// +/// # Panics +/// +/// Panics if any header PHI dst is redefined by a non-PHI instruction in the function. +#[cfg(debug_assertions)] +pub(super) fn verify_header_phi_dsts_not_redefined( + func: &MirFunction, + header_block: BasicBlockId, + phi_dsts: &std::collections::HashSet, +) { + if phi_dsts.is_empty() { + return; // No PHI dsts to protect + } + + // Check all blocks for non-PHI instructions that redefine PHI dsts + for (block_id, block) in &func.blocks { + for instr in &block.instructions { + // Skip PHIs in header block (they're the definitions we're protecting) + if *block_id == header_block && matches!(instr, MirInstruction::Phi { .. }) { + continue; + } + + // Check if this instruction redefines a PHI dst + if let Some(dst) = get_instruction_dst(instr) { + if phi_dsts.contains(&dst) { + panic!( + "[JoinIRVerifier/Phase135-P1] Header PHI dst {:?} redefined by non-PHI instruction in block {}:\n\ + Instruction: {:?}\n\ + Contract: Header PHI dsts must not be reused as dst in other instructions.\n\ + Fix: Ensure ConditionLoweringBox uses SSOT allocator (ConditionContext.alloc_value) to avoid ValueId collisions.", + dst, block_id.0, instr + ); + } + } + } + } +} diff --git a/src/mir/builder/control_flow/joinir/merge/mod.rs b/src/mir/builder/control_flow/joinir/merge/mod.rs index b9460ae8..dd227c31 100644 --- a/src/mir/builder/control_flow/joinir/merge/mod.rs +++ b/src/mir/builder/control_flow/joinir/merge/mod.rs @@ -1065,9 +1065,20 @@ fn verify_joinir_contracts( loop_info: &LoopHeaderPhiInfo, boundary: &JoinInlineBoundary, ) { + // Phase 135 P1 Step 1: Verify condition_bindings consistency (before merge) + contract_checks::verify_condition_bindings_consistent(boundary); + contract_checks::verify_loop_header_phis(func, header_block, loop_info, boundary); verify_no_phi_dst_overwrite(func, header_block, loop_info); // Phase 204-2 verify_phi_inputs_defined(func, header_block); // Phase 204-3 contract_checks::verify_exit_line(func, exit_block, boundary); contract_checks::verify_valueid_regions(loop_info, boundary); // Phase 205-4 + + // Phase 135 P1 Step 2: Verify header PHI dsts not redefined (after merge) + let phi_dsts: std::collections::HashSet<_> = loop_info + .carrier_phis + .values() + .map(|entry| entry.phi_dst) + .collect(); + contract_checks::verify_header_phi_dsts_not_redefined(func, header_block, &phi_dsts); } diff --git a/tools/smokes/v2/profiles/integration/apps/phase135_trim_mir_verify.sh b/tools/smokes/v2/profiles/integration/apps/phase135_trim_mir_verify.sh index 05186fd9..4b25935f 100644 --- a/tools/smokes/v2/profiles/integration/apps/phase135_trim_mir_verify.sh +++ b/tools/smokes/v2/profiles/integration/apps/phase135_trim_mir_verify.sh @@ -2,6 +2,11 @@ # Phase 135: MIR verify regression for Trim(A-3) + Pattern2 lowering # Purpose: prevent ValueId/SSA corruption introduced by allocator mismatch or duplicated boundary copies. # +# P0: ConditionLoweringBox uses SSOT allocator (ConditionContext.alloc_value) +# P1: contract_checks Fail-Fast: +# - verify_condition_bindings_consistent (alias allowed, conflict fails) +# - verify_header_phi_dsts_not_redefined (PHI dst overwrite detection) +# # Expected: `--verify` PASS for the Phase 133 Trim-derived fixture. source "$(dirname "$0")/../../../lib/test_runner.sh"