From 60f358eb8e2562fe48c20f97b0546da5dcfb9990 Mon Sep 17 00:00:00 2001 From: LLeaves Date: Wed, 5 Aug 2026 08:42:03 +0800 Subject: [PATCH 1/2] fix(exceptions): bind straight-line cleanup state --- dexdec/src/analysis/value_recovery.rs | 143 ++++++++++++++++-- dexdec/src/ir/exception/cleanup.rs | 54 +++++++ .../expected/SynchronizedAdvanced.kt.expected | 9 +- 3 files changed, 190 insertions(+), 16 deletions(-) diff --git a/dexdec/src/analysis/value_recovery.rs b/dexdec/src/analysis/value_recovery.rs index fc1f66b..e783581 100644 --- a/dexdec/src/analysis/value_recovery.rs +++ b/dexdec/src/analysis/value_recovery.rs @@ -18,7 +18,7 @@ mod source; mod ssa_constants; use crate::analysis::SemanticTransform; -use crate::ir::analysis::{DominanceError, SsaVar}; +use crate::ir::analysis::{DominanceError, PhiMerge, SsaVar}; use crate::ir::{ BlockId, SemanticFoldError, SemanticMethod, SourceVariableContext, SsaSemantics, ValueSemantics, CFG, @@ -258,26 +258,41 @@ impl SemanticTransform for ValueRecovery { "value.ssa.specialization_apply", specialization_schedule.apply(method.body_mut()) )?; - let retained = - crate::profile_scope!("value.ssa.cleanup_roots", cleanup_state_values(&method)); + let retained = crate::profile_scope!( + "value.ssa.cleanup_roots", + cleanup_state_values(&method, &self.ssa_constants) + ); let constants = SsaValueSolver::new(&recovered_phis, &self.ssa_constants, &retained) .solve_profiled(&mut method)?; Ok(method.into_values(constants, recovered_phis)) } } -fn cleanup_state_values(method: &SemanticMethod) -> BTreeSet { - let mut values = method +fn cleanup_state_values( + method: &SemanticMethod, + constants: &BTreeMap, +) -> BTreeSet { + let roots = method .state() .regions() .cleanup_value_bindings() .iter() - .flat_map(|(handler, normal)| [*handler, *normal]) + .flat_map(|(handler, normal)| [*handler, *normal]); + cleanup_state_closure(roots, method.state().values().phis(), constants) +} + +fn cleanup_state_closure( + roots: impl IntoIterator, + phis: &[PhiMerge], + constants: &BTreeMap, +) -> BTreeSet { + // Shared null/zero roots can be inlined, but a cleanup root that is a + // non-trivial constant (for example a catch-path `-1`) must stay live. + let mut values = roots + .into_iter() + .filter(|value| !trivial_cleanup_constant(constants, *value)) .collect::>(); - let phis = method - .state() - .values() - .phis() + let phis = phis .iter() .map(|phi| (phi.result, phi)) .collect::>(); @@ -287,7 +302,7 @@ fn cleanup_state_values(method: &SemanticMethod) -> BTreeSet) -> BTreeSet, mut value: SsaVar) -> bool { + let mut visited = BTreeSet::new(); + while visited.insert(value) { + match constants.get(&value) { + Some(crate::ir::InsnArg::Reg(register)) => { + let Some(next) = SsaVar::from_reg(register) else { + return false; + }; + value = next; + } + Some(crate::ir::InsnArg::Lit(_)) => return true, + Some(crate::ir::InsnArg::Wrapped(instruction)) => { + return matches!( + instruction.insn_type, + crate::ir::InsnType::Const | crate::ir::InsnType::ConstStr + ); + } + None => return false, + } + } + false +} + +fn trivial_cleanup_constant( + constants: &BTreeMap, + mut value: SsaVar, +) -> bool { + let mut visited = BTreeSet::new(); + while visited.insert(value) { + match constants.get(&value) { + Some(crate::ir::InsnArg::Reg(register)) => { + let Some(next) = SsaVar::from_reg(register) else { + return false; + }; + value = next; + } + Some(crate::ir::InsnArg::Lit(literal)) => return literal.is_zero(), + Some(crate::ir::InsnArg::Wrapped(instruction)) => { + return matches!( + instruction.insn_type, + crate::ir::InsnType::Const | crate::ir::InsnType::ConstStr + ) && instruction.args.iter().any(|argument| match argument { + crate::ir::InsnArg::Lit(literal) => literal.is_zero(), + _ => false, + }); + } + None => return false, + } + } + false +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::ir::analysis::PhiInput; + use crate::ir::{ArgType, EdgeKind, InsnArg, InstructionId}; + + #[test] + fn cleanup_state_closure_does_not_retain_shared_null_constant() { + let result = SsaVar::new(1, 26); + let live_input = SsaVar::new(1, 25); + let normal = SsaVar::new(12, 3); + let null = SsaVar::new(1, 8); + let phis = [PhiMerge { + block: BlockId::new(118), + instruction: InstructionId::new(0), + result, + inputs: vec![ + PhiInput { + predecessor: BlockId::new(74), + edge_kind: EdgeKind::Normal, + value: null, + }, + PhiInput { + predecessor: BlockId::new(117), + edge_kind: EdgeKind::Normal, + value: live_input, + }, + ], + }]; + let constants = BTreeMap::from([(null, InsnArg::lit(0, ArgType::unknown_object()))]); + + assert_eq!( + cleanup_state_closure([result, normal], &phis, &constants), + BTreeSet::from([result, live_input, normal]) + ); + } + + #[test] + fn cleanup_state_closure_retains_a_nonzero_constant_root() { + let catch_result = SsaVar::new(1, 13); + let normal = SsaVar::new(1, 6); + let zero = SsaVar::new(1, 0); + let constants = BTreeMap::from([ + (catch_result, InsnArg::lit(-1, ArgType::INT)), + (zero, InsnArg::lit(0, ArgType::INT)), + ]); + + assert_eq!( + cleanup_state_closure([catch_result, normal, zero], &[], &constants), + BTreeSet::from([catch_result, normal]) + ); + } +} + struct SsaValueSolver<'a> { recovered_phis: &'a BTreeSet, constants: &'a BTreeMap, diff --git a/dexdec/src/ir/exception/cleanup.rs b/dexdec/src/ir/exception/cleanup.rs index 60289cd..c08deeb 100644 --- a/dexdec/src/ir/exception/cleanup.rs +++ b/dexdec/src/ir/exception/cleanup.rs @@ -1487,6 +1487,7 @@ impl<'a> CleanupProof<'a> { .map(|(handler, normal)| (handler, 0, normal, 0)), ); } + let prior_values = self.values.clone(); if !self.instruction(handler_insn, normal_insn) { return self.reject( handler, @@ -1496,6 +1497,12 @@ impl<'a> CleanupProof<'a> { CleanupMismatchReason::Instruction, ); } + // The normal cleanup copy is elided after this proof, so + // values consumed by the two observable operations must be + // represented by the same source variable. Branch operands + // are recorded by `branch_relation`; straight-line cleanup + // operations need the same treatment here. + self.record_state_bindings(&prior_values); if handler != normal { self.matched_normal.insert(StatementOrigin { block: normal, @@ -2372,4 +2379,51 @@ mod tests { assert!(proof.bind_block(handler, observable)); assert!(!proof.bind_block(handler, target)); } + + #[test] + fn straight_line_cleanup_records_observable_argument_binding() { + let handler = BlockId::new(0); + let normal = BlockId::new(1); + let handler_value = RegisterArg::new_ssa( + 1, + 7, + ArgType::object("java/io/Closeable"), + ); + let normal_value = RegisterArg::new_ssa( + 12, + 3, + ArgType::object("java/io/Closeable"), + ); + let mut cfg = CFG::new("cleanup_straight_line_state"); + let mut handler_block = Block::new(handler); + handler_block.push(InsnNode::invoke( + InvokeType::Static, + 0, + vec![InsnArg::Reg(handler_value.clone())], + )); + cfg.add_block(handler_block); + let mut normal_block = Block::new(normal); + normal_block.push(InsnNode::invoke( + InvokeType::Static, + 0, + vec![InsnArg::Reg(normal_value.clone())], + )); + cfg.add_block(normal_block); + + let values = SsaValueGraph::default(); + let origins = SsaOrigins::analyze(&values); + let handler_blocks = BTreeSet::from([handler]); + let rethrow_blocks = BTreeSet::from([handler]); + let mut proof = + CleanupProof::new(&cfg, &values, &origins, &handler_blocks, &rethrow_blocks); + + assert!(proof.compare(handler, normal).expect("cleanup proof")); + assert_eq!( + proof.value_bindings(), + BTreeSet::from([( + SsaVar::from_reg(&handler_value).expect("handler SSA value"), + SsaVar::from_reg(&normal_value).expect("normal SSA value"), + )]) + ); + } } diff --git a/dexdec/tests/testcases/expected/SynchronizedAdvanced.kt.expected b/dexdec/tests/testcases/expected/SynchronizedAdvanced.kt.expected index 1a68c70..27f4d97 100644 --- a/dexdec/tests/testcases/expected/SynchronizedAdvanced.kt.expected +++ b/dexdec/tests/testcases/expected/SynchronizedAdvanced.kt.expected @@ -131,16 +131,15 @@ public open class SynchronizedAdvanced { public fun syncWithFinally(p0: Int): Int { synchronized(SynchronizedAdvanced.lock1!!) { - var result: Int + var value: Int try { - result = 100 / p0 + value = 100 / p0 } catch (exception: ArithmeticException) { - result = -1 - return result + return -1 } finally { SynchronizedAdvanced.value = 0 } - return result + return value } } From 2fc15a2413cf414af313263df3cd1bf070de5ca0 Mon Sep 17 00:00:00 2001 From: LLeaves Date: Sat, 8 Aug 2026 01:31:40 +0800 Subject: [PATCH 2/2] fix(exceptions): preserve shared handler cleanup --- dexdec/src/ir/exception.rs | 98 +++++++++++++++++++++++++++++- dexdec/src/ir/exception/cleanup.rs | 28 ++++++--- 2 files changed, 115 insertions(+), 11 deletions(-) diff --git a/dexdec/src/ir/exception.rs b/dexdec/src/ir/exception.rs index b633c9e..5f8f8b6 100644 --- a/dexdec/src/ir/exception.rs +++ b/dexdec/src/ir/exception.rs @@ -103,6 +103,58 @@ impl TryRegion { } } +/// Canonical handler entries referenced by more than one protected region. +fn shared_handler_entries(handlers: impl IntoIterator) -> BTreeSet { + let mut owners = BTreeMap::>::new(); + for (region, entry) in handlers { + owners.entry(entry).or_default().insert(region); + } + owners + .into_iter() + .filter_map(|(entry, regions)| (regions.len() > 1).then_some(entry)) + .collect() +} + +/// Catch entries shared by try regions that do not share a cleanup identity. +/// +/// Split fragments of one source `try` reuse the same catch and finally +/// entries; those must still recover structured `finally`. Two independent +/// tries that only share a catch tail must not elide that tail on behalf of +/// one region. +fn conflicting_shared_catch_entries(regions: &[TryRegion]) -> BTreeSet { + let mut owners = BTreeMap::)>>::new(); + for region in regions { + let cleanup = region + .handlers + .iter() + .find(|handler| handler.kind != HandlerKind::Catch) + .map(|handler| handler.canonical_entry); + for handler in region.catch_handlers() { + owners + .entry(handler.canonical_entry) + .or_default() + .insert((region.id, cleanup)); + } + } + owners + .into_iter() + .filter_map(|(entry, identities)| { + let region_count = identities + .iter() + .map(|(region, _)| *region) + .collect::>() + .len(); + let cleanup_identities = identities + .iter() + .map(|(_, cleanup)| *cleanup) + .collect::>(); + let conflicting_cleanup = + cleanup_identities.len() > 1 || cleanup_identities == BTreeSet::from([None]); + (region_count > 1 && conflicting_cleanup).then_some(entry) + }) + .collect() +} + #[derive(Debug, Clone)] pub struct CleanupContraction { pub entry: BlockId, @@ -409,6 +461,7 @@ impl<'a> ExceptionAnalyzer<'a> { HandlerDomains::assign(self.cfg, &mut regions); let nested_handlers = NestedHandlerDomains::analyze(®ions); let recovery_order = Self::cleanup_recovery_order(®ions); + let shared_handler_entries = conflicting_shared_catch_entries(®ions); let mut elided_instructions = BTreeSet::new(); let mut cleanup_contractions = Vec::new(); @@ -422,7 +475,13 @@ impl<'a> ExceptionAnalyzer<'a> { .ok_or(ExceptionInvariantError::MissingExceptionScope(region_id))?; let nested_cleanup = nested_handlers.cleanup(region.id); let nested_all = nested_handlers.all(region.id); - let cleanup = CleanupRecovery::new(self.cfg, self.values, &normal_dominators).recover( + let cleanup = CleanupRecovery::new( + self.cfg, + self.values, + &normal_dominators, + &shared_handler_entries, + ) + .recover( region, &nested_cleanup, &nested_all, @@ -4342,6 +4401,43 @@ mod tests { } } + #[test] + fn identifies_handler_entry_shared_across_protected_regions() { + let shared = BlockId::new(7); + let private = BlockId::new(8); + let entries = shared_handler_entries([(1, shared), (1, shared), (2, shared), (2, private)]); + + assert_eq!(entries, BTreeSet::from([shared])); + } + + #[test] + fn split_try_fragments_that_share_finally_are_not_conflicting_catches() { + let mut cleanup = catch_handler(4, &[4]); + cleanup.kind = HandlerKind::Cleanup; + let left = try_region( + 1, + 0, + 4, + &[0, 1], + vec![catch_handler(3, &[3]), cleanup.clone()], + ); + let right = try_region(2, 4, 8, &[2], vec![catch_handler(3, &[3]), cleanup]); + + assert!(conflicting_shared_catch_entries(&[left, right]).is_empty()); + } + + #[test] + fn independent_tries_sharing_only_a_catch_are_conflicting() { + let catch = BlockId::new(3); + let left = try_region(1, 0, 4, &[0], vec![catch_handler(3, &[3])]); + let right = try_region(2, 4, 8, &[1], vec![catch_handler(3, &[3])]); + + assert_eq!( + conflicting_shared_catch_entries(&[left, right]), + BTreeSet::from([catch]) + ); + } + #[test] fn shared_noop_catch_continuation_is_not_inherited_by_outer_handler() { let mut cfg = CFG::new("shared_noop_catch_continuation"); diff --git a/dexdec/src/ir/exception/cleanup.rs b/dexdec/src/ir/exception/cleanup.rs index c08deeb..00b05e2 100644 --- a/dexdec/src/ir/exception/cleanup.rs +++ b/dexdec/src/ir/exception/cleanup.rs @@ -68,6 +68,7 @@ pub(super) struct CleanupRecovery<'a> { cfg: &'a CFG, values: &'a SsaValueGraph, normal_dominators: &'a DominatorTree, + shared_handler_entries: &'a BTreeSet, } impl<'a> CleanupRecovery<'a> { @@ -75,11 +76,13 @@ impl<'a> CleanupRecovery<'a> { cfg: &'a CFG, values: &'a SsaValueGraph, normal_dominators: &'a DominatorTree, + shared_handler_entries: &'a BTreeSet, ) -> Self { Self { cfg, values, normal_dominators, + shared_handler_entries, } } @@ -150,6 +153,9 @@ impl<'a> CleanupRecovery<'a> { .collect::>(); let owned = CleanupDomain::analyze(self.cfg, handler.semantic_entry, &handler.rethrow_blocks); + // Shared tails still recover as finally. Copy elision for those + // tails is withheld in CatchCleanupRecovery so one region cannot + // delete cleanup still required by another occurrence. let mut copies = BTreeSet::new(); let mut copy_blocks = BTreeSet::new(); let mut contractions = Vec::new(); @@ -263,6 +269,7 @@ impl<'a> CleanupRecovery<'a> { &owned, &handler.rethrow_blocks, self.normal_dominators, + self.shared_handler_entries, ) .recover(&catch_domains)? else { @@ -631,6 +638,7 @@ struct CatchCleanupRecovery<'a> { handler_blocks: &'a BTreeSet, rethrow_blocks: &'a BTreeSet, normal_dominators: &'a DominatorTree, + shared_handler_entries: &'a BTreeSet, } impl<'a> CatchCleanupRecovery<'a> { @@ -642,6 +650,7 @@ impl<'a> CatchCleanupRecovery<'a> { handler_blocks: &'a BTreeSet, rethrow_blocks: &'a BTreeSet, normal_dominators: &'a DominatorTree, + shared_handler_entries: &'a BTreeSet, ) -> Self { Self { cfg, @@ -651,6 +660,7 @@ impl<'a> CatchCleanupRecovery<'a> { handler_blocks, rethrow_blocks, normal_dominators, + shared_handler_entries, } } @@ -660,6 +670,12 @@ impl<'a> CatchCleanupRecovery<'a> { ) -> Result, super::ExceptionInvariantError> { let mut recovered = RecoveredCleanupCopies::default(); for domain in domains { + // Shared catch tails still allow the outer handler to become + // `finally`. Only their copies are left in place so another + // protected region cannot lose required cleanup. + if self.shared_handler_entries.contains(&domain.entry) { + return Ok(None); + } let mut proofs = BTreeMap::new(); for candidate in &domain.blocks { let specialized = CleanupSpecialization::new( @@ -2384,16 +2400,8 @@ mod tests { fn straight_line_cleanup_records_observable_argument_binding() { let handler = BlockId::new(0); let normal = BlockId::new(1); - let handler_value = RegisterArg::new_ssa( - 1, - 7, - ArgType::object("java/io/Closeable"), - ); - let normal_value = RegisterArg::new_ssa( - 12, - 3, - ArgType::object("java/io/Closeable"), - ); + let handler_value = RegisterArg::new_ssa(1, 7, ArgType::object("java/io/Closeable")); + let normal_value = RegisterArg::new_ssa(12, 3, ArgType::object("java/io/Closeable")); let mut cfg = CFG::new("cleanup_straight_line_state"); let mut handler_block = Block::new(handler); handler_block.push(InsnNode::invoke(