StarSwitch and FloatingFloorLllBig derive from Platform, and three things that stop the other 41 - #1372
Conversation
✅ PR validation — PassedCommitted merge introduces no reconstruction or attribution regression. Full merge validation
Warnings: 1 linkcheck result(s) have unresolved relocations. Per-file link-check detailAll 19 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. |
Scene::~Scene stores TWO vptrs and then calls ActorBase's destructor:
str r2, [r4] ; _ZTV5Scene
str r1, [r4] ; _ZTV12ActorDerived <- ActorDerived's D2, INLINED
bl ActorBase::~ActorBase
A merely declared `virtual ~ActorDerived();` cannot produce that. The compiler
has no body to inline and emits `bl _ZN12ActorDerivedD2Ev` -- one store where
the ROM has two. Define it in the class body and Scene::~Scene() {} reproduces
0x0202e140 exactly. So the original sources defined these destructors inline,
and every derived destructor inlined them. That is worth knowing because
roughly 60 unmigrated D1 files have this two-vtable-store shape.
Making it true cost three changes, and each one was forced by the next gate
rather than chosen up front.
1. objisolate: CORRECT AN UNDEF VTABLE REFERENCE, NOT JUST AN EXTERNALISED ONE
An inlined base destructor stores a vptr for a class this object never
defines, so `_ZTV12ActorDerived` is UNDEF from the start with addend 8 --
never a candidate for externalisation, so the existing correction never
looked at it, and the guard refused the file.
objisolate PREDICTED THIS. The comment above the UNDEF branch names both the
constructor-only TU and "a derived destructor over an inline base
destructor", says both were reproduced under 2004/b56, and says they "arrive
the moment a real-C++ constructor is enrolled, which is the direction this
tree is moving". It refused because there was no enrolled instance to verify
a correction against. Scene is that instance.
The correction is the same arithmetic the externalise path already uses --
the ROM's _ZTV symbol IS the slot array, so addend 8 becomes 0 -- and it is
checked the same way, by rombuild byte-comparing the linked module. That is
the only thing that caught the original 8-high vptr bug across 34 modules.
ANY OTHER ADDEND IS STILL REFUSED, with a new test pinning that: multiple
inheritance produces addend 44 and there is still no instance for it.
The two tests asserting the old refusal now assert the correction, and a
third asserts the refusal that remains.
2. eligible.py: STB_LOPROC IS A DEFINITION
An inline function's out-of-line copy is emitted under mwcc's COMDAT
binding, STB_LOPROC. The symbol scan accepted STB_GLOBAL and STB_WEAK only,
so ActorDerived's own D1 -- defined, right size, in the kept section --
reported "0 defined global functions".
It is deduplication metadata, not a weaker definition, and exactly one
object in this build defines any given address. Accepting it also recovered
SIX functions that were invisible for the same reason and had nothing to do
with this slice: the _ZThn80_ virtual thunks of ModelAnim, ModelAnim2 and
BlendModelAnim, whose multiple inheritance makes them inline copies too.
3. ActorDerived's D1 file carries a FORCING TU
With the definition in the header that file cannot define it again, and a TU
that merely includes the header emits nothing. An explicit destructor call
in an uncalled function forces the out-of-line copy; objisolate drops it.
RESULT
eligible.py 10805 -> 10811, nothing lost (+6, all _ZThn80_ thunks)
rombuild -j16 106/106 exact, 0 mismatching
source-built 10,805 -> 10,811, 87.82% -> 87.83%
test_objisolate 6 passed
port_refcheck 393 references, 0 stale
langmode ratchet PASS
no --no-verify
NOT DONE HERE. Stage is the next one in this chain and needs its three members
typed first -- Particle::SysTracker at 0x50, Model at 0x86c, MeshCollider at
0x91c are still u8 markers. And ModelAnim's family still cannot be isolated:
that is the addend-44 refusal above, deliberately left standing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XxDmkQ47fWa3GB6mj8xEQe
72 classes derive from Platform -- more than from anything but Actor itself --
and its header was still a rung-0 generated skeleton: a flat struct of u8 pad
blocks with no base, no vtable and no destructor, duplicating Actor's fields
from 0x05c down.
THE DESTRUCTOR IS THE EVIDENCE, and it closes on three boundaries at once:
t[0] = _ZTV8Platform
MovingMeshCollider::~MovingMeshCollider(this + 0x124)
Model::~Model(this + 0xd4)
Actor::~Actor(this)
Actor is 0xd0, so Platform's own data starts there. Model is 0x50, so a Model
at 0xd4 ends at exactly 0x124 -- where the destructor puts the collider.
MovingMeshCollider is 0x1c8, so it ends at 0x2ec. Three sizes, each asserted by
its own header, each landing on an offset the destructor names independently.
WHAT THE MARKERS TURNED OUT TO BE. Four of the generated fields were never
Platform's: unk_0f0 is the Model's own mat4x3 (0xd4 + 0x1c), and unk_114/118/
11c are that matrix's translation row (+0x24/+0x28/+0x2c). UpdateModelPosAndRotY
was writing the model's position through raw offsets and now says so, with the
`this + 0xf0` magic address gone.
AND 0x2ec IS A SECOND MATRIX, on the same kind of arithmetic. The generated
header had 0x24 of padding there and three s32 at 0x310/0x314/0x318 -- but
0x2ec + 0x24 IS 0x310, so those three are that span's last row and 0x2ec..0x31c
is one Matrix4x3. UpdateClsnPosAndRot settles it: it copies the model's matrix
in and then writes the actor's position into 0x310/0x314/0x318. A matrix and its
translation, not four unrelated fields. It is what the mesh collider is
transformed by, so it is mClsnMat.
Also migrated: IsClsnInRange and IsClsnInRangeOnScreen (unk_0b0 was
Actor::mFlags). Five sources, three shadow structs deleted, and the two
remaining `Obj`/`MMC` stand-ins are gone entirely.
ONE IDIOM DELIBERATELY KEPT. UpdateClsnPosAndRot still casts to a flat 12-word
struct for the matrix copy. `mClsnMat = mModel.mat4x3` is the readable spelling
and is +0x10 bytes: Matrix4x3 is {Matrix3x3 r; Vector3 t;}, so member-wise
assignment copies 9 words then 3 (ldm/stm 4+4+1, then a separate 3) where the
ROM copies 12 flat as 4+4+4. include/MovingMeshCollider.h already records this
obstacle for SetFile and Transform and asks for one shared flat-copy helper in
a pass of its own. This is not that pass.
The destructor is defined INLINE, per the convention this branch establishes --
all 72 subclasses inline its vptr store rather than calling it, so the body has
to be visible. Its D1 file carries the forcing TU.
GATES
build_pin.verify 5/5 (True, '2004/b56')
eligible.py 10811 -> 10811, name list IDENTICAL
rombuild -j16 106/106 exact, 0 mismatching, 10,811 source-built, 87.83%
check_header_offsets 8 commented fields, 0 mismatched, 0 unparsed, 0x31e
no --no-verify
A first eligible run reported func_ov006_020c3ad8 as "compile failed" and it is
`void func_ov006_020c3ad8(void) {}` with no includes -- it cannot see this
header, and it compiles standalone. Transient build contention; a serial re-run
returned an identical name list. Recorded because the reflex on a broken
neighbour should be to re-run before believing it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XxDmkQ47fWa3GB6mj8xEQe
#1374 (mine) gave Platform three trailing `s16` at 0x31e/0x320/0x322 and a data size of 0x324. The fields are real; the class they were put on is not. They are BowserFireSeaArena's own, and they move back. The evidence I used was one-sided. BowserFireSeaArena reads all three and its own mModel2 sits at 0x324, which is true whether the three belong to Platform or to BowserFireSeaArena -- a derived class's fields start at the base's DATA size rounded to their alignment, so with Platform ending at 0x31e the three s16 land at 0x31e/0x320/0x322 and mModel2 at 0x324 either way. One class cannot tell the difference. StarSwitch can, and it says the opposite: its own first field is an `s32` at 0x320, which is exactly 0x31e rounded up to 4. If Platform owned 0x31e..0x324 that field would be inside the base, and `sizeof(StarSwitch) == 0x354` cannot hold. It does not -- StarSwitch.h fails to compile against the version on main, which is how this was caught, and it compiles against this one. So Platform's data ends at 0x31e and its sizeof is 0x320, the alignment round-up. check_header_offsets now reports the span as 0x31e rather than 0x324. Re-verified under the correction: BowserFireSeaArena's D1, D0 and InitResources all still reproduce, bytes and relocation destinations, and BowserFireSeaArena.h still spans 0x570. rombuild -j16 --no-rom 106/106 exact, 0 mismatching. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015AXm5k53WFPjCYcDHRDX3x
…ings that stop the other 41 First subclasses on top of #1370. Both headers were generated skeletons restating all of Actor and Platform; they are `struct X : Platform` now, their destructors are real methods, and five other sources came along because they had to compile against the new header. Nothing below 0x320 is declared twice any more. The renames are all Actor's real names: mActorID/mActorId -> actorID, mParam -> param1, and a local `typedef int Fix12` deleted from three files -- it shadowed the real Fix12 template the moment Platform.h made it visible. Raw offsets became members: `this + 0x124` is mMeshCollider, `this + 0x2ec` is mClsnMat, `this + 0xd4` is mModel. I tried nine subclasses and kept two. The seven that came back out are the useful part of this commit, because each names a different blocker: 1. FOUR HAVE AN INTERMEDIATE CLASS. ShutterBob, ShutterHmc, FallBlockLll and MetalNetLift store THREE vtables, not two -- e.g. FallBlockLll writes _ZTV15daObjFlMaruta_c, then _ZTV10dBgActor_c, then _ZTV8Platform. There is a class between them and Platform (dBgActor_c, and an unnamed one at data_ov002_021099e4 for the Shutters). It has to be reconstructed first. 2. THREE USE PLATFORM'S TAIL PADDING, AND check_header_offsets CANNOT MODEL IT. DonutBlock, FortressWall and BigBrickBlock put fields at 0x31e/0x31f. Platform's last field ends at 0x31e and its size rounds to 0x320, and the Itanium ABI lets a derived class use a non-POD base's tail padding -- the bytes confirm it, DonutBlock::Behavior reads this+0x31e and reproduces. check_header_offsets starts a derived class's fields at the base's SIZEOF rather than its DATA SIZE, so it reports every field mismatched and EXITS 1. All three verified (DonutBlock: five functions, including two rewritten off their shadow structs) and all three are reverted, because shipping headers that make a gate hard-fail is worse than shipping fewer. The tool wants the base's data size; that is the next change, and it unblocks all three plus every other subclass with a field below 0x320. 3. ONE GREW ITS .text SECTION. BigMovingIceBlock::Behavior byte-verified and then went ineligible: `.text section 0x258 != declared 0x234`. The TU emits something beyond the declared function once Platform.h is visible -- the inline destructor's COMDAT copy is the obvious suspect and is unproven. Worth knowing because ENROLL.PY PRESERVES `complete` BY SYMBOL NAME, so the now-ineligible file stayed enrolled, compiled anyway, and took ov056 down with 461 mismatching words. An eligibility loss is not self-correcting; check the bracket before trusting the build. Two field-placement notes for whoever does the next batch: - Do NOT pad from 0x31e. The base's asserted size already covers 0x31e..0x31f, so a `u8 pad_31e[2]` shifts every later field by 2 and reports as a mismatch. Start padding at 0x320. - `mAngleY += x` and `mAngleY = mAngleY + x` are NOT interchangeable, and which one matches is per-SITE. DonutBlock::Behavior needs the compound form (the ROM CSEs the field address); Player::St_WallJump_Init needs the other. Same for a bool temp: `int b = (x & 8) != 0; if (b)` emits movne/moveq where a direct `if` folds them. GATES build_pin.verify 7/7 (True, '2004/b56') eligible.py 10811 -> 10811, name list IDENTICAL rombuild -j16 106/106 exact, 0 mismatching, 10,811 source-built, 87.83% check_header_offsets StarSwitch 16 fields 0 mismatched; FloatingFloorLllBig 2/0 no --no-verify Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XxDmkQ47fWa3GB6mj8xEQe
|
Built on #1370's Platform, which #1374 supersedes — see the note there. The two classes here (StarSwitch, FloatingFloorLllBig) are rebuilt against the correct That width conflict is the open question this PR's framing missed. |
2a5f048 to
51bd759
Compare
dcf05f7 to
6eff8f3
Compare
… and three classes come back The Itanium ABI lets a derived class place members in a non-POD base's TAIL PADDING, and this tree's classes do it. Platform's last field ends at 0x31e and its size rounds to 0x320, so DonutBlock's s16 lands at 0x31e -- confirmed by the bytes, since DonutBlock::Behavior reads this+0x31e and reproduces the ROM. check_header_offsets started a derived class's fields at the base's SIZEOF. For those classes it reported every field mismatched and EXITED 1, which is why #1372 shipped without them. THE FIX IS NOT A TAIL-PADDING SPECIAL CASE. Starting at the data size is simply the correct offset that sizeof was approximating, and it is right for both kinds of class, because the walk already aligns each field: a derived class whose first own field is a 4-byte value still gets 0x31e rounded up to 0x320. The data size is read off the LAST COMMENTED FIELD -- the ROM evidence the tool already checks against -- rather than recomputed, so it cannot drift from the walk it feeds. CONTROL: every header in include/ was run through the tool before and after. The output is byte-identical -- no existing verdict moves, and the only headers whose result changes are the ones that could not be written until now. So DonutBlock, FortressWall and BigBrickBlock come back, with the source rewrites #1372 had to revert with them: DonutBlock 5 functions. Behavior loses a `typedef int Fix12` and stand-in MeshColliderBase and Platform structs; the `t[0x1e/2]` read off `this + 0x300` was its own unk_31e reached the long way round. InitResources loses three raw offsets -- 0xd4, 0x124 and 0x2ec are mModel, mMeshCollider and mClsnMat. FortressWall 4 functions BigBrickBlock 4 functions Two spellings in DonutBlock::Behavior are load-bearing and were measured, not chosen: `mAngleY += unk_31e` (the ROM CSEs the field address; the expanded form is the matching one in Player::St_WallJump_Init, so this is per-SITE), and a bool materialised into an int temp (the ROM emits movne/moveq and then tests, where a direct `if` folds them). GATES build_pin.verify 13/13 (True, '2004/b56') across the three classes check_header_offsets DonutBlock 1/0, FortressWall 4/0, BigBrickBlock 4/0 and identical output on every other header in include/ eligible.py 10811 -> 10811, name list IDENTICAL rombuild -j16 106/106 exact, 0 mismatching, 10,811 source-built no --no-verify Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XxDmkQ47fWa3GB6mj8xEQe
|
Rebased — and StarSwitch caught a real error of mine on main. This branch now carries the fix, as a separate commit below yours.
My evidence was one-sided. Your Re-verified under the correction: Your commit This is also nice corroboration for #1375 — "a derived class starts at the base's DATA SIZE" is exactly the rule that settles it, arrived at from the other direction. |
|
My reasoning for I told you StarSwitch was decisive — that its
Byte accesses at 0x31e, 0x31f and 0x321. So This also puts #1372/#1375 and #1377 in direct conflict, and it is worth resolving before either lands. #1377 rebuilds nine subclasses on main's uncorrected Platform, every one of them starting its own fields at Both this PR and #1375 validate green as they stand. |
|
Re-checked: green, zero commits behind main, no conflict and no rebase needed — but now fully contained in #1377.
Both PRs are green and they cannot both merge: they change If #1377 lands, this PR and #1375 retire with it. The one thing neither of them has is #1370's migration of |
2 participants
StarSwitch and FloatingFloorLllBig derive from Platform, and three things that stop the other 41
First subclasses on top of #1370. Both headers were generated skeletons
restating all of Actor and Platform; they are
struct X : Platformnow, theirdestructors are real methods, and five other sources came along because they
had to compile against the new header. Nothing below 0x320 is declared twice
any more.
The renames are all Actor's real names: mActorID/mActorId -> actorID, mParam ->
param1, and a local
typedef int Fix12deleted from three files -- it shadowedthe real Fix12 template the moment Platform.h made it visible. Raw offsets
became members:
this + 0x124is mMeshCollider,this + 0x2ecis mClsnMat,this + 0xd4is mModel.I tried nine subclasses and kept two. The seven that came back out are the
useful part of this commit, because each names a different blocker:
FOUR HAVE AN INTERMEDIATE CLASS. ShutterBob, ShutterHmc, FallBlockLll and
MetalNetLift store THREE vtables, not two -- e.g. FallBlockLll writes
_ZTV15daObjFlMaruta_c, then _ZTV10dBgActor_c, then _ZTV8Platform. There is a
class between them and Platform (dBgActor_c, and an unnamed one at
data_ov002_021099e4 for the Shutters). It has to be reconstructed first.
THREE USE PLATFORM'S TAIL PADDING, AND check_header_offsets CANNOT MODEL IT.
DonutBlock, FortressWall and BigBrickBlock put fields at 0x31e/0x31f.
Platform's last field ends at 0x31e and its size rounds to 0x320, and the
Itanium ABI lets a derived class use a non-POD base's tail padding -- the
bytes confirm it, DonutBlock::Behavior reads this+0x31e and reproduces.
check_header_offsets starts a derived class's fields at the base's SIZEOF
rather than its DATA SIZE, so it reports every field mismatched and EXITS 1.
All three verified (DonutBlock: five functions, including two rewritten off
their shadow structs) and all three are reverted, because shipping headers
that make a gate hard-fail is worse than shipping fewer. The tool wants the
base's data size; that is the next change, and it unblocks all three plus
every other subclass with a field below 0x320.
ONE GREW ITS .text SECTION. BigMovingIceBlock::Behavior byte-verified and
then went ineligible:
.text section 0x258 != declared 0x234. The TU emitssomething beyond the declared function once Platform.h is visible -- the
inline destructor's COMDAT copy is the obvious suspect and is unproven.
Worth knowing because ENROLL.PY PRESERVES
completeBY SYMBOL NAME, so thenow-ineligible file stayed enrolled, compiled anyway, and took ov056 down
with 461 mismatching words. An eligibility loss is not self-correcting;
check the bracket before trusting the build.
Two field-placement notes for whoever does the next batch:
so a
u8 pad_31e[2]shifts every later field by 2 and reports as amismatch. Start padding at 0x320.
mAngleY += xandmAngleY = mAngleY + xare NOT interchangeable, andwhich one matches is per-SITE. DonutBlock::Behavior needs the compound form
(the ROM CSEs the field address); Player::St_WallJump_Init needs the other.
Same for a bool temp:
int b = (x & 8) != 0; if (b)emits movne/moveq wherea direct
iffolds them.GATES
build_pin.verify 7/7 (True, '2004/b56')
eligible.py 10811 -> 10811, name list IDENTICAL
rombuild -j16 106/106 exact, 0 mismatching, 10,811 source-built, 87.83%
check_header_offsets StarSwitch 16 fields 0 mismatched; FloatingFloorLllBig 2/0
no --no-verify
Stacked on #1370.
🤖 Generated with Claude Code