Rename Seaweed to daObjWakame_c and promote it to a single-file TU - #2169
Conversation
✅ PR validation — PassedCommitted merge passes; 8 contributor credit reassignment(s) noted, not a blocker. Full merge validation
Contributor credit moved (8)
Byte-verified means the range carries Warnings: contributor attribution changed, not a blocker (8 changed, 0 lost -- src/actors/d_a_obj_wakame.cpp: tangosdev -> github-actions[bot]; src/actors/d_a_obj_wakame.cpp: tangosdev -> github-actions[bot]; src/actors/d_a_obj_wakame.cpp: tangosdev -> github-actions[bot]; +5 more); 8 address range(s) left the byte-verified set while enrolled totals held steady: ov002:0x020bc414-0x020bc444, ov002:0x020bc444-0x020bc488, ov002:0x020bc488-0x020bc4c8, ov002:0x020bc4c8-0x020bc4f8, ov002:0x020bc4f8-0x020bc520, +3 more; 7 more function(s) now claim a match that nothing compiles; enroll them in a delinks.txt to have the ROM build check them. Per-file link-check detailAll 1 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. |
The coined name reached no symbol home. _ZTI/_ZTS/_ZTV records are length-prefixed mangled strings, so `7Seaweed` matches nothing at any address in the ROM and none of this class's vague-linkage data could ever be word-compared against the cartridge -- which is what blocks a class from being its own key-function TU and from declaring a verifiable compiler_only_output. The rename therefore has to land before any promotion, not after. THE NAME IS READ FROM THE CARTRIDGE, not from a tool's summary. ov002 0x02109b78 is a __si_class_type_info; word [1] points at 0x02109b84, which holds exactly `13daObjWakame_c`, and word [2] points at arm9 0x0208e390, which is _ZTI8dActor_c. So the ROM spells the class daObjWakame_c and its one base, at subobject offset 0, is dActor_c -- which is what include/Seaweed.h already described from the factory and the destructor, under the wrong name. IT IS A LEAF, and that is measured rather than assumed: the word 0x02109b78 occurs exactly once anywhere under extracted/ -- offset 0x5c554 of ov002, which is 0x02109bb4, the typeinfo slot of its own vtable header at _ZTV-4 (the symbols.txt address 0x02109bb8 is the address point). The two hits the scan reports are the same image under two paths, extracted/dsd/arm9_overlays/ov002.bin and extracted/overlays/overlay_0002.bin. No other class's __si_class_type_info points at it. SEVEN OF THE NINE ROWS tools/class_rename.py rewrote in symbols/actor_renames.tsv were reverted, because they are not this class's. The tool matched on the old name's prose neighbourhood rather than on addresses; an audit of every touched row against config/arm9/overlays/ov002/symbols.txt put 0x020bc5e0, 0x020bc618, 0x020bc6a4, 0x020bc6d4, 0x020bc6fc and 0x020bc81c inside HealingHeart, and 0x02109c74 at _ZTV12HealingHeart. Only the two rows this class actually owns were kept: the spawn function at 0x020bc5a8 and its spawn info at 0x02109b94. This is the same cross-class hazard that produced review finding 4 on #2168, caught before the PR this time. (Separately, those seven rows are mislabelled on main as well, with the wrong class name against addresses HealingHeart owns -- a pre-existing defect in a live lookup table that needs its own evidence and its own change, not a quiet fix folded in here.) symbols/actor_renames_report.txt is left untouched for the same reason. Every line the tool changed there is one of those same seven HealingHeart rows, and the ledger row each one reports on still names the old claimer -- so rewriting the report alone would have made the report and the table it reports on disagree. The tool also rewrote actor_renames.tsv LF where the repo keeps it CRLF, which made `diff` report all 2501 lines changed and hid the real nine. It is written back CRLF here. 8 sources, the header, the shadow TU and the manifest move. Prose references to this class in three sibling manifests and one sibling TU -- historical notes that name it as it stood at #1728 -- follow the rename too, so the class stays findable under one name; none of them is code. 106/106 exact, 100.000000%, mismatching 0. CONVERTED holds at 2591, ratchet +0/-0 -- the moved entries are a rename within the set, not a backslide. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FsXWgEM3Gtbqs6zEdwLMYV
Eight per-function objects become one. src/actors/d_a_obj_wakame.cpp is the
whole of the cartridge's contiguous linker run .text 0x020bc414..0x020bc5e0 --
eight functions and nothing else -- and the production build links this object
instead of eight. The filename is tools/tu_names.py's: candidate_stem
('daObjWakame_c') is d_a_obj_wakame.
THE DESTRUCTOR IS INLINE IN THE HEADER, AND THAT IS WHAT MAKES THE TU POSSIBLE.
Declared out of line, mwccarm 2004/b56 emits D0 before D1 -- the reverse of the
cartridge, which has D1 at 0x020bc414 and D0 at 0x020bc444 -- and adds a third,
homeless D2 that no ROM address claims; tools/objisolate.py then refuses the
whole TU rather than one function. Defined as `virtual ~daObjWakame_c() {}` in
the header, the pair comes out in ROM order and there is no D2. The lever is
well established -- `git grep -l "emits D0 before D1" include/` names twelve
headers carrying it, four of them measured directly in this series (Vector3,
daObjBSwdoor_c, daSoundObj_c, daSetSE_c) -- and it is safe here only because the
leaf measurement in the rename commit says nothing derives from this class.
AN INLINE {} DESTRUCTOR IS NOT AN EMPTY DESTRUCTOR, which is new here and worth
recording. D1 is 0x30 bytes where a scalar-only sibling like daSoundObj_c's is
0x24, and the difference is the member: `add r0,r4,#0xd4` sets up the argument
and `bl _ZN9ModelAnimD1Ev` destroys it, with the vptr store `str r1,[r4]`
sitting between them. The compiler emits that call from an empty body because
ModelAnim has a destructor of its own. In full, 0x020bc414 reads `push {r4,lr} /
mov r4,r0 / ldr r1,[pc,#0x1c] / add r0,r4,#0xd4 / str r1,[r4] / bl
_ZN9ModelAnimD1Ev / mov r0,r4 / bl _ZN8dActor_cD2Ev / mov r0,r4 / pop {r4,lr} /
bx lr`, plus the literal 0x02109bb8 -- vptr store, member destroyed, then the
base destructor called and `this` returned. (Called, not tail-called: the `bl`
is followed by mov/pop/bx.) Every instruction is what an empty body produces;
writing anything between the braces would add code the cartridge does not have.
TWO COINED SHADOW STRUCTS ARE GONE, replaced by real member calls that byte-
match. Render's legacy `Base`/`Derived` pair existed only to reach a vptr at
this+0xd4 by hand; mwccarm 2004/b56 does not devirtualize a virtual call through
a member of known dynamic type -- the ROM itself proves it, `ldr r2,[r0,#0xd4]!
/ ldr r2,[r2,#0x14] / blx r2` at 0x020bc4f8 -- so the plain `mModelAnim.Render
(0)` compiles to the same dispatch. Behavior's shadow likewise becomes
`mModelAnim.Advance()`. Both verified by byte comparison, not by argument.
DELIBERATELY NOT DONE: data_ov002_0210e0d4 and _0210e0dc keep their mangled
spelling. They are two SharedFilePtr objects, but include/decl_common.h declares
both as `extern int []` and is included by a large part of the tree, so retyping
them is a tree-wide change with its own evidence and its own verify cycle -- not
something to fold into a TU promotion. The TU says so at the declaration.
NAMING THE CLASS PUTS ITS VAGUE-LINKAGE DATA IN THIS OBJECT, because this TU
defines the key function. Nine records come out and romdata_check compares each
against the cartridge with relocations applied, per symbol:
_ZTV13daObjWakame_c ov002 0x02109bb8 VERIFIED, 124 bytes = 31 slots
_ZTI13daObjWakame_c ov002 0x02109b78 VERIFIED
_ZTS13daObjWakame_c ov002 0x02109b84 VERIFIED
_ZTI8dActor_c / _ZTI7dBase_c / _ZTI7fBase_c arm9 VERIFIED
_ZTS8dActor_c / _ZTS7dBase_c / _ZTS7fBase_c arm9 PARTIAL
nothing DIFFERS
The entry's own manifest note said the opposite of all of this until now. It was
written while the class still carried the coined name, when none of these
records could be word-compared at all, and tools/class_rename.py then
substituted the new name into both halves of its comparison -- leaving it
claiming a length mismatch between daObjWakame_c and daObjWakame_c. It is
rewritten to the measured result.
THE VTABLE CLAIM IS SCOPED TO THE SLOTS, deliberately. A symbols.txt _ZTV
address is the ADDRESS POINT, eight bytes past the table's real start; the
{offset-to-top, _ZTI pointer} header word pair is emitted here and word-compared
by nothing, so the 31 slots are proved and those two words are not. The three
PARTIAL rows are the known dsd extent shortfall on _ZTS records -- a range that
stops short, not a disagreement about bytes.
The header now carries the real layout with both of its witnesses: the factory's
`mov r0,#312` and include/ModelAnim.h's own 0x64 size assert close on each other
at 0xd4 + 0x64 = 0x138 with nothing left over, and the member's offset has four
independent witnesses in code that runs. The vtable was diffed slot by slot
against _ZTV8dActor_c (arm9 0x0208e3a4): both 31 words, differing in exactly
slots 0, 3, 6, 9, 16 and 17 -- the six members the header declares and no others.
tubuild verify : 8/8 MATCH, objisolate clean, reloc-destinations clean
linkcheck : baseline control refreshed, modules PASS, ROM IDENTICAL
rombuild -j16 : 106/106 exact, 100.000000%, mismatching 0
CONVERTED : 2591 -> 2592 (+1 / -0), banked
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FsXWgEM3Gtbqs6zEdwLMYV
2cf8420 to
2b6c33b
Compare
Review, and what it changedA review pass re-derived every ROM claim in this PR independently — the vtable diff, the leaf byte scan, the D1 disassembly, the devirtualization argument, the layout closure and all nine 1. The cross-class ledger leak survived in a second file. The rename commit correctly reverted the seven HealingHeart rows in 2. The new manifest carried a self-refuting note. Two prose corrections came out of the same pass and are in the amended commit message and the body above: The diff from the reviewed head is four files, all text: the manifest note, the header's opening line, the report file revert, and the tool usage example. Signing off here rather than as an approval, since this repo's PRs share one account. |
1 participant
Seaweed was never the cartridge's name for this class, and until it was corrected none of its RTTI could be compared against the ROM at all. This renames it to what ov002 actually spells, then makes it a genuine single-file translation unit: eight per-function objects become one,
src/actors/d_a_obj_wakame.cpp, the whole of the contiguous linker run.text 0x020bc414..0x020bc5e0.Two commits, and the order is load-bearing. A coined name is a length-prefixed mangled string that matches nothing at any address, so a class carrying one can never have its vague-linkage data word-compared, which is exactly what a key-function TU needs. The rename has to land first.
The name is read from the cartridge
ov002
0x02109b78is a__si_class_type_info; word [1] points at0x02109b84, which holds exactly13daObjWakame_c, and word [2] points at arm90x0208e390, which is_ZTI8dActor_c. The ROM spells the classdaObjWakame_cand its one base, at subobject offset 0, isdActor_c— which is whatinclude/Seaweed.halready described from the factory and the destructor, under the wrong name.It is a leaf, measured rather than assumed: the word
0x02109b78occurs exactly once anywhere underextracted/— offset0x5c554of ov002, which is0x02109bb4, the typeinfo slot of its own vtable header at_ZTV−4. (The scan reports two hits becauseextracted/dsd/arm9_overlays/ov002.binandextracted/overlays/overlay_0002.binare the same image under two paths.) No other class's__si_class_type_infopoints at it.Seven of nine rename rows were reverted, because they are not this class's
tools/class_rename.pyrewrote nine rows insymbols/actor_renames.tsv. Auditing every touched row againstconfig/arm9/overlays/ov002/symbols.txtput0x020bc5e0,0x020bc618,0x020bc6a4,0x020bc6d4,0x020bc6fcand0x020bc81cinside HealingHeart, and0x02109c74at_ZTV12HealingHeart. Only the two rows this class owns were kept — the spawn function at0x020bc5a8and its spawn info at0x02109b94. This is the same cross-class hazard that produced review finding 4 on #2168, caught before the PR this time.symbols/actor_renames_report.txtis left untouched for the same reason. Every line the tool changed there is one of those same seven HealingHeart rows, and the ledger row each one reports on still names the old claimer — so rewriting the report alone would have made the report and the table it reports on disagree.The tool also rewrote
actor_renames.tsvLF where the repo keeps it CRLF, which madediffreport all 2501 lines changed and hid the real nine. It is written back CRLF here.The destructor is inline in the header, and that is what makes the TU possible
Declared out of line, mwccarm 2004/b56 emits D0 before D1 — the reverse of the cartridge, which has D1 at
0x020bc414and D0 at0x020bc444— and adds a third, homeless D2 that no ROM address claims;tools/objisolate.pythen refuses the whole TU rather than one function. Defined asvirtual ~daObjWakame_c() {}in the header, the pair comes out in ROM order and there is no D2. The lever is well established —git grep -l "emits D0 before D1" include/names twelve headers carrying it, four of them measured directly in this series (Vector3, daObjBSwdoor_c, daSoundObj_c, daSetSE_c) — and it is safe here only because of the leaf measurement above.An inline
{}destructor is not an empty destructor — that is new here and worth recording. D1 is0x30bytes where a scalar-only sibling like daSoundObj_c's is0x24, and the difference is the member:add r0,r4,#0xd4sets up the argument andbl _ZN9ModelAnimD1Evdestroys it, with the vptr storestr r1,[r4]sitting between them. The compiler emits that call from an empty body becauseModelAnimhas a destructor of its own. In full,0x020bc414reads:plus the literal
0x02109bb8— vptr store, member destroyed, then the base destructor called andthisreturned. (Called, not tail-called: theblis followed bymov/pop/bx.) Every instruction is what an empty body produces; writing anything between the braces would add code the cartridge does not have.Two coined shadow structs are gone, and the bytes agree
Render's legacy
Base/Derivedpair existed only to reach a vptr atthis+0xd4by hand. mwccarm 2004/b56 does not devirtualize a virtual call through a member of known dynamic type — the ROM itself proves it,ldr r2,[r0,#0xd4]! / ldr r2,[r2,#0x14] / blx r2at0x020bc4f8— so the plainmModelAnim.Render(0)compiles to the same dispatch. Behavior's shadow likewise becomesmModelAnim.Advance(). Both settled by byte comparison, not by argument.The layout has two witnesses and they close on each other
SIZE 0x138is the factory's own literal,mov r0, #312intofBase_c::operator new.include/ModelAnim.hassertssizeof(ModelAnim) == 0x64, and0xd4 + 0x64 = 0x138exactly — a factory literal in ov002 and a size assert on an arm9 class meeting with nothing left over, so there is no room for a trailing field the header has missed.The member's offset has four witnesses, all of them code that runs: the factory constructs it at
this+0xd4; the destructor destroys it there; Render loads a vptr fromthis+0xd4and dispatches slot 5 through it, so+0xd4is a polymorphic object rather than bytes; and Behavior callsAnimation::Advanceonthis+0x124, where0x124 − 0xd4 = 0x50is exactly whereinclude/ModelAnim.hpins theAnimationbase.The vtable was diffed slot by slot against
_ZTV8dActor_c(arm90x0208e3a4): both 31 words, differing in exactly slots 0, 3, 6, 9, 16 and 17 — the six members the header declares and no others. Every other slot holds the base's own word and is inherited, so it is deliberately not redeclared.Vague-linkage data, per symbol
Naming the class puts its RTTI in this object, because this TU defines the key function. Nine records come out;
romdata_checkcompares each against the cartridge with relocations applied:_ZTV13daObjWakame_c0x02109bb8_ZTI13daObjWakame_c0x02109b78_ZTS13daObjWakame_c0x02109b84_ZTI8dActor_c/_ZTI7dBase_c/_ZTI7fBase_c_ZTS8dActor_c/_ZTS7dBase_c/_ZTS7fBase_cThe entry's own manifest note said the opposite of all of this until now. It was written while the class still carried the coined name, when none of these records could be word-compared at all, and
class_rename.pythen substituted the new name into both halves of its comparison — leaving it claiming a length mismatch betweendaObjWakame_canddaObjWakame_c. It is rewritten to the measured result.The vtable claim is scoped to the slots, deliberately. A
symbols.txt_ZTVaddress is the address point, eight bytes past the table's real start; the{offset-to-top, _ZTI pointer}header pair is emitted here and word-compared by nothing, so the 31 slots are proved and those two words are not. The three PARTIAL rows are the known dsd extent shortfall on_ZTSrecords — a range that stops short, not a disagreement about bytes.Deliberately not done
data_ov002_0210e0d4and_0210e0dckeep their mangled spelling. They are twoSharedFilePtrobjects, butinclude/decl_common.hdeclares both asextern int []and is included by a large part of the tree, so retyping them is a tree-wide change with its own evidence and its own verify cycle — not something to fold into a TU promotion. The TU says so at the declaration.Proof
Rebased onto
3940d4429and rebuilt after the rebase — the 106/106 above is the post-rebase run.🤖 Generated with Claude Code
https://claude.ai/code/session_01FsXWgEM3Gtbqs6zEdwLMYV