Merge pull request #20 from LLeavesG/fix/semantic-type-cleanup

Drop vacuous predicates and keep required phi copy sources

Author: asLody

dexdec/src/analysis/value_recovery/flow.rs

@@ -217,6 +217,17 @@ impl SsaCopyFlow {
         value
     }
 
+    fn omitted_source(&self, mut value: SsaVar) -> SsaVar {
+        let mut visited = BTreeSet::new();
+        while self.omitted.contains(&value) && visited.insert(value) {
+            let Some(source) = self.sources.get(&value).copied() else {
+                break;
+            };
+            value = source;
+        }
+        value
+    }
+
     fn argument(&self, value: SsaVar) -> InsnArg {
         InsnArg::reg_ssa(
             value.reg_num,
@@ -431,9 +442,14 @@ impl<'ir> ValueFlowGraph<'ir> {
             };
             for input in &phi.inputs {
                 self.retained_phi_inputs.insert(input.value);
+                let source = self.copies.omitted_source(input.value);
+                self.retained_phi_inputs.insert(source);
                 if excluded.contains_key(&input.value) {
                     pending.push(input.value);
                 }
+                if source != input.value && excluded.contains_key(&source) {
+                    pending.push(source);
+                }
             }
         }
         self.phis.retain(|phi| !recovered.contains(&phi.result));
@@ -800,35 +816,52 @@ impl<'ir> ValueFlowGraph<'ir> {
     }
 
     fn required_phi_inputs(&self) -> BTreeSet<SsaVar> {
-        let phis = self
-            .phis
-            .iter()
-            .map(|phi| (phi.result, phi))
-            .collect::<BTreeMap<_, _>>();
-        let mut pending = self
+        let pending = self
             .phis
             .iter()
             .filter(|phi| self.has_reaching_use(phi.result))
             .map(|phi| phi.result)
             .collect::<Vec<_>>();
-        let mut required = self.retained_phi_inputs.clone();
-        let mut visited = BTreeSet::new();
-        while let Some(result) = pending.pop() {
-            if !visited.insert(result) {
-                continue;
+        required_phi_input_closure(
+            &self.phis,
+            &self.copies,
+            pending,
+            self.retained_phi_inputs.clone(),
+        )
+    }
+}
+
+fn required_phi_input_closure(
+    phis: &[PhiMerge],
+    copies: &SsaCopyFlow,
+    mut pending: Vec<SsaVar>,
+    mut required: BTreeSet<SsaVar>,
+) -> BTreeSet<SsaVar> {
+    let phis = phis
+        .iter()
+        .map(|phi| (phi.result, phi))
+        .collect::<BTreeMap<_, _>>();
+    let mut visited = BTreeSet::new();
+    while let Some(result) = pending.pop() {
+        if !visited.insert(result) {
+            continue;
+        }
+        let Some(phi) = phis.get(&result) else {
+            continue;
+        };
+        for input in &phi.inputs {
+            required.insert(input.value);
+            let source = copies.omitted_source(input.value);
+            required.insert(source);
+            if phis.contains_key(&input.value) {
+                pending.push(input.value);
             }
-            let Some(phi) = phis.get(&result) else {
-                continue;
-            };
-            for input in &phi.inputs {
-                required.insert(input.value);
-                if phis.contains_key(&input.value) {
-                    pending.push(input.value);
-                }
+            if source != input.value && phis.contains_key(&source) {
+                pending.push(source);
             }
         }
-        required
     }
+    required
 }
 
 #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
@@ -875,8 +908,10 @@ impl SemanticVisitor for ControlSymbolClosure {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::ir::analysis::PhiInput;
     use crate::ir::{
-        InsnNode, InsnType, InstructionId, RegionId, SemanticCatch, SemanticExpression,
+        BlockId, EdgeKind, InsnNode, InsnType, InstructionId, RegionId, SemanticCatch,
+        SemanticExpression,
     };
 
     fn operation(
@@ -945,4 +980,38 @@ mod tests {
 
         assert!(graph.is_bound(binding));
     }
+
+    #[test]
+    fn required_phi_inputs_retain_transitive_copy_sources() {
+        let headers = SsaVar::new(12, 1);
+        let handler_copy = SsaVar::new(13, 3);
+        let null = SsaVar::new(13, 0);
+        let merged = SsaVar::new(13, 4);
+        let phis = vec![PhiMerge {
+            block: BlockId::new(113),
+            instruction: InstructionId::new(200),
+            result: merged,
+            inputs: vec![
+                PhiInput {
+                    predecessor: BlockId::new(108),
+                    edge_kind: EdgeKind::Normal,
+                    value: handler_copy,
+                },
+                PhiInput {
+                    predecessor: BlockId::new(110),
+                    edge_kind: EdgeKind::Normal,
+                    value: null,
+                },
+            ],
+        }];
+        let mut copies = SsaCopyFlow::default();
+        copies.sources.insert(handler_copy, headers);
+        copies.omitted.insert(handler_copy);
+
+        let required = required_phi_input_closure(&phis, &copies, vec![merged], BTreeSet::new());
+
+        assert!(required.contains(&handler_copy));
+        assert!(required.contains(&headers));
+        assert!(required.contains(&null));
+    }
 }

dexdec/src/ir/region/control.rs

@@ -40,6 +40,12 @@ impl ControlCandidate {
         let closure = ControlRegionClosure::new(cfg, facts, tree, handlers, entry, &domain);
         let blocks = closure.close(&blocks, matches!(&kind, RegionKind::Loop(_)))?;
         Self::refresh_loop_follow(cfg, facts, &mut kind, &blocks);
+        // Domain clipping can drop the header when it sits outside the lexical
+        // owner of the body (e.g. loop header above a nested try). Inserting
+        // that truncated region would leave `entry` outside `blocks`.
+        if !blocks.contains(&entry) {
+            return Ok(());
+        }
         if Self::already_inserted(tree, &kind, entry, &blocks) {
             return Ok(());
         }
@@ -55,6 +61,9 @@ impl ControlCandidate {
                 let closure = ControlRegionClosure::new(cfg, facts, tree, handlers, entry, &domain);
                 let core = closure.close(&core, matches!(&kind, RegionKind::Loop(_)))?;
                 Self::refresh_loop_follow(cfg, facts, &mut kind, &core);
+                if !core.contains(&entry) {
+                    return Ok(());
+                }
                 let placement = if Self::foreign_entries(facts, entry, &core).is_empty() {
                     tree.insert_laminar_region(kind, entry, core)?
                 } else {

dexdec/src/ir/region/tree.rs

@@ -1462,6 +1462,13 @@ impl SynchronizationPlacement {
                             cfg.block(*block)
                                 .map(|block| (block.offset, block.id.raw()))
                                 .unwrap_or((u32::MAX, block.raw()))
+                        })
+                        .or_else(|| {
+                            remaining.iter().copied().min_by_key(|block| {
+                                cfg.block(*block)
+                                    .map(|block| (block.offset, block.id.raw()))
+                                    .unwrap_or((u32::MAX, block.raw()))
+                            })
                         });
                 }
             }

dexdec/src/ir/semantic.rs

@@ -1018,16 +1018,28 @@ impl SemanticNode {
     }
 
     pub fn guard(condition: SemanticPredicate, node: SemanticNode) -> Self {
-        if condition.constant_value() == Some(true) || matches!(node, Self::Empty) {
-            node
-        } else if condition.constant_value() == Some(false) {
-            Self::Empty
-        } else {
-            Self::If {
+        match condition.constant_value() {
+            Some(true) => return node,
+            Some(false) => return Self::Empty,
+            None => {}
+        }
+        if matches!(node, Self::Empty) {
+            // Vacuous branches must still evaluate effectful predicates.
+            // Value recovery can inline invokes into `If` conditions and leave
+            // both arms empty; dropping the condition would erase those effects.
+            if condition.effects().is_pure() {
+                return Self::Empty;
+            }
+            return Self::If {
                 condition: SemanticOperand::new(condition),
-                then_node: Box::new(node),
+                then_node: Box::new(Self::Empty),
                 else_node: None,
-            }
+            };
+        }
+        Self::If {
+            condition: SemanticOperand::new(condition),
+            then_node: Box::new(node),
+            else_node: None,
         }
     }
 
@@ -1042,7 +1054,17 @@ impl SemanticNode {
             None => {}
         }
         match (then_node, else_node) {
-            (Self::Empty, None | Some(Self::Empty)) => Self::Empty,
+            (Self::Empty, None | Some(Self::Empty)) => {
+                if condition.effects().is_pure() {
+                    Self::Empty
+                } else {
+                    Self::If {
+                        condition: SemanticOperand::new(condition),
+                        then_node: Box::new(Self::Empty),
+                        else_node: None,
+                    }
+                }
+            }
             (Self::Empty, Some(else_node)) => Self::guard(condition.negate(), else_node),
             (then_node, None | Some(Self::Empty)) => Self::guard(condition, then_node),
             (then_node, Some(else_node)) => Self::If {
@@ -1054,6 +1076,70 @@ impl SemanticNode {
     }
 }
 
+#[cfg(test)]
+mod branch_effect_tests {
+    use super::*;
+    use crate::ir::{
+        ArgType, InstructionId, InvokeType, LiteralArg, MemberReference, MethodDescriptor,
+        MethodReference,
+    };
+
+    fn invoke_predicate() -> SemanticPredicate {
+        let mut instruction = InsnNode::new(InsnType::Invoke, 0);
+        instruction.id = InstructionId::new(1);
+        instruction.payload.invoke_type = Some(InvokeType::Virtual);
+        instruction.payload.reference = Some(MemberReference::Method(MethodReference {
+            owner: ArgType::object("java/util/ArrayList"),
+            name: "remove".into(),
+            descriptor: MethodDescriptor {
+                parameters: vec![ArgType::object("java/lang/Object")],
+                return_type: ArgType::BOOLEAN,
+            },
+        }));
+        instruction.result = Some(RegisterArg::new_ssa(0, 0, ArgType::BOOLEAN));
+        let operation = SemanticOperation::from_instruction(instruction).expect("invoke");
+        SemanticPredicate::Test(operation)
+    }
+
+    #[test]
+    fn branch_keeps_effectful_predicate_when_arms_are_empty() {
+        let node = SemanticNode::branch(invoke_predicate(), SemanticNode::Empty, None);
+        match node {
+            SemanticNode::If {
+                condition,
+                then_node,
+                else_node,
+            } => {
+                assert!(matches!(then_node.as_ref(), SemanticNode::Empty));
+                assert!(else_node.is_none());
+                assert!(!condition.value.effects().is_pure());
+            }
+            other => panic!("expected vacuous if, got {other:?}"),
+        }
+    }
+
+    #[test]
+    fn branch_drops_pure_predicate_when_arms_are_empty() {
+        let mut instruction = InsnNode::new(InsnType::If, 0);
+        instruction.id = InstructionId::new(2);
+        instruction.payload.if_op = Some(IfOp::Ne);
+        let operation = SemanticOperation::from_parts(
+            instruction,
+            vec![
+                SemanticExpression::Register(RegisterArg::new_ssa(1, 0, ArgType::INT)),
+                SemanticExpression::Literal(LiteralArg::int(0)),
+            ],
+            None,
+        );
+        let node = SemanticNode::branch(
+            SemanticPredicate::Test(operation),
+            SemanticNode::Empty,
+            None,
+        );
+        assert!(matches!(node, SemanticNode::Empty));
+    }
+}
+
 #[derive(Debug, Clone)]
 pub struct SsaSemantics {
     values: SsaValueGraph,