Render: a merge entered late by an irreducible edge is read on that edge's promise, and spelled again if it breaks

A forward edge can retreat in reverse postorder when the region is irreducible: its copy is spelled
after the first reads of the merge it enters. Owing that copy at those reads made them residual
whatever the copy would be. The merge now owes only the forward edges spelled before it, the late
edge is verified instead: the first attempt reads the merge as a variable, `Values::broken` names
each merge whose late copy gapped, and `render` spells the function again with those merges owing
one more than any edge pays (`spell_function`; each attempt adds a merge to `forced`, so there are
at most as many attempts as merges). The rule is still "every forward edge renders its copy",
checked by the attempt that stands.

Measured over bash, ls-toybox and pigz (2,508 functions), natural order against the base: the
conservative rule of the previous commits moved 51 functions (+437 residual sites), 36 of them
(+424 sites, 174 names) only because a late edge paid after the first reads (pigz `0x4118e0`
`rax_8`, bash `0x46500` 31 late edges, +34 sites); now 13 move (+9 sites). Of the 13, seven bash
functions only renumber the locals of their block-transfer loops (`reached_5`, `to_5`: lazily
declared names are numbered in reverse postorder instead of write order), bash `0x8ee40` lists its
`_next` temporaries in another order, and five hold a gapped forward copy: bash `0x2dd90` (`rbx_2`),
`0x48140` (`r13_4`, which also stops a return tail from being copied over a residual), `0x85d70`
(`rsp_8` read twice, the entry copy at `0x85de0` is a gap), ls-toybox `0x392dc` and `0x501c8` (a
256-bit merge, which has no residual, so its read becomes a gap). The order sweep still gives 0
differing under O1, O2 and O3.

Tests, from byte literals under the four write orders: an irreducible cycle whose closing edge
renders its copy reads without a residual (22 bytes; fails at the previous commit on the plain read
turned residual), and one whose closing edge carries a frame address reads the merge as a residual
after the second attempt (36 bytes; with the retry removed it fails).

Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01456qmqCGpCWJkrg6JUZNNM

crates/r2dec/src/render/mod.rs

