Preserve manifest-owned section order for intact C++ TUs - #2103
Merged
Conversation
✅ PR validation — PassedCommitted merge passes; 6 contributor credit reassignment(s) noted, not a blocker. Full merge validation
Contributor credit moved (6)
Byte-verified means the range carries Warnings: contributor attribution changed, not a blocker (6 changed, 0 lost -- src/_ZN11dScMgBase_c24OnHitByCannonBlastedCharEv.c: tangosdev -> andrewboudreau; src/_ZN14dScMgD3DBase_c15OnHitByMegaCharEv.c: ruspecial -> andrewboudreau; src/_ZN14dScMgD3DBase_c24OnHitByCannonBlastedCharEv.c: tangosdev -> andrewboudreau; +3 more); 5 linkcheck result(s) have unresolved relocations. Per-file link-check detailAll 1202 changed file(s) compile to the ROM byte-for-byte with correct relocation targets.
The private worker commits a test merge, builds the stock ROM profile, compares every executable module, measures matched and source-built code, checks contributor lineage, and verifies affected relocations. The mod profile is opt-in and is not part of this merge gate. |
andrewboudreau
added
the
attribution-override
andrewboudreau
commented
Aug 31, 2026
andrewboudreau
left a comment
andrewboudreau
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed at 589dddc74, in a wired worktree with the real toolchain. No changes requested. One ask on the description, one latent note, and a stack-ordering blocker that is not yours.
The references red is cleared
I had a finding queued for the config/manifest docstring — the dead-references gate reads that slash as a repo-rooted path. 589dddc74 already fixed it. Re-measured across refs to be sure the fix is real and complete: origin/main 0, origin/cpp/flyguy-genuine-tu 0, origin/tools/intact-tu-section-order 0, origin/cpp/rccarpet-intact-tu 0. Nothing owed here.
The one verdict-changing hunk is not in the title
_relocation_module_set is the only place this PR changes what verify_owned_sections will accept, and the PR reads as a section-order change. Please say so in the description — a reviewer skimming for "does this relax a gate" should not have to find it in the diff.
Having found it: it is a bugfix, not a loosening, and I can show that rather than assert it.
RL.normalize_module('overlays(0,4)') -> 'overlays(0,4)' # unchanged
RL.normalize_module('overlay(2)') -> 'ov002'
The old comparison was candidate_module != expected["target_module"], where the candidate is always a concrete ovNNN. A group spelling normalizes to itself, so that inequality was unconditionally true for every overlays(...) row — a guaranteed WRONG-MODULE that no correct input could ever clear. That is not a rare corner: the spelling is all over the tracked config (706 rows in config/arm9/overlays/ov006/relocs.txt alone), with groups running up to 22 members.
The replacement is bounded in the two ways that matter:
- manifest and config must name the identical set, so it is a license, not a wildcard;
- the target address comparison is untouched, and the address is the byte-relevant half. Only module metadata is relaxed, to exactly the ambiguity dsd itself emits.
And the singleton path is provably behaviour-preserving, not merely tested to be: {a} != {b} is a != b, and c not in {a} is c != a. Every ordinary spelling takes the same branch it took before.
The three tests pin the right properties — same group both sides, candidate must be a member, different configured group rejected.
Latent, not an ask
_MULTI_OVERLAY_MODULE has no whitespace tolerance, so overlays(2, 7) falls through to the singleton branch and becomes a module string nothing can ever match. That is fail-closed — a false refusal, not a false pass — and I measured zero spaced rows in the tracked config, so it costs nothing today. \s* around the comma would make it robust if dsd's formatter ever changes.
The reorder itself
Read reorder_same_named_nontext_sections line by line against the ELF layout. The field offsets are right: Elf32_Sym.st_shndx at +14, Elf32_Shdr.sh_link at 0x18, sh_info at 0x1c.
The part worth calling out is that sh_info is remapped only when it genuinely holds a section index — RelocationSection or SHF_INFO_LINK. A blanket remap would have silently corrupted .symtab, where sh_info means the local-symbol count, and nothing downstream would have said so. Getting that distinction right is the whole difficulty of this function.
The refusal set is genuinely fail-closed rather than decorative: SHT_GROUP, SHT_SYMTAB_SHNDX, SHN_XINDEX, non-32-bit, non-ET_REL, program headers present, e_shentsize != 40, sh_entsize != 16, .text, duplicate groups, non-permutation index lists, mixed content types. Payloads never move; only headers and the indices that name them do.
CI will not exercise any of that — so I ran it
test_objisolate.Isolate carries @unittest.skipUnless(_compiler(), "mwccarm not present"), and both new test_tubuild order tests open with if not _toolchain(): return. CI has no mwccarm. So of the nine new tests, five skip in CI — including every one that moves real bytes in a real object. Only the three module-set tests and the two tu_production tests actually run there. Pre-existing convention, not something you introduced, but it means a green check on this PR is not evidence about the reorder.
Ran in a wired worktree at 589dddc74, with test_tubuild._toolchain() asserted True first so I was not reading my own skips as passes:
tools/test_objisolate.py 37 passed
tools/test_tu_production.py 20 passed
tools/test_tubuild.py 63 passed (81s)
What I liked
assert [symbols[name]["st_shndx"] for name in owned] != retained, (
"the real fixture must retain RcCarpet's measured RTTI/resource inversion")The RcCarpet fixture asserts that it still exhibits the defect it exists to test. If a future change makes the inversion stop occurring, this test fails loudly instead of passing vacuously forever. This tree has repeatedly been bitten by tests that quietly stopped testing anything; that line is the antidote, and I would like to see more of it.
Ordering — this cannot land yet, and not because of anything here
The stack is #2096 → #2103 → #2104.
#2096 still carries CHANGES REQUESTED from me over the two coined symbol names. Re-measured at 28587e4e5: FlyGuy_VT7 and FlyGuy_VT14 are still in config/arm9/overlays/ov070/symbols.txt and in the manifest. A coined name blocks TU promotion and blocks data verification, so that has to clear at the base before anything above it moves.
#2104 is cut from this branch and inherits all of the above, so it lands third.
On content, this one is ready. Holding purely on the base.
This was referenced Sep 1, 2026
andrewboudreau
force-pushed
the
tools/intact-tu-section-order
branch
from
September 1, 2026 02:49
589dddc to
6ef413a
Compare
Collaborator
Author
|
Re-cut the five reviewed commits directly onto current main (2f62cd8), removing the FlyGuy/source-stack dependency. Range-diff is patch-identical across all five commits; the PR now contains only the original six tooling/test files. Fresh validation on head 6ef413a: py_compile passed; objisolate/tu_production/tubuild suites 118/118; full production build 11,088/11,088 with zero mismatches, 2/2 source-owned data claims, 106/106 modules, and exact stock ROM; port references 405/405; diff clean. The nine symbol diagnostics were identical in the strict control and candidate. |
This was referenced Sep 1, 2026
andrewboudreau
commented
Sep 1, 2026
andrewboudreau
left a comment
andrewboudreau
left a comment
Collaborator
Author
There was a problem hiding this comment.
Hold lifted — it was on a base that no longer exists. Merging.
My earlier hold on this PR was that it sat on #2096's coined FlyGuy names. That's stale: this branch is now cut on main and carries tools-only changes.
git log origin/main..origin/tools/intact-tu-section-order -- include src config
(empty)
The only FlyGuy_VT7 / FlyGuy_VT14 strings anywhere in this tree are in tools/test_tubuild_vtable_partitions.py, which is already on main and untouched here. Nothing in this PR asserts a disputed name.
Measured on the real merge tree
base origin/main @ 2f62cd8708eb
merge tree 24ec5a1d5d74
converted-ratchet pass -> pass langmode-ratchet pass -> pass
dead-references pass -> pass layout-check pass -> pass
duplicate-sources pass -> pass src-tu-refs pass -> pass
header-offsets pass -> pass source-coverage pass -> pass
Because this changes tubuild.py and tu_production.py — the tooling every existing intact TU on main goes through — static gates aren't enough. Full build of main + this PR:
source-built functions: 11,088 reproducing: 11,088 mismatching: 0
module fidelity: 106/106 exact, 100.000000%
intact TU gates: dsd modules PASS, zero new symbol errors, storage aliases exact
ROM-build analysis: PASS
No existing promotion regresses under the new section ordering.
The RcCarpet fixture change is the right call
be8e77210 swaps a hard-coded src_tu/actors/daObjRcCarpet_c.cpp for a manifest lookup. That's what keeps the test alive across the eventual src_tu/ → src/ promotion instead of going red the day RcCarpet lands. Good change, independent of how #2104's naming dispute resolves.
One measurement worth recording
I tried to retarget #2125 (the Kaitendai TU) straight onto main to avoid stacking it behind this. It does not build without this PR:
isolate: intact TU preparation refused: ov047/daObjKm3_Kaitendai_c
licensed .data symbol _ZTI20daObjKm3_Kaitendai_c links at 0x02112334,
manifest emitted address says 0x02112328
.data relocation 0x02112330 WRONG-SYMBOL ... (8 more)
_ZTI and data_ov047_02112334 land at each other's addresses and every .data relocation follows the swap. That is exactly the defect this PR fixes, so the stacking is a genuine dependency, not convenience — worth stating because path-disjointness would have suggested otherwise.
Sign-off is a comment because this repo has one shared account and no PR here can carry a formal approval. Merging with a merge commit rather than a squash so #2125 retargets cleanly.
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.
Rooted directly on current main; the previous FlyGuy stack dependency has been removed.\n\nAdds fail-closed manifest-driven ordering for repeated owned non-text input sections before intact-TU verification and production. It preserves payload and relocation bytes, remaps ELF section references, and rejects unsupported or incomplete layouts.\n\nThis PR also deliberately changes one verifier acceptance path: _relocation_module_set compares composite overlay destinations as identical bounded module sets. The old scalar comparison unconditionally rejected valid overlays(...) rows after the candidate normalized to a concrete ovNNN. The new path still requires the manifest/config sets to be identical, requires the concrete target module to be a member, and leaves target-address checking unchanged; singleton behavior is unchanged.\n\nFresh current-main validation at 6ef413a:\n- patch-identical five-commit range-diff from the reviewed head\n- objisolate, tu_production, and tubuild suites: 118/118 passed with the wired compiler\n- full production: 11,088/11,088 functions, 2/2 source-owned data claims, 106/106 modules, zero mismatches, exact stock ROM\n- strict control: identical stock ROM and the same nine pre-existing symbol diagnostics\n- 97/97 C++ TUs compile; port references 405/405\n\nNo claims, attempt-ledger, source, or dashboard artifacts are included.