From 7c1b90519f056a55dcbb27cd324ff478d6138e2c Mon Sep 17 00:00:00 2001 From: agra Date: Thu, 4 Jun 2026 02:00:13 +0300 Subject: [PATCH] fix(emit): PHI predecessor for and/or operand that emits sub-CFG (issue 0078) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A string `==`/`!=` used as an operand of a short-circuit `and`/`or` emitted invalid LLVM (`PHI node entries do not match predecessors!`). String compares expand into their own memcmp sub-CFG during LLVM emission, so the operand finishes in a later basic block (`str.merge`) than the one the IR block started in. `fixupPhiNodes` wired the short-circuit merge PHI's incoming edge to `block_map[ir_block]` (the block the IR block started as), recording a stale predecessor (`%entry`/`%and.rhs.0`). Fix: record the builder's actual insertion block after emitting each IR block's instructions (`term_block_map`, via `LLVMGetInsertBlock`) and use it as the PHI predecessor. General — corrects the incoming block for any operand that emitted intermediate basic blocks (string `==`, value `match`, …), not just string `==`. Regression: examples/0045-basic-string-eq-short-circuit.sx (string `==` on both sides of `and` and of `or`, plus a match-value + enum-payload `==` shape). Fails (LLVM abort) pre-fix, passes after. --- .../0045-basic-string-eq-short-circuit.sx | 53 +++++++ .../0045-basic-string-eq-short-circuit.exit | 1 + .../0045-basic-string-eq-short-circuit.stderr | 1 + .../0045-basic-string-eq-short-circuit.stdout | 5 + ...perand-of-short-circuit-and-invalid-phi.md | 131 ++++++++++++++++++ src/ir/emit_llvm.zig | 21 ++- 6 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 examples/0045-basic-string-eq-short-circuit.sx create mode 100644 examples/expected/0045-basic-string-eq-short-circuit.exit create mode 100644 examples/expected/0045-basic-string-eq-short-circuit.stderr create mode 100644 examples/expected/0045-basic-string-eq-short-circuit.stdout create mode 100644 issues/0078-string-eq-operand-of-short-circuit-and-invalid-phi.md diff --git a/examples/0045-basic-string-eq-short-circuit.sx b/examples/0045-basic-string-eq-short-circuit.sx new file mode 100644 index 0000000..5f0ed2b --- /dev/null +++ b/examples/0045-basic-string-eq-short-circuit.sx @@ -0,0 +1,53 @@ +// String `==`/`!=` as an operand of a short-circuit `and`/`or`. +// +// A string compare lowers to its own multi-block memcmp sub-CFG, so the +// operand finishes in a later basic block than the one the short-circuit +// started in. The `and`/`or` merge PHI must take that actual block as the +// incoming predecessor. +// +// Regression (issue 0078): combining string equality with `and`/`or` used to +// emit invalid LLVM (`PHI node entries do not match predecessors!`). +#import "modules/std.sx"; + +Json :: enum { + str: string; + int_: s64; + null_; +} + +main :: () { + a := "k"; + b := "v"; + + // string == on both sides of `and` + and_tt := a == "k" and b == "v"; + and_tf := a == "k" and b == "x"; + and_ft := a == "z" and b == "v"; + print("and: {} {} {}\n", and_tt, and_tf, and_ft); + + // string == on both sides of `or` + or_ff := a == "z" or b == "x"; + or_tf := a == "k" or b == "x"; + or_ft := a == "z" or b == "v"; + print("or: {} {} {}\n", or_ff, or_tf, or_ft); + + // string == feeding both `and` and `or` in one expression + mixed := a == "k" and b == "v" or a == "z"; + print("mixed: {}\n", mixed); + + // string `!=` operands too + ne := a != "z" and b != "z"; + print("ne: {}\n", ne); + + // the larger shape: a match-expression value plus an enum-payload string + // == combined under `and`/`or`. + v : Json = .str("v"); + kind := if v == { + case .str: 1; + case .int_: 2; + case .null_: 3; + } + ok := kind == 1 and v.str == "v"; + bad := kind == 2 or v.str == "x"; + print("payload: {} {}\n", ok, bad); +} diff --git a/examples/expected/0045-basic-string-eq-short-circuit.exit b/examples/expected/0045-basic-string-eq-short-circuit.exit new file mode 100644 index 0000000..573541a --- /dev/null +++ b/examples/expected/0045-basic-string-eq-short-circuit.exit @@ -0,0 +1 @@ +0 diff --git a/examples/expected/0045-basic-string-eq-short-circuit.stderr b/examples/expected/0045-basic-string-eq-short-circuit.stderr new file mode 100644 index 0000000..8b13789 --- /dev/null +++ b/examples/expected/0045-basic-string-eq-short-circuit.stderr @@ -0,0 +1 @@ + diff --git a/examples/expected/0045-basic-string-eq-short-circuit.stdout b/examples/expected/0045-basic-string-eq-short-circuit.stdout new file mode 100644 index 0000000..1c9fcca --- /dev/null +++ b/examples/expected/0045-basic-string-eq-short-circuit.stdout @@ -0,0 +1,5 @@ +and: true false false +or: false true true +mixed: true +ne: true +payload: true false diff --git a/issues/0078-string-eq-operand-of-short-circuit-and-invalid-phi.md b/issues/0078-string-eq-operand-of-short-circuit-and-invalid-phi.md new file mode 100644 index 0000000..352a595 --- /dev/null +++ b/issues/0078-string-eq-operand-of-short-circuit-and-invalid-phi.md @@ -0,0 +1,131 @@ +# 0078 — string `==` as an `and`/`or` operand emits an invalid PHI + +> **RESOLVED.** Root cause was in the LLVM emitter, not the `and`/`or` +> lowering: `fixupPhiNodes` wired each short-circuit merge PHI's incoming +> edge to `block_map[ir_block]` — the LLVM block the IR block *started* as. +> But a single IR instruction can expand into its own sub-CFG during +> emission (string `==`'s `str.memcmp`/`str.merge` blocks; a value `match`'s +> arm blocks), leaving the builder in a later block. The terminator — and +> therefore the real predecessor edge — lands in that later block, so the +> recorded predecessor was stale (`%entry`/`%and.rhs.0` instead of +> `%str.merge`). Fix: in `src/ir/emit_llvm.zig`, record the builder's +> *actual* insertion block after emitting each IR block's instructions +> (`term_block_map`, captured via `LLVMGetInsertBlock`) and use that as the +> PHI predecessor in `fixupPhiNodes`. General — corrects the incoming block +> for ANY operand that emitted intermediate basic blocks, not just string +> `==`. Mirrors the issue-0066 "stale PHI incoming-block after an operand +> emits new blocks" shape. Regression: `examples/0045-basic-string-eq-short-circuit.sx`. + +# Symptom + +A string equality (`a == "x"`) used as an operand of a short-circuit +`and` / `or` emits LLVM IR that fails verification — the JIT (`sx run`) +and AOT paths both abort before running: + +``` +LLVM verification failed: PHI node entries do not match predecessors! + %bp = phi i1 [ false, %entry ], [ %str.eq10, %and.rhs.0 ] +label %entry +label %str.merge +Instruction does not dominate all uses! + %str.eq10 = phi i1 [ false, %and.rhs.0 ], [ %str.ceq9, %str.memcmp6 ] + %bp = phi i1 [ false, %entry ], [ %str.eq10, %and.rhs.0 ] +``` + +Integer/`error`-tag equality in the same position is fine — only the +string `==` operand miscompiles, because string `==` lowers to its own +multi-block memcmp with an internal PHI (`str.eq` ← {`str.memcmp`, +short-circuit false}). When that result is then consumed by the `and`/`or` +short-circuit merge, the predecessor set the outer PHI records does not +match the actual CFG: the string-compare's merge block becomes a +predecessor of the `and` merge, but the outer PHI still lists the original +`entry`/`and.rhs` edges. The inner `str.eq` PHI also ends up referenced +from a block it does not dominate. + +# Reproduction + +```sx +#import "modules/std.sx"; +main :: () { + a := "k"; + b := "v"; + r := a == "k" and b == "v"; // string == as an `and` operand + print("{}\n", r); +} +``` + +``` +$ ./zig-out/bin/sx run repro.sx +LLVM verification failed: PHI node entries do not match predecessors! +... +``` + +`a == "k" or b == "v"` reproduces it identically (`or.rhs` in place of +`and.rhs`). A single `a == "k"` (no `and`/`or`) compiles and runs fine, as +does `x == 1 and y == 2` (integer operands). So the trigger is specifically +a **string `==`/`!=` as an operand of a short-circuit `and`/`or`** — the +operand emits its own `str.memcmp`/`str.merge` sub-CFG, and the +short-circuit PHI then records a stale predecessor block. + +A related `match.merge`-predecessor variant of the same PHI mismatch also +appears in a LARGER function that mixes several enum-payload accesses +(`v.str`/`v.int_`) and `match` expressions with multiple `and`/`or` +operations (it surfaced while writing +`examples/0714-modules-json-reader.sx`). It did NOT reduce to a small +standalone repro — each construct compiles fine in isolation, and a single +payload-access operand (`true and e.a == 1`) or a preceding `match` +expression followed by an `and` of locals both compile — which points at +cumulative basic-block bookkeeping in the `and`/`or` lowering rather than a +single local pattern. The string-`==` case above is the reliable minimal +reproduction; the broader fix should address PHI predecessor tracking for +any `and`/`or` operand that emits intermediate basic blocks. + +# Expected + +`r` should be `true` (both compares hold) and the program print `true`. +Generally: a `string ==`/`!=` result must be usable as an operand of +`and`/`or` exactly like any other `bool`. + +# Workaround (until fixed) + +Don't combine string equality with `and`/`or` in one expression; split +into separate statements / separate boolean locals: + +```sx +ok_k := a == "k"; +ok_v := b == "v"; +r := ok_k and ok_v; // each string-eq materialized before the short-circuit +``` + +# Background / where to look + +The string `==` lowering (search `str.eq` / `str.memcmp` / `str.merge` +block names in `src/ir/lower.zig`) produces a value via a PHI that joins +the memcmp-equal block and the early-out (length-mismatch / short-circuit) +block. The boolean `and`/`or` lowering builds its own `and.rhs` / +`and.merge` (resp. `or.*`) blocks and a merge PHI. When the LHS (or RHS) +of the `and`/`or` is itself a string compare, the outer short-circuit +lowering must take the string-compare's *actual current block* (its merge +block) as the incoming predecessor for the outer PHI — not the block that +was current before the string compare emitted its sub-CFG. The mismatch +above is the classic "PHI incoming-block is stale after the operand +emitted new basic blocks" bug: the fix is to re-read the builder's current +insertion block when wiring the `and`/`or` PHI incoming edges, rather than +caching it before lowering the operand. This mirrors the shape of the +match-arm PHI fix in issue 0066. + +Discovered while writing the std.json reader regression example +(`examples/0714-modules-json-reader.sx`, flow step F2.2): an assertion +`key == "k" and val.str == "v"` triggered it. The reader library code +itself does not use this pattern; the example was rewritten to assert the +two string equalities separately. + +# Verification (once fixed) + +```sh +./zig-out/bin/sx run repro.sx # prints: true +``` + +Add a regression example (next free `examples/NNNN-*.sx` slot) that uses a +string `==` on both sides of an `and` and on both sides of an `or`, and +the full suite + `zig build test` must stay green. diff --git a/src/ir/emit_llvm.zig b/src/ir/emit_llvm.zig index 183892c..e246e59 100644 --- a/src/ir/emit_llvm.zig +++ b/src/ir/emit_llvm.zig @@ -124,6 +124,13 @@ pub const LLVMEmitter = struct { // Maps (func_idx, block_idx) → LLVM BasicBlock block_map: std.AutoHashMap(u64, c.LLVMBasicBlockRef), + // For each IR block, the LLVM block its terminator was actually emitted + // into. Usually equals `block_map[block]`, but an instruction can expand + // into its own sub-CFG (string `==`'s memcmp blocks, a value `match`'s + // arm blocks) and leave the builder in a later block, so the terminator — + // and therefore the PHI predecessor edge — lands there instead. Keyed the + // same way as `block_map`. + term_block_map: std.AutoHashMap(u64, c.LLVMBasicBlockRef), // Cached LLVM types cached_i1: c.LLVMTypeRef, @@ -278,6 +285,7 @@ pub const LLVMEmitter = struct { .func_map = std.AutoHashMap(u32, c.LLVMValueRef).init(alloc), .global_map = std.AutoHashMap(u32, c.LLVMValueRef).init(alloc), .block_map = std.AutoHashMap(u64, c.LLVMBasicBlockRef).init(alloc), + .term_block_map = std.AutoHashMap(u64, c.LLVMBasicBlockRef).init(alloc), .pending_phis = std.ArrayList(PendingPhi).empty, .cached_i1 = c.LLVMInt1TypeInContext(ctx), .cached_i8 = c.LLVMInt8TypeInContext(ctx), @@ -311,6 +319,7 @@ pub const LLVMEmitter = struct { self.jni_slots.deinit(); self.global_map.deinit(); self.block_map.deinit(); + self.term_block_map.deinit(); self.di_files.deinit(); var fsc_it = self.frame_str_cache.keyIterator(); while (fsc_it.next()) |k| self.alloc.free(k.*); @@ -1270,6 +1279,7 @@ pub const LLVMEmitter = struct { // Clear pending phis for this function self.pending_phis.clearRetainingCapacity(); + self.term_block_map.clearRetainingCapacity(); // Emit instructions for each block — use first_ref to sync ref numbering for (func.blocks.items, 0..) |block, bi| { @@ -1285,6 +1295,12 @@ pub const LLVMEmitter = struct { _ = inst_i; self.emitInst(&instruction, func_idx); } + + // The terminator may have landed in a later LLVM block than `bb` + // if an instruction in this IR block expanded into its own sub-CFG. + // Record where the builder actually is so PHI predecessors point at + // the block that holds the branch, not the block we started in. + self.term_block_map.put(block_key, c.LLVMGetInsertBlock(self.builder)) catch unreachable; } // Fixup PHI nodes: scan all blocks for branches that pass args @@ -1300,7 +1316,10 @@ pub const LLVMEmitter = struct { for (func.blocks.items, 0..) |block, bi| { const src_key = makeBlockKey(func_idx, @intCast(bi)); - const src_bb = self.block_map.get(src_key) orelse continue; + // Predecessor is the block the terminator was emitted into, which + // differs from `block_map[bi]` when an instruction expanded the + // block into a sub-CFG (string `==`, value `match`, …). + const src_bb = self.term_block_map.get(src_key) orelse continue; for (block.insts.items) |instruction| { switch (instruction.op) {