Declare dScMgBase_c slot 33 (Virtual84) across the minigame family - #2108
Conversation
Slot 33 of the dScMgBase_c keystone: the base declares `virtual void Virtual84()` and the two classes that override it declare it too, so all 19 tables that carry the slot emit it from source instead of inheriting a gap. romdata_check moves verifiedBytes +4 and partialBytes +124 -- the 32 tables x 4 bytes this slot is worth -- with every other counter and the `differing` set unchanged in both directions. Structurally this is slot 26 again: the same 19 tables, the same two owning classes (dScMgSingle3DBase_c plus its 13 children, dScMgD3DBase_c plus its 4), 17 inherited. Two things make it different. It is the only slot in the campaign with NOTHING to correct. Slots 26, 29, 30, 31 and 32 each retired a name the recovery pass had borrowed across a fork of the hierarchy; here the pass guessed nothing, so there is no `recovered name:` line on the base body or on either override. The name is the repo's own no-name spelling after the +0x84 vtable offset, the convention fBase_c already uses for Virtual34/Virtual38. It is the first slot whose override lives inside a PROMOTED intact-object TU. dScMgSingle3DBase_c's body is in src/actors/dScMgSingle3DBase_c.cpp, so the rename is an in-file identifier rewrite, and with the base declared the body becomes a real member definition -- exactly what slot 26 did in that same file three commits ago. It was the last vtable slot in that unit still spelled as a mangled free function; the only one left under a func_<module>_<address> name, 0x0210a534, is in no vtable at all. The class emits a byte-exact 34-slot prefix now. Signature evidence. `void`, because none of the three bodies assigns a result and the one caller ignores what falls out -- and because the Single3D body is a real member definition now, where a declared `int` with no return statement would be a lie. No explicit parameters, measured: scanning arm9 and all 103 overlays for the dispatch pair `ldr rD,[rN,#0x84]` (Rn != pc) followed within three instructions by `blx rD`/`bx rD` finds two sites image-wide, one of them in ov004/ov006 -- ov004:0x020b09d0 inside BeforeInitResources, reading `mov r0,r4; ldr r1,[r0]; ldr r1,[r1,#0x84]; blx r1`, so r1 is the loaded pointer and cannot also be a second argument. The scanner was validated by re-running it at +0x80 and reproducing slot 32's known call site. What it does: engine bring-up. Graphics modes and VRAM banks for both screens, both BG-enable shadows initialised to 0x10 (the value slots 30 and 31 later save and restore), a language-indexed character file decompressed into both engines' BG char VRAM, OBJ palette file 0xc3 into both, and the scene object published into the global registry -- which is what fixes `this` as the incoming argument. It runs first out of BeforeInitResources; slot 31 runs last out of the same function and slot 32 out of AfterInitResources, so the order is: bring the engines up (33), dress the sub screen (31), dress the main screen (32). Verified: rombuild 11,088/11,088 reproducing, 106/106 modules exact, 100.000000% of compared bytes. check_dead_references, port_refcheck, check_duplicate_sources, check_src_tu_compiles, check_header_offsets --changed and the langmode ratchet all pass. Two slots left: 34-35. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QhhAeJwXBnfPp7B5DNjCwh
andrewboudreau
added
the
attribution-override
✅ PR validation — PassedCommitted merge passes; 2 contributor credit reassignment(s) noted, not a blocker. Full merge validation
Contributor credit moved (2)
Byte-verified means the range carries Warnings: contributor attribution changed, not a blocker (2 changed, 0 lost -- src/_ZN11dScMgBase_c9Virtual84Ev.c: tangosdev -> andrewboudreau; src/actors/dScMgSingle3DBase_c.cpp: github-actions[bot] -> andrewboudreau). Per-file link-check detailAll 192 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. |
|
Self-review — I wrote this, so this is a record of what I checked, not an approval. Nobody in this repo can approve a PR (one shared account), so the sign-off is the comment. Static gates, on the merge tree rather than the branch. Byte verification. Things I went looking for and did not find:
What I'd want a second pair of eyes on:
Known outstanding, not blocking: Slot 34 is up as #2110; slot 35 is the last one. |
andrewboudreau
left a comment
andrewboudreau
left a comment
There was a problem hiding this comment.
Reviewed at a3d94efa1a0d. This is the strongest PR in the stack, and I think it settles the question that is holding the other five. One ask, and it is about a claim's checkability, not its truth.
I verified every ROM claim in this PR independently against the cartridge. All of them hold. Details below, because on a campaign this long the useful thing is a second reading of the same bytes, not agreement.
What this one does that the previous five did not
It declares void, and for the right reason. virtual void Virtual84(); /* slot 33 */. Your argument — that one of the three bodies is about to become a real member definition, "where a declared int with no return statement would be a lie the compiler has to paper over" — is exactly the argument I made on #2107 for slot 32, and it is better stated here than I stated it. This PR adds zero new header/definition splits. It is the first in the stack that doesn't.
It converts a mangled free function into a real member. extern "C" void func_ov006_0210a708(char *obj) → void dScMgSingle3DBase_c::Virtual84(), with obj → this, inside the promoted TU. That is the project goal rather than a rename, and it retires the last vtable slot in that unit still spelled func_<module>_<address>.
The arity claim is now conclusive, and you validated the scanner first. On #2107 I said the +0x80 evidence did not carry, because the three sites there put the pointer in r1/r2/r3 and the mov r4,r1 park tracks caller liveness rather than callee signature. At +0x84 the site reads mov r0,r4; ldr r1,[r0]; ldr r1,[r1,#0x84]; blx r1 — r1 holds the loaded pointer, so it cannot also carry a second argument. That is the structurally decisive form. And re-running the scan at +0x80 to reproduce slot 32's known site is a positive control. MEASURED is earned here. I withdraw nothing from what I said on #2107 and I am not asking for anything on this one.
It tracks the index shift it causes. include/dScMgAmida_c.h updates Unk36 from "lands on 33" to "lands on 34". That is the kind of thing that silently rots.
Independent verification of the vtable claims
Dumped from the cartridge (extracted/overlays/overlay_000{4,6}.bin), slot indices from each _ZTV's recorded address:
| table | slot 33 | own slots in 18-35 | real extent |
|---|---|---|---|
_ZTV11dScMgBase_c @020bc0c0 (ov004) |
020b265c |
— | 36 words |
_ZTV12dScMgAmida_c @0213b918 |
020b265c (inherited) |
Unk36 at 36 |
37 words |
_ZTV19dScMgSingle3DBase_c @0213e448 |
0210a708 |
26, 33 | 36 words |
_ZTV14dScMgD3DBase_c @0213c62c |
020e7124 |
24-31, 33 | 36 words |
Every one agrees with this PR:
- Slot 33 of the base is
020b265c, the ov004 function you named. dScMgSingle3DBase_c's own slots among 18-35 are exactly 26 and 33 — the "identical reference sets" argument in the TU manifest is correct, and slot 26 (0210a600) and slot 33 (0210a708) are both this class's.dScMgD3DBase_c's own slots are exactly 24, 25, 26, 27, 28, 29, 30, 31, 33 — nine, as the header says. Slot 32 is020b27f4, inherited, which is why #2107 correctly did not declare it here.dScMgAmida_c's table is 37 words andUnk36really is slot 36 (020d1188), so "declaring slot 35 puts it on 36" is right.
The one ask: dScMgD3DBase_c's "byte-exact 34-slot prefix" cannot be checked
include/dScMgD3DBase_c.h now says:
its table is a byte-exact 34-slot prefix rather than a 31-slot one.
The claim about the cartridge is true — I read all 36 slots and they are one contiguous run of code addresses, with the first non-code word at index 36. The problem is the word "byte-exact", because for this one class nothing can establish it.
_ZTV14dScMgD3DBase_c's recorded extent stops at 20 words:
_ZTV14dScMgD3DBase_c kind:data(any) addr:0x0213c62c
data_ov006_0213c67c kind:data(any) addr:0x0213c67c ambiguous <- slot 20
So a 34-slot prefix is being asserted against a symbol whose tracked extent covers 20 of those 34 slots. Its two siblings are fine — dScMgBase_c and dScMgSingle3DBase_c both run the full 36 to the next symbol.
data_ov006_0213c67c is not a real object. It is the slot-20 word itself:
ov006/relocs.txt from:0x0213c67c kind:load to:0x020b2990 module:overlays(0,4)
config/arm9/relocs.txt from:0x02090ba0 kind:load to:0x0213c67c module:overlays(6,98)
The first row is the vtable slot doing its job — 020b2990 is exactly the word I dumped at index 20. The second is an arm9 pointer table entry that dsd could not resolve to one overlay, so it marked it ambiguous and fanned it into ov006. Overlays share address space; the pointer is overlays(6,98)'s to disambiguate, and the symbol it produced in ov006 is an artifact of that.
Two things follow, and I want to be clear about which is yours:
Not yours, and please do not fix it here. The truncated extent is pre-existing debt on main. I have it open separately, and I have measured that correcting it moves a ratchet — it is its own PR with its own baseline conversation. Repairing it as a rider on this one would put a ratchet movement inside a slot-declaration PR, which is exactly the mixing I have been asking the stack to avoid.
Yours, and it is one sentence. The header states byte-exact for a class where the tracked extent ends at slot 20. Either drop byte-exact for that class specifically, or add a clause noting the extent is truncated at 20 words by an ambiguous cross-overlay symbol and that the 34-slot agreement is read from the cartridge rather than from tracked config. The dScMgSingle3DBase_c header can keep its wording unchanged — its extent is genuinely 36.
While I was there: no workflow runs romdata_check.py. It appears in source-coverage.yml only inside a comment, and tool-tests.yml runs test_romdata_check, its unit tests, not the gate. It is not in the pre-push hook either. So no CI check on this PR — or any of the six — evaluates whether an emitted vtable agrees with the cartridge. That is not an objection to this PR; it is why I read the bytes myself, and why the wording of a byte-exact claim matters more here than it normally would.
Gates
vs origin/cpp/minigame-slot32 @567649493a6b merge tree 04718d0ca6a3 8/8 pass -> pass
vs origin/main @5e88bfb5ed0f merge tree 32641c686d21 8/8 pass -> pass
All seven GitHub checks green, PR validation SUCCESS. The config half carries no coined data symbols — I check for that specifically after #2096 and #2104, and there are none: all four renames are address- and size-preserving function renames.
Off-by-one, unchanged status
This PR does not add one. Slot 33 has three bodies and a void reading, so it correctly breaks the "consecutive slot no body pins" chain rather than extending it. The two stale sentences are still inherited from below:
:606 slot 31 -- "the third consecutive slot no body pins" <- should be fourth
:693 slot 32 -- "and the FOURTH consecutive slot no body pins" <- should be fifth
Fix them in whichever of #2106/#2107 lands second, as I asked there — not here, or all three PRs conflict on the same lines.
Ordering, and what I would like to happen
Stack is #2099 → #2100 → #2102 → #2106 → #2107 → #2108. The five below all carry changes-requested from me on one thing: header declares int, definition defines void, eight symbols, no tool in the tree compares the two surfaces.
This PR resolves that question, and I would like the stack to adopt its answer. Slot 33 declares void because the bodies return nothing and one of them is a real member definition where int would force a manufactured return 0;. That reasoning applies unchanged to slots 28 through 32 — none of those bodies sets a result either, and #2064-style promotion will eventually reach them too. Flip those five declarations to void at the bottom of the stack and every changes-requested I hold on this campaign clears in one edit.
On content this is the first PR in the stack I would approve. It is held only by its base.
1 participant
Slot 33 of the
dScMgBase_ckeystone. Stacked on #2107 (slot 32); base it oncpp/minigame-slot32, land it after that one.What lands
dScMgBase_cdeclaresvirtual void Virtual84(); /* slot 33 */, and the two classes that override it —dScMgSingle3DBase_canddScMgD3DBase_c— declare it too. All 19 tables that carry the slot now emit it from source; 17 of them inherit.Structurally this is slot 26 again — the same 19 tables, the same two owning classes (
dScMgSingle3DBase_c+ 13 children,dScMgD3DBase_c+ 4), the same 17 inherited. Two things make it different.1. The only slot in the campaign with nothing to correct
Slots 26, 29, 30, 31 and 32 each retired a name the recovery pass had borrowed across a fork of the hierarchy. Here the pass guessed nothing: there is no
recovered name:line on the base body or on either override. Neither body even had a header comment — the two files began straight at#include "types.h". Both now carry one.Virtual84is the repo's own no-name spelling after the+0x84vtable offset, the conventionfBase_calready uses forVirtual34/Virtual38. The ROM names nothing here:dScMgBase_cis a scene (fBase_c -> dBase_c -> dScene_c -> dScMgBase_c), anddActor_c— whose names slots 18-30 borrowed by index — has no slot 33 at all.2. The first override inside a promoted intact-object TU
dScMgSingle3DBase_c's body lives insrc/actors/dScMgSingle3DBase_c.cpp, promoted in #2064, so the rename is an in-file identifier rewrite rather than a file move. With the base declared, the body becomes a real member definition —void dScMgSingle3DBase_c::Virtual84(), mangling to the same symbol the free function was hand-named. That is exactly what slot 26 did in that same file three commits ago, and that comment ended "Slot 33 below is still waiting on the same declaration."It was the last vtable slot in that unit still spelled as a mangled free function. The one function left there under a
func_<module>_<address>name,0x0210a534, is in no vtable at all — confirmed by scanning every_ZTVin ov006 for that address and finding zero references. The class emits a byte-exact 34-slot prefix now.Signature evidence
Return type
void. None of the three bodies assigns a result and the one caller ignores whatever falls out. It is also the honest choice rather than a default: the Single3D body is a real member definition now, and a declaredintwith noreturnstatement would be a lie in source.No explicit parameters, measured. Scanning arm9 and all 103 overlays for the dispatch pair —
ldr rD,[rN,#0x84]withRn != pc, followed within three instructions byblx rD/bx rD— finds exactly two sites image-wide, of which one is in ov004 or ov006:r1 is the loaded pointer, so it cannot also be a second argument.
thisis carried, and the base body proves it: it writesobj[0x68]and*(int*)(obj+0x6c).Three refinements over the naive scan, all of which matter:
insn & 0x0FF00FFF == 0x05900000 | off, not0x0FFF0FFF— bits 19-16 are Rn, so masking them in only ever matches one base register;Rn == 15is excluded (pc-relative literal-pool loads at the same encoding);+0x84and would have produced a false "this slot is never dispatched" claim.The scanner was validated against a known answer before being trusted here: re-run at
+0x80it returnsov004:0x020b0900, reproducing slot 32's call site.What it does, and when
Engine bring-up. Graphics modes for both engines, VRAM banks assigned, both BG-enable shadows (
data_0209d45cmain,data_0209d454sub) initialised to0x10— the value slots 30 and 31 later save, clear bits out of, and restore. A language-indexed character file is decompressed into both engines' BG char VRAM, OBJ palette file0xc3is loaded into both, and the scene object is published into the global registry atdata_ov000_020beb74.The three bodies differ the way 2D and 3D differ: the base is the plain 2D sequence;
dScMgD3DBase_caddsDisableAllBanks, texture and texture-palette banks, and keeps the decompressed file pointers alive instead of freeing them;dScMgSingle3DBase_cadds G3X fog setup.Ordering, from
BeforeInitResources(ov004:0x020b0930): slot 33 is dispatched near the top, slot 31 last before the function returns 1, and slot 32 out ofAfterInitResources. Bring the engines up (33), dress the sub screen (31), dress the main screen (32).Verification
rombuild.py -j 16 --no-rom: 11,088 / 11,088 reproducing, 106/106 modules exact, 100.000000% of compared bytes.romdata_check --json, diffed against the slot-32 baseline in both directions:verifiedBytes35,420 → 35,424 (+4),partialBytes11,872 → 11,996 (+124). That is 128 bytes = 32 tables × 4. Every other counter and the entiredifferingset are unchanged.check_dead_references,port_refcheck,check_duplicate_sources,check_src_tu_compiles(97/97),check_header_offsets --changed origin/main, langmode ratchet: all pass.Prose re-counted
dScMgD3DBase_c.h's "one more slot stays undeclared" block is now empty — all nine of that class'sdScMgBase_coverrides are spelled out.dScMgAmida_c.h'sUnk36moves from landing on 33 to landing on 34 (still the family's lastDIFFERS; declaring slot 35 puts it on 36 and closes it).dScMgSingle3DBase_c.hgoes from a 33-slot to a 34-slot prefix and "remaining three (33-35)" to "remaining two". The TU manifest'sfunctions[8].symbolbecomes the mangled name,legacy_sourceleft alone per the slot-26 precedent in638b24c84.Does NOT establish
Virtual84is a placeholder, not a ROM name. Nothing in the cartridge names this function.src_turegeneration is still outstanding.src_tu/actors/unit_ov004_020b0a38.cppholds the base bodies for slots 31, 32 and 33 under stalefunc_ov004_*names. It is"status": "text-verified", not enrolled, andsrc_tu/is not in the ROM build, so both gates that read it are green — but it is due now, throughtools/tubuild.py, as its own follow-up PR. Folding it in here would break the one-slot-per-change safety argument.dActor_c's borrowed names remains the owner's call.Two slots left: 34 (
Virtual88, 4 tables) and 35 (Virtual8C, 1 table). Slot 35 closes_ZTV12dScMgAmida_cand takes_ZTV11dScMgBase_cfrom PARTIAL to VERIFIED at 36 slots.