Join unclaimed data ranges into their referencer - #147
Open
StilesCrisis wants to merge 4 commits into
Open
Conversation
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.
…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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018n9Wm7wVbnqadSbbThVc7c
- 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018n9Wm7wVbnqadSbbThVc7c
Require that a joined range contain relocations, that all of them target the owning unit, and that every reference to the range comes from that unit's code. Data that is only referenced from one unit but points nowhere is no longer claimed, since a Matching unit's source may merely extern it. Skip code sections, keep run boundaries 4-byte aligned, and when a gap does not begin with an owned symbol, end the auto split at the first one that is so the next pass can claim it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018n9Wm7wVbnqadSbbThVc7c
Author
|
@encounter Sorry for the churn! Please take a look when you're able. It's larger than before, sorry, but this bullet-proofs it against making an invalid/cyclical link graph. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When decomping a function with a switch, the switch's jump table can sometimes land far away from the code in a different split. This prevented labels inside the function body from being represented properly; instead, the jump table would represented each location as myfunction + offset.
This isn't broken, but it's suboptimal, and it also prevents tools like m2c from decompiling the switch at all; it asserts because it expects the jump table to point directly at real labels.
This PR adds new logic which detects single-owner relocation tables like this, and ensures that they travel along with the function itself.
This is a significant rewrite of #146. The previous version was too permissive and would end up making link cycles. This version is much more strict about only operating on sections which are clearly jump-table shaped. I've re-tested it against my entire project and confirmed that it fixes all the known-busted switch statements in my code, without causing any breakage elsewhere.