Scene, Stage, BootScene: real C++ destructors, and the inline-base-destructor fix that unlocks their siblings - #1391
Merged
Merged
Conversation
…cene's ten children can all reach Scene was down to one backlog file. Its immediate base ActorDerived has no inline operator delete, so mwcc had no way to inline the Memory::Deallocate call the ROM's D0 makes -- add one directly to Scene instead. That also puts it in reach of Scene's IMMEDIATE base for all ten dScene_c children (Stage, BootScene, and the eight still-unnamed siblings), the same way Enemy carries its own copy for its 51 subclasses. Byte-verified under 2004/b56 (D0 and D1 both), eligible bracket unchanged (10813/10813, same failure-category breakdown), rombuild 106/106 exact, port_refcheck clean.
The Stage migration agent found it independently: the ROM's Stage::~Stage inlines TWO vptr stores (its own, then Scene's) before calling ActorBase's D2 directly -- no call to a separate Scene::~Scene(). A merely declared `virtual ~Scene();` can't be inlined by a subclass (the compiler has no body to see) and emits `bl _ZN5SceneD2Ev` where the ROM has none. Trial compile: 80 bytes with the call, vs the ROM's 104 with no such call. Same defect class as ActorDerived's own inline destructor (already landed, load-bearing for Scene itself) -- just one level deeper. Fix is the same shape: define ~Scene() inline in the class body, and turn its D1/D0 files into forcing TUs (an uncalled function invoking the destructor / delete- expression) since they can no longer DEFINE it without redefining the inline body. Both still byte-verify exactly as before. This is what actually unlocks Stage, BootScene, and the other eight dScene_c children's D1/D0 migrations -- they were never blocked by their own layout, only by Scene's destructor being out-of-line. Byte-verified (D1 and D0 both, 2004/b56), eligible bracket unchanged (10813/10813), rombuild 106/106 exact.
BootScene (dScBoot_c) had two backlog files, both shadow-struct, and no
header of its own -- a generated skeleton existed under the ROM name
(dScBoot_c.h) but the class's functions mangle under the English coinage
BootScene, so langmode_audit saw has_header=False despite a header for the
same class technically existing under a different filename.
New include/BootScene.h follows Stage.h's precedent (named after the
coinage) as `struct BootScene : Scene`. It also corrects two layout bugs
the generated dScBoot_c.h had: offset 0x053 is a live flag, not padding
(two independent readers agree), and 0x056 is a real field the generator
never saw. sizeof asserts to 0x058, backed by the class's own operator
new(0x58) call. include/dScBoot_c.h is left in place -- InitResources and
Behavior still reach it and are out of this slice.
D1/D0 now real BootScene::~BootScene() {}, inlining Scene's now-inline
destructor the same way Stage's will. Needed one more thing beyond Scene's
own D0/D1 recipe: objisolate externalizes the compiler-synthesized
_ZTV9BootScene reference, so config/arm9/symbols.txt gets it as a data
alias at the same address as the existing data_02091528 -- the same
technique already used for _ZTV5Enemy.
Byte-verified (D1 and D0, 2004/b56), eligible bracket unchanged
(10813/10813), rombuild 106/106 exact, port_refcheck clean.
…e scan Found while giving Stage.h a real Particle::SysTracker member. The tool picked the FIRST struct-with-body in a file as its target regardless of name, so a header with a helper type declared before its main class (a namespace-nested struct, a C/C++ dual-spelling, a small file-local aggregate) had its helper checked instead of the class the file exists to verify. Confirmed tree-wide: 13 headers have this shape -- ActorBase.h, BMD_File.h, common.h, MaterialChanger.h, MeshCollider.h, MeshColliderBase.h, ModelBase.h, Sound.h, Stage.h, TextureSequence.h, TextureTransformer.h, and two private/ vtable headers. Most were reporting a silent "0 unparsed" pass on the wrong struct. Fix: only start scanning once the struct name matches the file's own stem; skip any earlier struct-with-body whole, the same way the tool already skips a NESTED one found after starting. Re-ran across every header in include/ before and after: no new mismatches anywhere, several files now get real coverage they never had (MeshCollider.h 7->14 fields checked, MeshColliderBase.h 0->8), a few resolve to "polymorphic, unmodelled" for their real class instead of a false pass on a helper, and two pre-existing spurious UNPARSED entries (MaterialChanger.h, TextureTransformer.h) turn out to have been artifacts of checking the wrong struct all along. Two more real gaps found by the same header, fixed alongside: a bare non-virtual destructor declaration (`~Name();`, no `virtual` and no typed-return prefix -- the `~` isn't in the type-name character class) fell through to UNPARSED instead of ending the field list, and a namespace-qualified field type (`Particle::SysTracker mSysTracker;`) failed to match the declaration regex at all rather than being reported as an honest unrecognised type. Remaining pre-existing UNPARSED entries (OamAttr.h's "unsigned short", Fix12.h's template parameter T) are unrelated and untouched.
PS_Init was the easy third: a static method (confirmed at the call site -- Stage::Behavior invokes it bare, no `this` in r0), migrated using the shadow-class idiom Stage::Behavior's own file already established. D1/D0 needed two things neither Scene nor BootScene did. First, Scene's destructor now being inline (previous commit) closes the gap Scene left -- Stage inlines Stage's own vptr store, then Scene's, then ActorDerived's, then calls ActorBase's D2 directly, matching the ROM's 104-byte body where the naive out-of-line-Scene version came out 80 bytes with an extra call. Stage itself does NOT need to go inline in turn: it is a leaf (no RTTI record names dScStage_c as a base), so nothing below it needs to see its body. Second, and the real work: Stage destroys three real members before that chain -- MeshCollider at 0x91c, Model at 0x86c, Particle::SysTracker at 0x50 -- and an empty destructor body only reproduces once they're typed as what they are, in the right order (C++ destroys in reverse declaration order, which is exactly the ROM's 0x91c/0x86c/0x50 sequence once declared increasing). Model and MeshCollider have known sizes. SysTracker did not -- until now: include/Particle.h and include/Particle__SysTracker.h turn out to be two separate gen_header.py shadows of the SAME class (confirmed by the actual matched functions using each), and their union gives a real, self-consistent size, 0x81c, that closes exactly on the gap Stage's own layout already implied. Declared locally in Stage.h rather than merging the two real headers, which is its own change with its own blast radius. First compile came out 1 word off: Model and MeshCollider aren't flush -- there's a real 0x60-byte gap between them (0x8bc..0x91c) the old shadow struct carried as untyped padding, silently dropped by placing the two members back to back. Restored it. Retyping unk_86c/unk_874/unk_8bc/unk_91c away broke four already-migrated methods that referenced those names directly (RenderModel, RenderModelTransparent, LoadTextureTransformers, CleanupResources) -- caught by the eligible.py bracket (10813 -> 10809), fixed by pointing each at the real members instead. C3 (the constructor) stays unmigrated -- same member-typing story now resolved, but it's an operator-new-plus-placement-construct combination rather than an in-class definition, and per the "zero constructors ever migrated" Phase-5 note it gets its own timeboxed slice rather than riding along here. Byte-verified (D1, D0, PS_Init, and all 4 ripple-affected files, 2004/b56), eligible bracket exact 10813/10813 after the fix-up, rombuild 106/106 exact, port_refcheck clean.
…ence A parallel research pass into the other nine dScene_c siblings found this one had real evidence sitting unused: _ZN11dScMgBase_cD2Ev destroys 8 elements at c+0xf4 via __destroy_arr(count=8, elem_size=0x24, ...), and the per-element destructor turns out to write two vtables back to back with no further calls -- dMgPsOpt_c::TouchIcon_c then dThIcon_c, read straight out of build/rtti.json. So 0xf4..0x214 is 8 contiguous 0x24-byte elements, not the untyped pad_0f4[0x128] the generated header carried. Neither element class has a header yet, so the array stays raw bytes rather than an invented struct type -- same restraint Stage.h uses for Particle:: SysTracker's own union-of-two-shadows reasoning. Needed one more fix to check_header_offsets: `u8 x[8][0x24];` is a 2-D array, and the declaration regex only ever had room for one bracket group, so the second bracket left `];` dangling after what should have been the line's terminating `;` and the whole line failed to match. Captures every bracket group as one blob now and multiplies the dimensions. Byte-neutral: total span 0xf4..0x21c is unchanged (0x24*8 + 8 == 0x128), nothing has ever referenced the field by name, eligible bracket exact 10813/10813, rombuild reused 10813/10813 objects from cache (0 recompiled -- confirms nothing depends on the changed bytes), 106/106 exact.
…skip_other A Fable review of the earlier fix (026dbda) caught a real regression before it shipped further: a one-line helper struct that opens AND closes on the same line -- struct BMA_File { u16 numFrames; }; in MaterialChanger.h and TextureTransformer.h -- set skip_other and then never checked THAT line for its own closing brace, only later ones. The real target struct's opening line was read while still "skipping" and silently swallowed whole. Concretely: MaterialChanger.h and TextureTransformer.h went from an honest UNPARSED failure (checking the wrong struct, at least visibly wrong) to a hollow "0 fields, 0 unparsed" pass -- checking nothing, indistinguishable from a clean result. Exactly the silent-no-op shape this whole tool exists to prevent, introduced by the commit that was fixing three other instances of it. Fix: only enter skip_other if the helper's own opening line doesn't already contain its closing brace. Re-verified across every header in include/: both files now correctly check their own real field (1 each, 0 mismatched), and MeshColliderBase.h flips from a bogus "8 fields, 0 mismatched" to the correct "skipped -- polymorphic, unmodelled" -- it turns out to have had the same one-liner-helper shape and was silently checking the wrong thing too, just landing on a number that happened to look plausible instead of on zero. No other file changed. eligible.py bracket unchanged (10813/10813, tool-only change, no source recompiled).
Vtable slot maps for dScMB_c/dScTitle_c/dScStarSel_c/dScGameOver_c/ dScMiniGm_c/dScDSMT_c/dScEntry_c/dScMgBase_c (module, overridden slots, example targets), gathered while migrating Scene/Stage/BootScene but not acted on -- all eight are already matched source sitting under func_* names, blocked only on naming/attribution, not matching. Persisted per the dtor-variant-audit.md precedent rather than left as a worktree transcript that gets torn down: the concrete finding was that a prior review of this same session's work flagged the maps as existing only as prose, at risk of costing a second session to re-derive. Also records: dScMgBase_c is a second hierarchy root (36-slot vtable, 32 further RTTI descendants -- a whole minigame family, 24 already headered), the strongest lead for the next naming pass; and a tree-wide comment defect -- every D0 (slot 17) "recovered name" comment across all eight classes mislabels the deleting destructor as OnYoshiTryEat, while the D1 (slot 16) comments on the same files are accurate. Scope beyond these eight not yet checked.
✅ PR validation — PassedCommitted merge introduces no reconstruction or attribution regression. Full merge validation
Warnings: 1 linkcheck result(s) have unresolved relocations; 2 affected source file(s) could not be fully link-checked. Per-file link-check detail2 of 62 changed file(s) do not match the ROM (NO-SYM).
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. |
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.
Summary
Migrates
Scene,Stage, andBootSceneto real, byte-verified C++ classes, and lands the fix that unlocks the same route for the other seven ofdScene_c's ten direct children (RTTI-confirmed hierarchy, matchesScene.h's own census).operator deleteon Scene, which also puts it in reach of every one of Scene's ten children as their immediate base.include/ActorDerived.halready documents one level up). Found independently by a parallel migration attempt onStage.PS_Init). Required derivingParticle::SysTracker's real size by cross-referencing two independently-named auto-generated headers of the same undocumented class — their union closes exactly on Stage's own layout gap.dScMgBase_cgains one real typed member (an 8-element array), evidenced from its own destructor's__destroy_arrcall and identified via the ROM's RTTI graph.tools/check_header_offsets.py: three latent bugs found and fixed (one caused by this session, two pre-existing and affecting 13 headers tree-wide, most silently checking the wrong struct in the file). A regression in the first fix was caught by a second-pass review before merge and fixed in the same PR.notes/dscene-c-siblings-census.md: vtable slot maps for the eightdScene_cchildren that have never had a single function named — all already byte-matched underfunc_*names, blocked only on attribution. Also flags a tree-wide comment defect (D0 destructors mislabeled_OnYoshiTryEatin ~700 files' auto-generated comments).Verification
Every commit individually byte-verified (
build_pin.verify, 2004/b56) and gated with the full suite before the next commit:eligible.pybracketed before/after every header edit — exact 10813/10813 throughout, no swapsrombuild.py -j16(never--no-rom) — 106/106 exact, 100% of compared bytes, on every commitport_refcheck.py— clean throughoutprepush_attribution.py— 0 changed, 0 lostcheck_references.py— re-run fresh at final HEAD; the one failure it reports (Whomp::InitResources,func_02057410, an ov091 symbol) is confirmed pre-existing and unrelated by reproducing identically on a scratch worktree at pristineorigin/mainwith zero changes appliedTest plan
rombuild.py -j16106/106 exact at final HEADeligible.pybracket unchanged (10813/10813) across every header editcheck_header_offsets.pyre-run across every header ininclude/, before/after, zero new mismatchesport_refcheck.pycleancheck_references.pyfailure confirmed pre-existing on pristineorigin/main🤖 Generated with Claude Code