Join unclaimed data ranges into their referencer (when only one referencer exists) - #146
Closed
StilesCrisis wants to merge 3 commits into
Closed
Join unclaimed data ranges into their referencer (when only one referencer exists)#146StilesCrisis wants to merge 3 commits into
StilesCrisis wants to merge 3 commits into
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.
StilesCrisis
force-pushed
the
join-anon-data-to-referencing-unit
branch
from
August 30, 2026 21:45
6dcfcd2 to
4739dc5
Compare
StilesCrisis
changed the title
StilesCrisis
changed the title
Author
|
@encounter Friendly ping, do you have any questions about this PR? I'm running with it locally and it's improved output for some switches. |
Owner
|
Thanks for the PR! It looks directionally okay to me. Could you replace every comment with a hand-written one that better explains the logic? They read very Claude to me |
Author
|
Absolutely, I can do that. Thanks for getting back to me :) |
…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
Author
|
Done. |
Author
|
Actually, please hold off on merging this briefly. Re-running it against my project, I think there might be a bug. I'll keep you posted. |
- 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
StilesCrisis
force-pushed
the
join-anon-data-to-referencing-unit
branch
from
September 4, 2026 22:08
d745249 to
aa790c2
Compare
Author
|
Yeah, this has a bug when running it against a sufficiently large codebase. It ends up causing “cyclic reference” errors. I’ll keep digging, but will close this for now. |
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
2 participants
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 asmyfunction + 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. In the project that I'm working on, this allowed 4 previously-generic
auto_XXunits to be absorbed into their real owning units (concretely, from 679 to 675 objects), meaning four new switch statements that will now m2c properly.Separately, I sent a PR to the m2c repo which solved the issue on their end, and they asked me to investigate a dtk fix first. (That's at matt-kempster/m2c#360 )