[AUTOMATED] fix(p6): insert snip-reads trim COPYs after the op causing an INDIRECT effect (GH-181)

[mahaloz]

Fixes #181

Root cause

Two stacked defects put the copy of a call's out-parameter textually above the call that fills it, so freecon(v1) consumed the pre-call NULL:

1. Missing upstream carve-out in Merge::snipReads's insertion point (substrate/funcdata.rs (do_snip_reads_insert_point)). Upstream merge.cc:461-464 ("snip must come after OP CAUSING EFFECT") inserts the cover-trim COPY of an INDIRECT-defined Varnode after the op iop-encoded in the INDIRECT's input 1 (the call), never at the INDIRECT marker itself, which sits before the call in block order. kuna's port unconditionally returned the defining op, so the COPY feeding freecon landed before the lgetfilecon_raw CALL in the p-code. Ported faithfully via the existing op_iop_decode (the PcodeOp::getOpFromConst equivalent); the isInput() arm already matched upstream.
2. A reordering hazard in kuna's own foldcallret (default-on, DIV-14, p6_variables/kuna_callretfold.rs). With the p-code fixed, the fold's order-safety predicate still sank the call expression into the if condition across the trim COPY that reads the call's own INDIRECT output, re-hoisting the copy at the text level. An op strictly between the call and its use that reads a value the call indirectly writes (an input defined by a CPUI_INDIRECT whose iop input names the call) is now a fold barrier. Marker ops are skipped: a later call's own INDIRECTs chain the earlier call's versions as inputs but have no textual evaluation point (without the skip, nearly every back-to-back call pair stops folding).

Both are strict correctness fixes: no new option, no DIV row.

Audit of the other upstream carve-out sites

• trimOpOutput (merge.cc:663) — kuna do_trim_op_output already had the INDIRECT arm.
• eliminateIntersect (merge.cc:554) / snipOutputInterference (merge.cc:814) — already ported via indirect_effect_op.
• mergeIndirect's snip (merge.cc:877) inserts before the INDIRECT deliberately (addr-forced convention) — kuna matches.
• trimOpInput inserts before the reading op / end of pred block — kuna matches.

Witness (tests/bug-repro/libselinux.so.1 @ 0x17370)

Before:

    v4 = NULL;
    v1 = v4;                                              // copy BEFORE the call
    if ((0 <= (int)lgetfilecon_raw(a0,&v4)) && (!strcmp(a1,v4)))
    ...
    freecon(v1);                                          // frees the pre-call NULL

After (the Ghidra/Binja shape; re-verified on current main, where #306's stackguard entry-store removal also drops the canary lines from this function):

    v5 = NULL;
    v3 = lgetfilecon_raw(a0,&v5);
    v1 = v5;                                              // copy AFTER the call
    if ((0 <= v3) && (v3 = strcmp(a1,v5), !v3))
    ...
    freecon(v1);                                          // frees the post-call value

The issue's second witness (fsetfilecon_raw @ 0xf650 in the non-vendored -O2 -fno-inline build) was not re-verified as such, but the same-idiom fsetfilecon_raw @ 0xf670 in the vendored O2 build showed the identical hoisted shape and is fixed the same way, as is setfilecon_raw @ 0x1f680.

Whole-binary collateral audit

decompile-all --json base vs fixed on tests/bug-repro/{grep,sort,faillog,libselinux.so.1}: 110 changed functions of 1629 (grep 21/449, sort 33/490, faillog 6/69, libselinux 50/621). Every diff was reviewed (normalized vN -> v# to separate renumbering); each is one of:

• Trim COPY materialized at its correct place: new v = global; / v = slot; statements after the effect call where the base printed the copy hoisted, or printed degenerate self-assignments — the bogus dat_2b684 = dat_2b684; / dat_1e368 = dat_1e368; artifacts (the wrongly-placed COPY rendering as x = x) are gone from grep/sort/cvs outputs.
• Fold refusals where a statement between a call and its use reads the call's INDIRECT effect: the call gets the explicit Ghidra-style v = call(...); spill (e.g. the many error(...,dcgettext(...)) sites, return fcntl(...) -> v = fcntl(...); return v;). Semantically strictly safer; slightly more verbose.
• Downstream fallout of the above: local renumbering, a fold newly enabled where the copy no longer sits between, condition-collapse changes where a correctly-placed copy now occupies a branch arm.

No unexplained diffs; no lost statements or changed values found.

Stage tests

• New tests/stages/gh181-snipreads-indirect.xml (two passes: default proves both fixes; option foldcallret off isolates the p-code-level snipReads fix), with a tests/stages/README.md row. Fails 2/3 assertions on the base engine, passes 3/3 fixed. Corpus count in kuna-base/src/xml.rs bumped to 206 (recounted on the rebased tree: 83 datatests + 123 stages, including [AUTOMATED] fix(p6): close GH-180 - overflow guards read the pre-add operands (verified fixed by DIV-47; regression pinned) #304's gh180 testcase); docs/baseline-stages.json re-recorded on the rebased tree (473/473 on top of [AUTOMATED] fix(p6): close GH-180 - overflow guards read the pre-add operands (verified fixed by DIV-47; regression pinned) #304/[AUTOMATED] feat(analysis): datasyms - import ELF OBJECT symbols so libc globals render by name (DIV-76) #305/[AUTOMATED] fix(p7): stackguard - remove the entry-side canary init (and its TLS residue) with the stripped check #306's main -- [AUTOMATED] fix(p6): close GH-180 - overflow guards read the pre-add operands (verified fixed by DIV-47; regression pinned) #304's eight Overflow guards read the post-add value: v3 = v3 + v1; if (SCARRY8(v3,v1)) #180 assertions and [AUTOMATED] fix(p7): stackguard - remove the entry-side canary init (and its TLS residue) with the stripped check #306's re-pinned canary tests all pass together with this change; this branch's stage-XML patterns are variable-number-agnostic, so [AUTOMATED] fix(p7): stackguard - remove the entry-side canary init (and its TLS residue) with the stripped check #306's renumbering required no re-pinning here).
• Three existing stage tests pinned renderings the fixes legitimately changed and were updated, each with an explanatory comment:
  • ghangr-x8664-cvs-863633.xml — slot 0x58 is now merge-coalesced to one declaration even with dedupvardecls off (the duplicate-high source was the mis-placed snip COPY), mirroring the file's own precedent for slot 0x3c.
  • ghangr-morton-my-message-callback-bfd2fa.xml — pure renumbering (free(v6) -> free(v7); a new explicit trim-copy local); pattern loosened to free\(v[0-9]+\). This test's passes run with foldcallret off, so it also witnesses that the removed self-assign artifact came from the snipReads fix alone.
  • ghdec-symbol-keyed-local-decls.xml — the checksum call is now explicitly sequenced before the CONCAT13 that reads the slot it may write (the base form v1 = CONCAT13(slot_checksum(&v1,4),v1) had the read and the call unsequenced in one expression); the declaration-collapse behavior under test is unchanged.

Gates

• make test — PARITY OK (675/675, no upstream datatest changed)
• make test-stages — PARITY OK (473/473 after re-record on the rebased tree)
• make rust-test — green
• make check-spec — green (docs/spec/06-variables-and-merge.md gains prose for the snip-reads insertion rule and the fold barrier)

Speed

Interleaved min-of-11, kuna decompile-all tests/bug-repro/grep --json > /dev/null: base min 14.330s vs fixed min 14.325s = -0.04% (mins over 22 interleaved pairs; an initial round under heavy sibling-agent load read +4.5% and was discarded as contention -- the quieter second round's paired mins agree at noise level).

🤖 Generated with Claude Code

https://claude.ai/code/session_01VBn8vgwoHqPFnnV4ZMApfq