Merge pull request #19 from LLeavesG/fix/exception-sync-cleanup

Recover split finally families and synchronized try nesting

Author: asLody

dexdec/src/ir/analysis/control_contractions.rs

@@ -23,6 +23,13 @@ pub struct ControlContractions {
 }
 
 impl ControlContractions {
+    #[cfg(test)]
+    pub(crate) fn identity() -> Self {
+        Self {
+            domains: Vec::new(),
+        }
+    }
+
     pub fn from_regions(regions: &RegionGraph) -> Self {
         ContractionGraph::new(Self::region_relations(regions)).solve(CyclePolicy::Canonical)
     }
@@ -116,8 +123,8 @@ impl ControlContractions {
     ///
     /// Cleanup contraction can turn a non-critical physical edge into a
     /// critical quotient edge. When the contracted entry has one external
-    /// ingress and immediately continues inside the same component, its tail
-    /// is the edge-specific copy site.
+    /// ingress and every normal branch remains inside the same component, its
+    /// incoming edge is the edge-specific copy site.
     pub fn normal_copy_site(
         &self,
         cfg: &CFG,
@@ -144,7 +151,7 @@ impl ControlContractions {
             return None;
         }
         let entry_targets = cfg.normal_successors(entry).collect::<BTreeSet<_>>();
-        if entry_targets.len() != 1 || !entry_targets.is_subset(&component) {
+        if entry_targets.is_empty() || !entry_targets.is_subset(&component) {
             return None;
         }
         let edge = cfg
@@ -521,7 +528,10 @@ impl ComponentTerminals {
 mod tests {
     use std::collections::BTreeSet;
 
-    use super::{BlockId, ContractionGraph, CyclePolicy};
+    use super::{
+        BlockId, ContractionGraph, CyclePolicy, EdgeKind, NormalCopySite, RegionEdge, CFG,
+    };
+    use crate::ir::Block;
 
     #[test]
     fn contracts_acyclic_chain_to_its_sink() {
@@ -608,4 +618,48 @@ mod tests {
         assert!(!contractions.contracts_to(BlockId(1), BlockId(2)));
         assert!(!contractions.contracts_to(BlockId(2), BlockId(1)));
     }
+
+    #[test]
+    fn places_copy_on_branching_contracted_entry_edge() {
+        let predecessor = BlockId::new(0);
+        let entry = BlockId::new(1);
+        let left = BlockId::new(2);
+        let right = BlockId::new(3);
+        let successor = BlockId::new(4);
+        let alternative = BlockId::new(5);
+        let outside = BlockId::new(6);
+        let contractions = ContractionGraph::new([
+            (entry, left),
+            (entry, right),
+            (left, successor),
+            (right, successor),
+        ])
+        .solve(CyclePolicy::Canonical);
+        let mut cfg = CFG::new("branching_contracted_entry");
+        for block in 0..=6 {
+            cfg.add_block(Block::new(block));
+        }
+        cfg.add_edge(predecessor, entry, EdgeKind::True);
+        cfg.add_edge(predecessor, alternative, EdgeKind::False);
+        cfg.add_edge(entry, left, EdgeKind::True);
+        cfg.add_edge(entry, right, EdgeKind::False);
+        cfg.add_edge(left, successor, EdgeKind::Normal);
+        cfg.add_edge(right, successor, EdgeKind::Normal);
+
+        assert_eq!(
+            contractions.normal_copy_site(&cfg, predecessor, entry, successor),
+            Some(NormalCopySite::Edge(RegionEdge {
+                source: predecessor,
+                target: entry,
+                kind: EdgeKind::True,
+            }))
+        );
+
+        cfg.add_edge(entry, outside, EdgeKind::SwitchDefault);
+        assert_eq!(
+            contractions.normal_copy_site(&cfg, predecessor, entry, successor),
+            None,
+            "a branch leaving the contracted component must reject edge placement"
+        );
+    }
 }

dexdec/src/ir/analysis/register_liveness.rs

@@ -13,11 +13,26 @@ pub struct RegisterLiveness {
 impl RegisterLiveness {
     pub fn analyze(cfg: &CFG) -> Self {
         let mut definitions = BTreeMap::new();
+        let mut definitions_before_exception = BTreeMap::new();
         let mut upward_uses = BTreeMap::new();
         for block in cfg.blocks.values() {
             let mut block_definitions = BTreeSet::new();
+            let mut exceptional_definitions = BTreeSet::new();
             let mut block_uses = BTreeSet::new();
-            for instruction in &block.insns {
+            // Exception successors observe the register state immediately before the
+            // terminal throwing instruction. Its result exists only on normal exit.
+            let exceptional_boundary = cfg
+                .successors_with_kind(block.id)
+                .iter()
+                .any(|(_, kind)| kind.is_exception())
+                .then(|| {
+                    block
+                        .insns
+                        .iter()
+                        .rposition(|instruction| instruction.can_throw())
+                })
+                .flatten();
+            for (index, instruction) in block.insns.iter().enumerate() {
                 let uses = instruction
                     .args
                     .iter()
@@ -36,9 +51,13 @@ impl RegisterLiveness {
                 }
                 if let Some(result) = &instruction.result {
                     block_definitions.insert(result.reg_num);
+                    if exceptional_boundary.is_none_or(|boundary| index < boundary) {
+                        exceptional_definitions.insert(result.reg_num);
+                    }
                 }
             }
             definitions.insert(block.id, block_definitions);
+            definitions_before_exception.insert(block.id, exceptional_definitions);
             upward_uses.insert(block.id, block_uses);
         }
 
@@ -51,13 +70,32 @@ impl RegisterLiveness {
         loop {
             let mut changed = false;
             for block in cfg.block_ids().into_iter().rev() {
-                let output = cfg
-                    .successors(block)
-                    .flat_map(|successor| live_in.get(&successor).into_iter().flatten().copied())
+                let mut normal_output = BTreeSet::new();
+                let mut exceptional_output = BTreeSet::new();
+                for (successor, kind) in cfg.successors_with_kind(block) {
+                    let output = if kind.is_exception() {
+                        &mut exceptional_output
+                    } else {
+                        &mut normal_output
+                    };
+                    output.extend(live_in.get(successor).into_iter().flatten().copied());
+                }
+                let output = normal_output
+                    .union(&exceptional_output)
+                    .copied()
                     .collect::<BTreeSet<_>>();
                 let mut input = upward_uses.get(&block).cloned().unwrap_or_default();
                 let defined = definitions.get(&block).cloned().unwrap_or_default();
-                input.extend(output.difference(&defined).copied());
+                let defined_before_exception = definitions_before_exception
+                    .get(&block)
+                    .cloned()
+                    .unwrap_or_default();
+                input.extend(normal_output.difference(&defined).copied());
+                input.extend(
+                    exceptional_output
+                        .difference(&defined_before_exception)
+                        .copied(),
+                );
                 if live_out.get(&block) != Some(&output) {
                     live_out.insert(block, output);
                     changed = true;

dexdec/src/ir/analysis/source_variables.rs

@@ -54,14 +54,8 @@ impl SourceVariableAllocation {
         regions: &RegionGraph,
     ) -> Result<Self, SourceVariableError> {
         let contractions = ControlContractions::for_edge_arguments(cfg, regions);
-        let required_phis = RequiredPhiValues::collect(
-            cfg,
-            values,
-            root,
-            constants,
-            recovered_phis,
-            &contractions,
-        )?;
+        let required_phis =
+            RequiredPhiValues::collect(cfg, values, root, constants, &contractions)?;
         let statement_definitions = StatementDefinitions::collect(root);
         let mut cleanup_values = regions
             .cleanup_value_bindings()

dexdec/src/ir/analysis/source_variables/phi.rs

@@ -505,7 +505,6 @@ impl RequiredPhiValues {
         graph: &SsaValueGraph,
         root: &SemanticNode,
         constants: &BTreeMap<SsaVar, InsnArg>,
-        recovered: &BTreeSet<SsaVar>,
         contractions: &ControlContractions,
     ) -> Result<BTreeSet<SsaVar>, SourceVariableError> {
         let mut collector = Self {
@@ -520,7 +519,13 @@ impl RequiredPhiValues {
         let edge_arguments = ContractedEdgeArguments::new(cfg, graph, constants, contractions);
         while let Some(value) = pending.pop() {
             if !visited.insert(value)
-                || (recovered.contains(&value) && materialized.contains(&value))
+                // A semantic statement owns its rewritten dependencies. In
+                // particular, gated value recovery can replace a CFG move
+                // from a Phi with a Select expression while retaining the
+                // move's result identity. Following the stale CFG operand
+                // would lower that already-recovered Phi a second time and
+                // place copies before the Select's branch definitions.
+                || materialized.contains(&value)
                 || PhiCopies::canonical_constant(constants, value).is_some()
             {
                 continue;
@@ -2837,6 +2842,67 @@ mod tests {
         assert_eq!(resolver.physical_type(moved_value), Some(ArgType::BOOLEAN));
     }
 
+    #[test]
+    fn semantic_select_definition_does_not_rematerialize_its_cfg_phi() {
+        let entry = BlockId::new(0);
+        let left_block = BlockId::new(1);
+        let right_block = BlockId::new(2);
+        let join = BlockId::new(3);
+        let left = RegisterArg::new_ssa(1, 0, ArgType::INT);
+        let right = RegisterArg::new_ssa(1, 1, ArgType::INT);
+        let phi_result = RegisterArg::new_ssa(1, 2, ArgType::INT);
+        let moved = RegisterArg::new_ssa(3, 0, ArgType::INT);
+        let phi_value = SsaVar::from_reg(&phi_result).expect("phi SSA value");
+
+        let mut cfg = CFG::new("recovered_select_move");
+        cfg.entry = entry;
+        cfg.add_block(Block::new(entry));
+        let mut left_body = Block::new(left_block);
+        left_body.push(InsnNode::const_value(left.clone(), 1));
+        cfg.add_block(left_body);
+        let mut right_body = Block::new(right_block);
+        right_body.push(InsnNode::const_value(right.clone(), 2));
+        cfg.add_block(right_body);
+        let mut join_body = Block::new(join);
+        join_body.push(InsnNode::phi(
+            phi_result.clone(),
+            vec![
+                (left_block.raw(), InsnArg::Reg(left.clone())),
+                (right_block.raw(), InsnArg::Reg(right.clone())),
+            ],
+        ));
+        join_body.push(InsnNode::move_insn(moved.clone(), InsnArg::Reg(phi_result)));
+        cfg.add_block(join_body);
+        cfg.add_edge(entry, left_block, EdgeKind::True);
+        cfg.add_edge(entry, right_block, EdgeKind::False);
+        cfg.add_edge(left_block, join, EdgeKind::Normal);
+        cfg.add_edge(right_block, join, EdgeKind::Normal);
+
+        let values = SsaValueGraph::build(&cfg).expect("SSA graph");
+        let root = SemanticNode::BasicBlock(SemanticBlock {
+            id: join,
+            statements: vec![SemanticStatement::definition(
+                InstructionId::new(7),
+                moved,
+                SemanticExpression::select(
+                    crate::ir::SemanticPredicate::True,
+                    SemanticExpression::Register(left),
+                    SemanticExpression::Register(right),
+                ),
+            )],
+        });
+        let required = RequiredPhiValues::collect(
+            &cfg,
+            &values,
+            &root,
+            &BTreeMap::new(),
+            &ControlContractions::identity(),
+        )
+        .expect("required Phi analysis");
+
+        assert!(!required.contains(&phi_value));
+    }
+
     #[test]
     fn copy_resolver_selects_nested_phi_value_for_exception_edge() {
         let entry = BlockId::new(3);

dexdec/src/ir/cfg.rs

@@ -561,6 +561,14 @@ impl CFG {
         self.preds_dirty = true;
     }
 
+    /// Remove one typed edge while preserving parallel edges with a different kind.
+    pub fn remove_edge_with_kind(&mut self, from: BlockId, to: BlockId, kind: EdgeKind) {
+        if let Some(successors) = self.successors.get_mut(&from) {
+            successors.retain(|edge| *edge != (to, kind));
+        }
+        self.preds_dirty = true;
+    }
+
     /// Get the kind of edge between two blocks, if it exists.
     pub fn get_edge_kind(&self, from: BlockId, to: BlockId) -> Option<EdgeKind> {
         self.successors