Skip to content

[AUTOMATED] fix(p9): branchflip armswap - an else-if collapse must not steal a nested if's brace - #282

Merged
mahaloz merged 1 commit into
mainfrom
fix/branchflip
Aug 11, 2026
Merged

[AUTOMATED] fix(p9): branchflip armswap - an else-if collapse must not steal a nested if's brace#282
mahaloz merged 1 commit into
mainfrom
fix/branchflip

Conversation

@mahaloz

@mahaloz mahaloz commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

The defect

A statement escaped its else arm and executed on the then-path too, so kuna
shipped semantically wrong C on a default-ON pass. Two independent witnesses,
both checked against the disassembly:

cleanflight serialUART @0x800ba0c (ARM Thumb, O0). kuna called
IOInit(rxIO) / IOConfigGPIOAF(rxIO) on the SERIAL_BIDIR path. The machine
code never takes it:

 800bab2: tst.w r8, #8
 800bab6: bne.n 0x800bb06        <- BIDIR arm
 800bada: tst.w r7, #1 ; beq 0x800bb22    <- RX guard (BIDIR never reaches it)
 800bb06-800bb20: BIDIR body, falls through to
 800bb22: <join>
       if (a3 & 8) { // branch-flip                <- options & SERIAL_BIDIR
         ...BIDIR body...
       }
-      else if ((a2 & 2) && (v2)) { ...TX... }
-      if ((a2 & 1) && (v3)) { ...RX... }          <- ESCAPED the else arm
+      else {
+        if ((a2 & 2) && (v2)) { ...TX... }
+        if ((a2 & 1) && (v3)) { ...RX... }
+      }

iproute2 ip netns_add @0x281bb (x86-64, O0). kuna read argv[1] and ran
get_s32 on the create path; 2821d: jg 282e5 jumps straight past that block.
Same fix re-nests it.