@@ -74,9 +74,7 @@ pub fn render(
     let work = DecompileWorkControl::new(control, DecompileWorkPhase::Structuring);
     work.poll()?;
     let name = rendered_name_of(input.name(), input.function().root());
-    let mut c = CFunction::new(name, result_type(input));
     let tags = tags::Tags::of(input);
-    let values = values::Values::new(input, &tags, std::rc::Rc::clone(&c.symbols));
     let function = input.function();
     let placement = crate::structure::place::Placement::compute(
         function.cfg(),
@@ -85,7 +83,7 @@ pub fn render(
         function.root(),
     );
     let order = crate::structure::place::placed_in_order(function);
-    let spelled = spelled::spell(input, values.as_ref(), &order, &work)?;
+    let (mut c, values, spelled) = spell_function(input, &tags, &name, &order, &work)?;
     let written = control::write(input, spelled, &placement, &work);
     if let Some(stop) = written.stopped {
         return Err(stop.into());
@@ -152,6 +150,38 @@ pub fn render(
     })
 }
 
+/// D2 over every placed block. A late edge's copy is promised to render; when one gaps, spell again with
+/// that merge owing one more (ADR D58). Each attempt adds a merge to `forced`, so they end.
+fn spell_function<'a>(
+    input: &RenderInput<'a>,
+    tags: &tags::Tags,
+    name: &str,
+    order: &[u64],
+    work: &DecompileWorkControl<'_>,
+) -> Result<(CFunction, Option<values::Values<'a>>, spelled::Spelled), DecompileExecutionStop> {
+    let mut forced = r2ssa::dense::IdSet::new(input.graph().values.len());
+    loop {
+        let c = CFunction::new(name.to_owned(), result_type(input));
+        let values = values::Values::new(input, tags, std::rc::Rc::clone(&c.symbols), &forced);
+        let spelled = spelled::spell(input, values.as_ref(), order, work)?;
+        let broken = values
+            .as_ref()
+            .map(values::Values::broken)
+            .unwrap_or_default();
+        if broken.is_empty() {
+            return Ok((c, values, spelled));
+        }
+        r2il::refusal_evidence!(
+            "late-copy-gapped",
+            "{name}: {} merges read as variables on a promise a late copy broke; spelling again",
+            broken.len()
+        );
+        broken
+            .into_iter()
+            .for_each(|merge| _ = forced.insert(merge));
+    }
+}
+
 /// What the values stage declared for `c`, or that its parameters are unknown.
 fn declare(c: &mut CFunction, input: &RenderInput<'_>, values: Option<&values::Values>) {
     match values {

crates/r2dec/src/render/values.rs

@@ -42,8 +42,12 @@ pub(super) struct Values<'a> {
     /// that points to its inferred structure.
     members: Vec<Option<CExpr>>,
     /// By value index: the definitions a named value owes before a read of it is a variable; a merge
-    /// owes one per forward edge in, and a back edge pays none (doc/adr-decompiler-rewrite.md, Merges).
+    /// owes one per early forward edge in, and a back edge pays none (doc/adr-decompiler-rewrite.md, Merges).
     owed: RefCell<Vec<u32>>,
+    /// The merges the last attempt read as variables on a late edge's promise and that edge gapped.
+    broken: RefCell<Vec<ValueId>>,
+    /// The merges that owe one definition more than any edge pays, which no read of them clears.
+    forced: r2ssa::dense::IdSet<ValueId>,
     /// The dominator tree and root, which tell a forward edge from a back edge.
     dom: &'a r2ssa::domtree::DomTree,
     root: u64,
@@ -783,6 +787,7 @@ impl<'a> Values<'a> {
         input: &RenderInput<'a>,
         tags: &super::tags::Tags,
         symbols: Rc<RefCell<SymbolTable>>,
+        forced: &r2ssa::dense::IdSet<ValueId>,
     ) -> Option<Self> {
         let artifact: &'a SsaArtifact = input.artifact();
         let graph = input.graph();
@@ -818,6 +823,8 @@ impl<'a> Values<'a> {
             declared: vec![false; graph.values.len()],
             members: Vec::new(),
             owed: RefCell::new(vec![1; graph.values.len()]),
+            broken: RefCell::new(Vec::new()),
+            forced: forced.clone(),
             dom: input.function().domtree(),
             root: input.function().root(),
             bound,
@@ -979,9 +986,13 @@ impl<'a> Values<'a> {
                 .borrow_mut()
                 .declare(name, c.clone(), SymbolRole::Carrier);
             self.names[output.0 as usize] = Some((id, ty));
+            let forced = u32::from(self.forced.contains(output));
             self.owed.get_mut()[output.0 as usize] = match inst.payload {
-                InstPayload::Phi { .. } => *forward[inst.block.0 as usize]
-                    .get_or_insert_with(|| self.forward_in(input, inst.block)),
+                InstPayload::Phi { .. } => {
+                    forced
+                        + *forward[inst.block.0 as usize]
+                            .get_or_insert_with(|| self.forward_in(input, inst.block))
+                }
                 InstPayload::Op(_) => 1,
             };
             self.locals.get_mut().push(CLocal {
@@ -993,8 +1004,8 @@ impl<'a> Values<'a> {
         }
     }
 
-    /// The forward edges into `block`, each a copy its merges owe: the placed predecessors it does not
-    /// dominate, at least one. `O(preds)`.
+    /// The copies a merge in `block` owes: one per placed predecessor it does not dominate that is
+    /// spelled before it; a later one pays after the first reads and is verified (`broken`). `O(preds)`.
     fn forward_in(&self, input: &RenderInput<'_>, block: r2ssa::BlockId) -> u32 {
         let Some(block) = self.graph.block(block) else {
             return 1;
@@ -1007,7 +1018,19 @@ impl<'a> Values<'a> {
             })
             .collect::<Vec<_>>();
         forward.dedup();
-        u32::try_from(forward.len()).map_or(u32::MAX, |n| n.max(1))
+        let early = (forward.iter())
+            .filter(|pred| self.graph.block_id_for_addr(**pred) < Some(block.id))
+            .count();
+        u32::try_from(if forward.is_empty() { 1 } else { early }).unwrap_or(u32::MAX)
+    }
+
+    /// The merges this attempt read as variables on a late edge's promise that the edge then broke:
+    /// the next attempt owes them one more, so no read of them is a variable. Sorted, once each.
+    pub(super) fn broken(&self) -> Vec<ValueId> {
+        let mut broken = self.broken.borrow().clone();
+        broken.sort_unstable();
+        broken.dedup();
+        broken
     }
 
     /// A frame object's address in the function's one frame array (D4), and its extent: the
@@ -2355,6 +2378,7 @@ impl<'a> Values<'a> {
             return Vec::new();
         };
         let forward = !self.dom.is_back_edge(from, to);
+        let late = forward && from_id > to_block.id;
         let mut copies = Vec::new();
         for inst in to_block.insts.iter().filter_map(|id| self.graph.inst(*id)) {
             let InstPayload::Phi { predecessors } = &inst.payload else {
@@ -2400,25 +2424,12 @@ impl<'a> Values<'a> {
             let (Some((source, attempt)), Some((name, ty))) =
                 (source, self.names[output.0 as usize].as_ref())
             else {
-                // The edge's gap stands for this merge and the value it could not carry.
-                self.residual.borrow_mut()[inst.0 as usize] = true;
-                if forward {
-                    r2il::refusal_evidence!(
-                        "gapped-forward-copy",
-                        "{from:#x} -> {to:#x} renders no copy, so every read of the merge stays residual"
-                    );
-                }
-                out.push(unrendered_copy(to));
+                out.push(self.gapped_copy((inst, output), (from, to), (forward, late)));
                 continue;
             };
             let copy = CStmt::Expr(source.clone());
-            if forward && from_id > to_block.id {
-                r2il::refusal_evidence!(
-                    "late-forward-edge",
-                    "{from:#x} -> {to:#x} pays a merge after {to:#x} was spelled, so its early reads stay residual"
-                );
-            }
-            self.account_copy(inst, output, attempt, (&copy, forward));
+            self.promised((from, to), late);
+            self.account_copy(inst, output, attempt, (&copy, forward && !late));
             match clobbers {
                 false => out.push(self.assign_value(output, *name, source)),
                 true => {
@@ -2432,19 +2443,50 @@ impl<'a> Values<'a> {
         out
     }
 
+    /// A late edge's copy rendered: the early reads of its merge were right to rest on it.
+    fn promised(&self, (from, to): (u64, u64), late: bool) {
+        if late {
+            r2il::refusal_evidence!(
+                "late-forward-edge",
+                "{from:#x} -> {to:#x} pays a merge after {to:#x} was spelled; its reads rest on the copy rendering"
+            );
+        }
+    }
+
+    /// The gap that stands for a merge and the value its copy could not carry. On a forward edge it keeps
+    /// the merge owed; on a late one it breaks the promise the merge's early reads were made on.
+    fn gapped_copy(
+        &self,
+        (inst, output): (InstId, ValueId),
+        (from, to): (u64, u64),
+        (forward, late): (bool, bool),
+    ) -> CStmt {
+        self.residual.borrow_mut()[inst.0 as usize] = true;
+        if forward {
+            r2il::refusal_evidence!(
+                "gapped-forward-copy",
+                "{from:#x} -> {to:#x} renders no copy, so every read of the merge stays residual"
+            );
+        }
+        if late && !self.forced.contains(output) {
+            self.broken.borrow_mut().push(output);
+        }
+        unrendered_copy(to)
+    }
+
     /// The merge `inst` has a copy on this edge, rendered whether or not `stmt` is written. Only a copy
-    /// on a forward edge pays what the merge owes, once per edge; a self copy rides a back edge.
+    /// that `pays` (on an early forward edge) lowers what the merge owes, once per edge.
     fn account_copy(
         &self,
         inst: InstId,
         output: ValueId,
         attempt: Attempt,
-        (stmt, forward): (&CStmt, bool),
+        (stmt, pays): (&CStmt, bool),
     ) {
         *self.pending.borrow_mut() = attempt;
         self.mark(inst);
         self.residual_in(inst, stmt);
-        if let (true, Some(owed)) = (forward, self.owed.borrow_mut().get_mut(output.0 as usize)) {
+        if let (true, Some(owed)) = (pays, self.owed.borrow_mut().get_mut(output.0 as usize)) {
             debug_assert!(*owed > 0, "a forward edge pays a merge once");
             *owed = owed.saturating_sub(1);
         }

crates/r2engine/tests/engine/structure.rs

@@ -455,3 +455,52 @@ fn a_merge_whose_forward_copies_all_render_reads_as_a_variable_in_every_order()
         assert!(c.contains("return (uint64_t)rax_3;"), "{order:?}\n{c}");
     }
 }
+
+/// `xor eax, eax; test edi, edi; je y; x: add eax, 1; cmp eax, esi; jl y; ret; y: add eax, 2; cmp eax, edx;
+/// jl x; ret`: two ways into the cycle x, y, so neither dominates the other and one edge of it is late.
+const IRREDUCIBLE: &[u8] = &[
+    0x31, 0xc0, 0x85, 0xff, 0x74, 0x08, 0x83, 0xc0, 0x01, 0x39, 0xf0, 0x7c, 0x01, 0xc3, 0x83, 0xc0,
+    0x02, 0x39, 0xd0, 0x7c, 0xf1, 0xc3,
+];
+
+#[test]
+fn a_merge_entered_late_by_an_irreducible_edge_reads_as_a_variable_when_every_copy_renders() {
+    // The edge that closes the cycle is spelled after the reads of the merge it enters, and renders its
+    // copy, so those reads are defined; no write order and no spelling order changes that.
+    let natural = c_under(WriteOrder::Natural, IRREDUCIBLE);
+    assert!(!natural.contains("r2sleigh_residual_"), "{natural}");
+    for order in ORDERS {
+        let c = c_under(order, IRREDUCIBLE);
+        assert_eq!(verdicts_of(&c), verdicts_of(&natural), "{order:?}\n{c}");
+        assert!(!c.contains("r2sleigh_residual_"), "{order:?}\n{c}");
+    }
+}
+
+/// `sub rsp, 0x18; xor eax, eax; test edi, edi; je y; x: lea rax, [rsp + 8]; test esi, esi; jne y; add rsp,
+/// 0x18; ret; y: add eax, 1; test edx, edx; jne x; add rsp, 0x18; ret`: the late edge carries a frame address.
+const IRREDUCIBLE_GAPPED: &[u8] = &[
+    0x48, 0x83, 0xec, 0x18, 0x31, 0xc0, 0x85, 0xff, 0x74, 0x0e, 0x48, 0x8d, 0x44, 0x24, 0x08, 0x85,
+    0xf6, 0x75, 0x05, 0x48, 0x83, 0xc4, 0x18, 0xc3, 0x83, 0xc0, 0x01, 0x85, 0xd2, 0x75, 0xeb, 0x48,
+    0x83, 0xc4, 0x18, 0xc3,
+];
+
+#[test]
+fn a_merge_entered_late_by_a_gapped_copy_reads_as_a_residual_whatever_was_promised_before() {
+    // The reads of the merge are spelled before the late edge, as variables on its promise; the edge
+    // gaps, so the function is spelled again with that merge owed one more.
+    let natural = verdicts_of(&c_under(WriteOrder::Natural, IRREDUCIBLE_GAPPED));
+    let carried = natural.never_assigned.to_lowercase();
+    assert!(!carried.is_empty() && !carried.contains(','), "{natural:?}");
+    for order in ORDERS {
+        let c = c_under(order, IRREDUCIBLE_GAPPED);
+        assert_eq!(verdicts_of(&c), natural, "{order:?}\n{c}");
+        assert!(c.contains("ValuesNotRendered"), "{order:?}\n{c}");
+        let reads = reads_of(&c, &carried);
+        assert!(
+            reads
+                .iter()
+                .all(|line| line.contains("r2sleigh_residual_u64(")),
+            "{order:?}: {reads:?}\n{c}"
+        );
+    }
+}