From 4739dc58124c8c92834f0ccca88e39ea0b22dd1f Mon Sep 17 00:00:00 2001 From: John Stiles Date: Sun, 30 Aug 2026 17:35:57 -0400 Subject: [PATCH 1/3] split: join unclaimed data ranges into their sole referencing unit create_gap_splits() currently buckets every address range not covered by an explicit split entry into a generic auto_XX_ADDR_section unit, even when every relocation inside that range points into exactly one already-split function (e.g. an anonymous jump table living in its own .rodata gap, entirely referenced by one .text function). Since that jump table's home object is a different translation unit than the function's, its entries can't use a GAS local (.L_) label to address something in another object - the only cross-object-visible name available is the enclosing function's own symbol, so write_asm's existing label synthesis (util/asm.rs) falls back to plain `funcName+offset` for every entry, including ones that alias another label's exact address. That's what breaks m2c's switch/jump-table handling downstream (matt-kempster/m2c#360) - m2c has to special-case parsing `symbol+offset` jtbl entries because dtk's own asm output never had a real label to give it in the first place. Add single_referencing_unit(), which returns the split unit already proven to be the sole address-owner of a candidate range (skipping relocations that don't yet resolve to a known split, and refusing to propose a unit that already owns a non-adjacent piece of this same section - ObjInfo::add_split merges same-unit/same-section splits by taking their min..max span, which is only correct for genuinely adjacent pieces). A generic gap can be large and contain many unrelated anonymous blobs, not just one function's data, so the whole-gap check alone rarely fires on real projects. ownership_run_end() finds the largest symbol-aligned prefix of a gap that agrees on one owner, evaluating ownership per symbol (a small, independent range) rather than re-probing single_referencing_unit over a growing prefix - the latter would hard-fail permanently on the first unresolvable relocation anywhere in the gap, however far it sits from the actual data in question. create_gap_splits() now uses this to both narrow a gap's boundary and name the resulting split in one step, instead of always minting a fresh auto_ unit. Verified against a real GameCube retail DOL (a community decomp project's full ~8MB main.dol, 3694 functions, 679 objects at baseline): output is unchanged except for 4 previously-generic auto_XX units being absorbed into their real owning units (679 -> 675 objects; identical total .fn/.obj symbol count; no new "Unsplit data" or other split_obj/validate_splits errors on a full run). The motivating jump table - previously 21 entries of `.4byte fn_800EB828+0x38` etc., all in a separate auto data unit - now lives in the same unit as the function and emits real `.rel fn_800EB828, .L_800EB860`-style relocations against real local labels, including for entries that alias another entry's exact target address. --- src/util/split.rs | 116 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 110 insertions(+), 6 deletions(-) diff --git a/src/util/split.rs b/src/util/split.rs index 14650e69..284dc4a9 100644 --- a/src/util/split.rs +++ b/src/util/split.rs @@ -740,17 +740,42 @@ fn create_gap_splits(obj: &mut ObjInfo) -> Result<()> { .filter(|(_, s)| s.address == current_address.address as u64) .collect_vec(), ); + // Narrow the split further if a prefix of it is solely owned by one + // already-known unit (e.g. a jump table referenced by one function). + let owned = ownership_run_end( + obj, + section, + &symbols, + current_address.address, + new_split_end.address, + ); + if let Some((owned_end, _)) = &owned { + if *owned_end < new_split_end.address { + new_split_end.address = *owned_end; + } + } + log::debug!( "Creating split from {:#010X}..{:#010X}", current_address, new_split_end ); - let unit = format!( - "auto_{:02}_{:08X}_{}", - current_address.section, - current_address.address, - section.name.trim_start_matches('.') - ); + let unit = owned + .map(|(_, unit)| unit) + // Don't reuse a unit this same pass already joined (add_split() merge risk). + .filter(|unit| { + !new_splits.iter().any(|(addr, s)| { + addr.section == current_address.section && &s.unit == unit + }) + }) + .unwrap_or_else(|| { + format!( + "auto_{:02}_{:08X}_{}", + current_address.section, + current_address.address, + section.name.trim_start_matches('.') + ) + }); new_splits.insert(current_address, ObjSplit { unit: unit.clone(), end: new_split_end.address, @@ -1772,6 +1797,85 @@ pub fn end_for_section(obj: &ObjInfo, section_index: SectionIndex) -> Result Option { + let mut found: Option<&str> = None; + for (_, reloc) in section.relocations.range(start..end) { + let target = &obj.symbols[reloc.target_symbol]; + let target_section_idx = target.section?; + let target_section = obj.sections.get(target_section_idx)?; + let (_, split) = target_section.splits.for_address(target.address as u32)?; + match found { + None => found = Some(split.unit.as_str()), + Some(unit) if unit == split.unit => {} + // Referenced by 2+ distinct already-known units: ambiguous, don't guess. + Some(_) => return None, + } + } + let unit = found?; + if section.splits.for_unit(unit).ok()?.is_some() { + return None; + } + Some(unit.to_string()) +} + +/// Finds the largest symbol-aligned prefix of `[start, limit)` owned by exactly one known unit, +/// so a jump table etc. buried in an otherwise-mixed gap can still be joined without requiring +/// the whole (possibly huge) gap to agree. `symbols` is every symbol in range, address order. +/// +/// Evaluates ownership per symbol rather than re-probing [`single_referencing_unit`] over a +/// growing prefix: that function hard-fails a whole range on its first unresolved relocation, +/// which would permanently poison every later, cleanly-owned symbol too. A symbol with no +/// resolvable owner is just "no evidence" and doesn't break an already-established run. +fn ownership_run_end( + obj: &ObjInfo, + section: &ObjSection, + symbols: &[(SymbolIndex, &ObjSymbol)], + start: u32, + limit: u32, +) -> Option<(u32, String)> { + let mut owner: Option = None; + let mut end: Option = None; + for (i, &(_, symbol)) in symbols.iter().enumerate() { + let sym_start = symbol.address as u32; + if sym_start < start { + continue; + } + let sym_end = symbols.get(i + 1).map(|&(_, s)| s.address as u32).unwrap_or(limit); + match single_referencing_unit(obj, section, sym_start, sym_end) { + Some(unit) => match &owner { + None => { + owner = Some(unit); + end = Some(sym_end); + } + Some(o) if *o == unit => end = Some(sym_end), + // A different already-known owner: stop before this symbol. + Some(_) => break, + }, + None => { + // No evidence either way; extend an already-started run over it, but don't + // start a run on a neutral symbol alone. + if owner.is_some() { + end = Some(sym_end); + } + } + } + } + end.zip(owner) +} + /// Generates a unit name for an autogenerated split. /// The name is based on the symbol name and section name. /// If the name is not unique, a number is appended to the end. From 02a57c27aebf8e09cec1b9c17f4e7adfdd05b0a1 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Fri, 4 Sep 2026 14:24:21 -0400 Subject: [PATCH 2/3] split: tighten comments in create_gap_splits/single_referencing_unit/ownership_run_end Trim inline and doc comments to be more concise, and move the add_split-merge rationale out of single_referencing_unit's doc comment since it's an implementation detail rather than part of the function's contract. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_018n9Wm7wVbnqadSbbThVc7c --- src/util/split.rs | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/src/util/split.rs b/src/util/split.rs index 284dc4a9..ff154813 100644 --- a/src/util/split.rs +++ b/src/util/split.rs @@ -740,8 +740,7 @@ fn create_gap_splits(obj: &mut ObjInfo) -> Result<()> { .filter(|(_, s)| s.address == current_address.address as u64) .collect_vec(), ); - // Narrow the split further if a prefix of it is solely owned by one - // already-known unit (e.g. a jump table referenced by one function). + // Identify and claim prefixes that have a single owner. let owned = ownership_run_end( obj, section, @@ -762,7 +761,7 @@ fn create_gap_splits(obj: &mut ObjInfo) -> Result<()> { ); let unit = owned .map(|(_, unit)| unit) - // Don't reuse a unit this same pass already joined (add_split() merge risk). + // Skip units already claimed in this section, to prevent add_split from merging them. .filter(|unit| { !new_splits.iter().any(|(addr, s)| { addr.section == current_address.section && &s.unit == unit @@ -1798,13 +1797,8 @@ pub fn end_for_section(obj: &ObjInfo, section_index: SectionIndex) -> Result found = Some(split.unit.as_str()), Some(unit) if unit == split.unit => {} - // Referenced by 2+ distinct already-known units: ambiguous, don't guess. + // Referenced by multiple distinct units; there's no single owner. Some(_) => return None, } } @@ -1831,14 +1825,8 @@ fn single_referencing_unit( Some(unit.to_string()) } -/// Finds the largest symbol-aligned prefix of `[start, limit)` owned by exactly one known unit, -/// so a jump table etc. buried in an otherwise-mixed gap can still be joined without requiring -/// the whole (possibly huge) gap to agree. `symbols` is every symbol in range, address order. -/// -/// Evaluates ownership per symbol rather than re-probing [`single_referencing_unit`] over a -/// growing prefix: that function hard-fails a whole range on its first unresolved relocation, -/// which would permanently poison every later, cleanly-owned symbol too. A symbol with no -/// resolvable owner is just "no evidence" and doesn't break an already-established run. +/// Finds the largest possible prefix of `[start, limit)` that is owned by exactly one known unit. +/// This lets us identify a jump table that starts at `start`. fn ownership_run_end( obj: &ObjInfo, section: &ObjSection, @@ -1861,12 +1849,11 @@ fn ownership_run_end( end = Some(sym_end); } Some(o) if *o == unit => end = Some(sym_end), - // A different already-known owner: stop before this symbol. + // A different owner; stop the search here. Some(_) => break, }, None => { - // No evidence either way; extend an already-started run over it, but don't - // start a run on a neutral symbol alone. + // Unknown provenance: only allowed if we've already started a run. if owner.is_some() { end = Some(sym_end); } From aa790c2770ac8f81d9fd6871324ecaf72615dd42 Mon Sep 17 00:00:00 2001 From: John Stiles Date: Fri, 4 Sep 2026 18:05:54 -0400 Subject: [PATCH 3/3] to be rewritten: - create_gap_splits() joined an unclaimed range into its referencing unit on relocation evidence alone, never checking link order - resolve_link_order() needs a unit's chunks in every section to agree on one global position; 4 of 37 joins on a real ~1300-object project contradicted that and made it cyclic - moved the graph construction into link_order_graph(), which can also take not-yet-applied splits plus a candidate - create_gap_splits() now rejects a join that would make the order cyclic and falls back to a plain auto_ split with no boundary trim Apologies for the Claude-written prose in the previous version of this message on a human-facing PR; that was uncalled for. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_018n9Wm7wVbnqadSbbThVc7c --- src/util/split.rs | 141 +++++++++++++++++++++++++++++++--------------- 1 file changed, 96 insertions(+), 45 deletions(-) diff --git a/src/util/split.rs b/src/util/split.rs index ff154813..fb5ba891 100644 --- a/src/util/split.rs +++ b/src/util/split.rs @@ -747,7 +747,18 @@ fn create_gap_splits(obj: &mut ObjInfo) -> Result<()> { &symbols, current_address.address, new_split_end.address, - ); + ) + // Skip units already claimed in this section, to prevent add_split from merging them. + .filter(|(_, unit)| { + !new_splits + .iter() + .any(|(addr, s)| addr.section == current_address.section && &s.unit == unit) + }) + // A unit's chunks in other sections already fix its place in the link order; + // claiming a range here that contradicts that would make the order cyclic. + .filter(|(_, unit)| { + link_order_is_acyclic(obj, &new_splits, Some((current_address, unit.as_str()))) + }); if let Some((owned_end, _)) = &owned { if *owned_end < new_split_end.address { new_split_end.address = *owned_end; @@ -759,22 +770,14 @@ fn create_gap_splits(obj: &mut ObjInfo) -> Result<()> { current_address, new_split_end ); - let unit = owned - .map(|(_, unit)| unit) - // Skip units already claimed in this section, to prevent add_split from merging them. - .filter(|unit| { - !new_splits.iter().any(|(addr, s)| { - addr.section == current_address.section && &s.unit == unit - }) - }) - .unwrap_or_else(|| { - format!( - "auto_{:02}_{:08X}_{}", - current_address.section, - current_address.address, - section.name.trim_start_matches('.') - ) - }); + let unit = owned.map(|(_, unit)| unit).unwrap_or_else(|| { + format!( + "auto_{:02}_{:08X}_{}", + current_address.section, + current_address.address, + section.name.trim_start_matches('.') + ) + }); new_splits.insert(current_address, ObjSplit { unit: unit.clone(), end: new_split_end.address, @@ -1227,54 +1230,74 @@ pub fn update_splits(obj: &mut ObjInfo, common_start: Option, fill_gaps: bo Ok(()) } -/// The ordering of TUs inside of each section represents a directed edge in a DAG. -/// We can use a topological sort to determine a valid global TU order. -/// There can be ambiguities, but any solution that satisfies the link order -/// constraints is considered valid. -#[instrument(level = "debug", skip(obj))] -fn resolve_link_order(obj: &ObjInfo) -> Result> { - #[allow(dead_code)] - #[derive(Debug, Copy, Clone)] - struct SplitEdge { - from: i64, - to: i64, +/// Builds the link order dependency graph from every split in `obj`, plus `extra` splits not yet +/// applied to `obj` and an optional `candidate` (address, unit) split. Returns the adjacency +/// list and the unit name for each node index. +fn link_order_graph<'a>( + obj: &'a ObjInfo, + extra: &'a BTreeMap, + candidate: Option<(SectionAddress, &'a str)>, +) -> Result<(Vec>, Vec<&'a str>)> { + // Per section: (address, unit, common), merged and sorted by address + let mut sections = vec![]; + for (section_index, section) in obj.sections.iter() { + let mut entries = section + .splits + .iter() + .map(|(addr, split)| (addr, split.unit.as_str(), split.common)) + .chain( + extra + .iter() + .filter(|(addr, _)| addr.section == section_index) + .map(|(addr, split)| (addr.address, split.unit.as_str(), split.common)), + ) + .chain( + candidate + .filter(|(addr, _)| addr.section == section_index) + .map(|(addr, unit)| (addr.address, unit, false)), + ) + .collect_vec(); + entries.sort_by_key(|&(addr, _, _)| addr); + sections.push((section.name.as_str(), entries)); } let mut unit_to_index_map = BTreeMap::<&str, usize>::new(); let mut index_to_unit = vec![]; - for (_, _, _, split) in obj.sections.all_splits() { - unit_to_index_map.entry(split.unit.as_str()).or_insert_with(|| { - let idx = index_to_unit.len(); - index_to_unit.push(split.unit.as_str()); - idx - }); + for (_, entries) in §ions { + for &(_, unit, _) in entries { + unit_to_index_map.entry(unit).or_insert_with(|| { + let idx = index_to_unit.len(); + index_to_unit.push(unit); + idx + }); + } } let mut graph = vec![vec![]; index_to_unit.len()]; - for (_section_index, section) in obj.sections.iter() { - let mut iter = section.splits.iter().peekable(); - if section.name == ".ctors" || section.name == ".dtors" { + for (section_name, entries) in §ions { + let mut iter = entries.iter().peekable(); + if *section_name == ".ctors" || *section_name == ".dtors" { // Skip __init_cpp_exceptions.o let skipped = iter.next(); log::debug!("Skipping split {:?} (next: {:?})", skipped, iter.peek()); } - while let (Some((a_addr, a)), Some(&(b_addr, b))) = (iter.next(), iter.peek()) { - if !a.common && b.common { + while let (Some(&(a_addr, a_unit, a_common)), Some(&&(b_addr, b_unit, b_common))) = + (iter.next(), iter.peek()) + { + if !a_common && b_common { // This marks the beginning of the common BSS section. continue; } - if a.unit != b.unit { + if a_unit != b_unit { log::debug!( "Adding dependency {} ({:#010X}) -> {} ({:#010X})", - a.unit, + a_unit, a_addr, - b.unit, + b_unit, b_addr ); - let a_index = *unit_to_index_map.get(a.unit.as_str()).unwrap(); - let b_index = *unit_to_index_map.get(b.unit.as_str()).unwrap(); - graph[a_index].push(b_index); + graph[unit_to_index_map[a_unit]].push(unit_to_index_map[b_unit]); } } } @@ -1299,6 +1322,34 @@ fn resolve_link_order(obj: &ObjInfo) -> Result> { graph[a_index].push(b_index); } + Ok((graph, index_to_unit)) +} + +/// Whether the link order would still be resolvable with `extra` splits and `candidate` added. +fn link_order_is_acyclic( + obj: &ObjInfo, + extra: &BTreeMap, + candidate: Option<(SectionAddress, &str)>, +) -> bool { + link_order_graph(obj, extra, candidate).is_ok_and(|(graph, _)| toposort(&graph).is_ok()) +} + +/// The ordering of TUs inside of each section represents a directed edge in a DAG. +/// We can use a topological sort to determine a valid global TU order. +/// There can be ambiguities, but any solution that satisfies the link order +/// constraints is considered valid. +#[instrument(level = "debug", skip(obj))] +fn resolve_link_order(obj: &ObjInfo) -> Result> { + #[allow(dead_code)] + #[derive(Debug, Copy, Clone)] + struct SplitEdge { + from: i64, + to: i64, + } + + let no_extra = BTreeMap::new(); + let (graph, index_to_unit) = link_order_graph(obj, &no_extra, None)?; + match toposort(&graph) { Ok(vec) => Ok(vec .iter()