Both witnesses were re-derived on the rebased base (11f40f46, i.e. with
#280 funcboundflow merged). funcboundflow bounds fall-through at a known
function entry and so could have moved a stripped-ARM boundary, but it did not
touch this one: the serialUART body is byte-identical between the pre-#280
build and the 11f40f46 base (both sha1[:10] = 4343611222, 68 lines, one
else if), so the disassembly correspondence above still holds verbatim. The
fixed build is the only one that differs (70 lines, zero else if).

The filed root cause was wrong

The triage record named Funcdata::block_if_flip_negated_guard's
swap_blocks(sif, 1, 2) as the site, on the theory that an arm which was a
BlockList of two BlockIfs lost its tail child. It did not. Dumping the
sblocks tree before and after the flip on serialUART shows a clean swap of
components 1 and 2 with every child intact, and nothing between the swap and
emission re-parents anything. The block tree is never corrupted; the defect is
entirely in the printer. It is also not architecture-specific (the record
reported three x86-64 reproducers that did not corrupt): a 12-line gcc -O0
x86-64 function reproduces it, and that is the stage test shipped here.

The real mechanism

S8 folds a run of sibling guards into ONE BlockIf whose condition
component
is a BlockList that leads with a whole if statement — the
standard Ghidra "condition block carries statements" shape. When branchflip
swaps the arms, that statement-carrying BlockIf lands in the else slot and
PrintC::emit_block_if takes the else if collapse path.

That collapse is a lazy brace: the clause frame registers a brace that opens
at the first line break, and cancels it if nothing forced one. Upstream decides
with emit->hasPendingPrint(&pendingBrace) — a pointer-identity test
against the frame's own PendingBrace. kuna's port asked the emitter "is any
brace pending", so the first nested if inside the clause's condition block
grabbed the ancestor's brace, cancelled it, rendered itself as the else if,
and left the real clause's if header on a fresh line at the parent's indent —
hoisting its body out of the arm.

The fix

Only the frame whose registration is still the active one (generation-stamp
match, kuna's existing stand-in for C++ pointer identity) may cancel and
collapse. Strict bug fix per docs/agents.md, no option: branchflip stays
default-ON, and branchflip off was only ever a workaround. No default changes,
so no DIV row (and no catalog counters move: 96 settables on both sides).

Guard rails (debug-only, zero cost in release) — and what they caught

  • PrintC::emit_block_if asserts that every registered pending brace is
    resolved by its own frame — fired or self-cancelled.
  • Funcdata::block_if_flip_negated_guard asserts that the multiset of leaf
    components reachable under the BlockIf is unchanged across the flip, since
    the flip is a pure arm swap. This is the guard rail the triage asked for; it
    polices the whole arm-rearranging family. It does not fire here, which is
    itself the evidence that the tree was never the problem.

Both were verified rather than assumed. Applying only the printer assertion
(not the fix) to the unfixed 11f40f46 base and running every corpus file
through it, the assertion fires on 3 of the 195 files — and two of them are
pre-existing instances already in kuna's own corpus that no assertion covered:

file before the fix
tests/stages/switchsharedcase-b2sum.xml four statements incl. goto label_401403 / goto label_401572 chains escape the else arm
tests/stages/ghangr-x8664-cvs-863633.xml a labelled label_4059c4: block and its body escape the else arm
tests/stages/ghdec-branchflip-armswap.xml the new reproducer

On the fixed tree the same corpus (112 stages + 83 datatests = 195 files,
413 + 675 assertions) runs under both assertions with 0 firings, and the
whole debug workspace suite (4,540 tests) passes with them live.

Corpus sweep (standing requirement 8)

Re-run against a base binary built at 11f40f46 so the A/B isolates this fix
on the current base rather than conflating it with #280. decompile-all, base vs
branch, over 155 decbench binaries / 86,634 functions — x86-64 ELF at
O0/O2/O2-noinline, ARM Cortex-M firmware, i386 PE:

arch binaries functions changed
x86-64 ELF 117 48,733 324 (0.66%)
ARM Cortex-M 26 36,870 111 (0.30%)
i386 PE 12 1,031 0
total 155 86,634 435 (0.50%)

Every one of the 435 was re-decompiled with both binaries and classified:

  • 435/435 byte-identical after stripping all whitespace and all braces — no
    statement added, removed, reordered or altered. The only change is bracing and
    indentation.
  • 435/435 carry the exact expected signature (an else if becoming a braced
    else {).
  • 435 nest strictly deeper, 0 shallower, 0 brace-balance changes.

(The earlier sweep against the pre-#280 base gave 441 changed with the same
verdict; the 6-function difference is #280's own effect on which bodies exist.)

Speed

Interleaved min-of-13 on an idle box (load 3.6), base 11f40f46 vs branch — no
option to flip, so this is scripts.pipeline.timeit's binary-flip analogue:

case min-of-13 median
cleanflight serialUART, one function -0.49% +0.30%
iproute2 netns_add, one function +0.01% +0.23%
decompile-all coreutils sort -0.12% -0.02%
decompile-all crazyflie CMSIS_DAP -0.13% +0.01%

Within the 5% budget — the change is two integer comparisons per BlockIf.

Ships with

  • tests/stages/ghdec-branchflip-armswap.xml — two-pass end-to-end testcase on a
    12-line x86-64 reproducer. 4 of its 6 asserts fail on the pre-fix binary and
    all 6 pass after. Corpus file count in kuna-base/src/xml.rs re-derived from the
    tree (83 datatests + 112 stages = 195, not by arithmetic) and
    docs/baseline-stages.json re-recorded (407 -> 413, additions only — no existing
    key moved).
  • Spec prose: docs/spec/09-emission.md gains a Pending-brace ownership
    paragraph (the normative rule); docs/spec/08-structuring.md records the
    arm-swap leaf-multiset invariant and points at it.
  • No option, no DIV row, no catalog counter changes.

Gates (verbatim, on this exact tree, rebased onto 11f40f46)

$ make test
datatests: 675/675 assertions passed
exit: 0

=== baseline parity ===
PARITY OK

$ make test-stages
datatests: 413/413 assertions passed
exit: 0

=== baseline parity ===
PARITY OK

$ make rust-test
MAKE_EXIT=0
workspace suite: 4540 passed; 0 failed; 37 ignored

$ make check-spec
check-spec OK (lenient mode)

$ kuna catalog --check
catalog OK: documents exactly the registered kuna options

Note for whoever merges the sibling option PRs (#281 guardarm, #283
loopcondhoist): each bumps settables 96 -> 97 and the corpus file count, so
this PR will conflict with them on kuna-base/src/xml.rs and
docs/baseline-stages.json. Resolve by re-deriving both from the merged tree,
not by hand-merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01C8UQbPqALzdUQ3cLLjUeKH

…t steal a nested if's brace

A statement escaped its `else` arm and executed on the then-path too, so kuna
shipped semantically wrong C on a default-ON pass. Two independent witnesses:

* cleanflight `serialUART` @0x800ba0c (ARM Thumb) called
  `IOInit(rxIO)`/`IOConfigGPIOAF(rxIO)` on the SERIAL_BIDIR path. The machine
  code never takes it: 800bab6 branches to the BIDIR body at 800bb06, which
  falls straight through to the join at 800bb22 and never reaches the RX guard
  at 800bada.
* iproute2 `ip netns_add` @0x281bb (x86-64) read `argv[1]` and ran `get_s32` on
  the create path; `2821d: jg 282e5` jumps past that block.

The filed root cause was wrong, and this matters for the guard rail. The
`branchflip` arm swap does NOT corrupt the block tree - dumping sblocks before
and after `Funcdata::block_if_flip_negated_guard` on serialUART shows a clean
swap of components 1 and 2 with every child intact, and no pass between the
swap and emission re-parents anything. The defect was entirely in the printer.

Mechanism. S8 folds a run of sibling guards into ONE `BlockIf` whose
*condition component* is a `BlockList` leading with a whole `if` statement.
When `branchflip` swaps the arms, that statement-carrying `BlockIf` lands in
the else slot and `PrintC::emit_block_if` takes the `else if` collapse path.
The collapse is a lazy brace: the clause frame registers a brace that opens at
the first line break. Upstream decides whether to cancel it with
`emit->hasPendingPrint(&pendingBrace)` - a pointer-identity test against the
frame's OWN `PendingBrace`. kuna's port asked the emitter "is any brace
pending", so the first nested `if` inside the clause's condition block grabbed
the ancestor's brace, cancelled it, rendered ITSELF as the `else if`, and left
the real clause's `if` header on a fresh line at the parent's indent - hoisting
its body out of the arm.

Fix: only the frame whose registration is still the active one (generation
stamp match, kuna's existing stand-in for C++ pointer identity) may cancel and
collapse. Strict bug fix per docs/agents.md, no option; `branchflip` stays
default-ON, and `branchflip off` was only ever a workaround.

Guard rails (both debug-only, zero cost in release):
* `PrintC::emit_block_if` now asserts that every registered pending brace is
  resolved by its own frame - fired or self-cancelled. That is the assertion
  that would have caught this bug.
* `Funcdata::block_if_flip_negated_guard` now asserts that the multiset of leaf
  components reachable under the `BlockIf` is unchanged across the flip, since
  the flip is a pure arm swap. It polices the whole arm-rearranging family
  (it does not fire here - the tree was never the problem).

Corpus sweep, old vs new binary, `decompile-all` over 155 decbench binaries /
86,634 functions (x86-64 ELF at O0/O2/O2-noinline, ARM Cortex-M firmware,
i386 PE): 441 functions changed (0.51%) - 324 x86-64, 117 ARM, 0 PE. Every one
of the 441 is byte-identical after stripping all whitespace and all braces, so
no statement was added, removed, reordered or altered; 440 carry the exact
expected signature (an `else if` becoming a braced `else {`) and every changed
function nests strictly deeper, never shallower. The 441st was a
`--max-fn-seconds` watchdog artifact under load and is identical on re-run.
Brace balance is unchanged in all 441.

Speed (interleaved min-of-11, old vs new binary): serialUART +1.14%,
netns_add +1.91%, `decompile-all` coreutils sort +0.59%, crazyflie CMSIS_DAP
-0.29%. Within the 5% budget.

Gates: make test 675/675 PARITY OK; make test-stages 411/411 PARITY OK; make
rust-test 4537 passed / 0 failed; make check-spec OK.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01C8UQbPqALzdUQ3cLLjUeKH
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant