Spell slot 28 void, so no declaration outlives what a body can honour - #2124
Closed
andrewboudreau wants to merge 1 commit into
Closed
Spell slot 28 void, so no declaration outlives what a body can honour#2124andrewboudreau wants to merge 1 commit into
andrewboudreau wants to merge 1 commit into
Conversation
dScMgBase_c::OnHitFromUnderneath was declared `int` and neither definition produced an int. dScMgSlot1_c's override fell off the end of a non-void function, which is undefined behaviour; it byte-matched only by accident of what SetSubBg1Offset happened to leave in r0. The base's own definition and dScMgD3DBase_c's flat-C one were already `void`. Four declarations of the slot existed and three are in this hierarchy, so they move together or the override stops overriding: include/dScMgBase_c.h:420 int -> void include/dScMgD3DBase_c.h:97 int -> void include/dScMgSlot1_c.h:158 int -> void include/dActor_c.h:145 is deliberately untouched: it is the parallel hierarchy and takes a `dActor_c &`, so it is a different function that merely shares an index. The ROM is neutral here and this PR does not claim otherwise. The slot stays UNPINNED -- `int` and `void` compile to the same bytes, which the branch's own comment already said. `void` is chosen because it is the only spelling both definitions can satisfy, not because the cartridge prefers it. If a caller is ever found consuming this slot, that measurement is what should change it back. Measured on this tree, matching the base branch's own figures exactly: reproducing: 11,088 mismatching: 0 module fidelity: 106/106 exact, 100.000000% ROM-build analysis: PASS Also drops the third and last copy of the "no caller loads vtable+0x70" claim, which ed243e8 corrected in the header's evidence block but not in src/_ZN12dScMgSlot1_c19OnHitFromUnderneathEv.cpp. It is replaced with what was actually measured -- three dispatch sites, all discarding -- and with a note on why the original was wrong, since a bare `ldr` at +0x70 is a field read and only the load/`blx` pair is a dispatch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ
✅ PR validation — PassedCommitted merge introduces no reconstruction or attribution regression. Full merge validation
Byte-verified means the range carries Per-file link-check detailAll 189 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
commented
Sep 1, 2026
andrewboudreau
left a comment
andrewboudreau
left a comment
Collaborator
Author
There was a problem hiding this comment.
Closing this. The fix is right; the base branch is wrong, and landing it here would break the build once the stack rolls up.
What I missed when I opened it
I cut this against cpp/minigame-slot28 (#2099), where the slot-28 headers say virtual int and the two mangled-name bodies say void:
include/dScMgBase_c.h:456 virtual int OnHitFromUnderneath();
src/_ZN11dScMgBase_c19OnHitFromUnderneathEv.cpp:32 extern "C" void _ZN11dScMgBase_c19OnHitFromUnderneathEv(void *c)
src/_ZN14dScMgD3DBase_c19OnHitFromUnderneathEv.c:26 void _ZN14dScMgD3DBase_c19OnHitFromUnderneathEv(void)
That reading is correct at this branch. What I did not check is that the same split is already resolved twelve PRs up the stack, in the opposite direction. Commit 8fb19d997 (#2114/#2115/#2116/#2118) converts all three bodies into real members and spells them int to match the headers — per its own recipe step 4, "spell the definition as <declRet> Class::Method(<declParams>)".
So there are two coherent resolutions of one split, and this PR holds the one that loses on position.
The merge is textually clean and then does not compile
Neither change touches the other's lines, so nothing conflicts. I applied this PR's header flip to the stack top (#2118 head 66f3c9360) and built:
_ZN14dScMgD3DBase_c19OnHitFromUnderneathEv.cpp:31: identifier 'dScMgD3DBase_c::OnHitFromUnderneath()' redeclared
was declared as: 'void ()'
now declared as: 'int'
_ZN12dScMgSlot1_c19OnHitFromUnderneathEv.cpp:44: same
_ZN11dScMgBase_c19OnHitFromUnderneathEv.cpp: same
Three hard errors, and the intact-TU link control fails behind them. Two green PRs, a red main — path disjointness proving nothing, again.
The change itself is still right, and I still intend to land it
Not retracted, just relocated. The evidence for void is unchanged and #2114's own prose is the strongest witness for it:
THE RETURN TYPE IS THE ONE THING HERE THAT IS NOT MEASURED, and slot 28 is the first in this campaign where that is true. ...
intis include/dActor_c.h's, kept because it is what this file already carried ... a hint, not a measurement.voidcompiles to the same bytes.
Three things make void the better spelling:
- No slot-28 body returns a value. Not one of the three. Under
int,dScMgBase_c::OnHitFromUnderneathcarries a barereturn;inside a value-returning function and then falls off the end, anddScMgSlot1_c's does too. The declaration describes a value no body produces. - The ROM is neutral.
voidandintare byte-identical here, so nothing is being traded away. - The one source arguing for
intisinclude/dActor_c.h— a parallel hierarchy this campaign has now measured wrong on slots 18, 29 and 32, and which #2114 itself overrules twice.
But a return type has to move every declaration and now every definition in one commit, and after 8fb19d997 those definitions exist. That is a change against the stack top, not against its base.
Plan
#2099 merges on its own — its shape compiles cleanly, because the base and D3DBase bodies are free functions carrying mangled names and the compiler never cross-checks those against the class declaration. Once the whole stack is on main I will re-cut this as a single atomic flip of the three headers and the three member definitions, with the byte evidence attached.
Closing rather than retargeting: rebasing a two-word fix onto a moving 12-PR stack costs more than re-cutting it once the stack is still.
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.
Opening this against
cpp/minigame-slot28rather than pushing to it, exactly as offered on #2099 — the branch isn't mine and I'm not going to rewrite someone else's work uninvited. Merge it, cherry-pick it, or reject it and do your own; any of those clears the block.What it changes
dScMgBase_c::OnHitFromUnderneathis declaredintand neither definition produces one:src/_ZN12dScMgSlot1_c19OnHitFromUnderneathEv.cppisint dScMgSlot1_c::OnHitFromUnderneath()and falls off the end. Returning nothing from a non-voidfunction is undefined behaviour. It byte-matches by accident of whatSetSubBg1Offsetleaves in r0.src/_ZN11dScMgBase_c19OnHitFromUnderneathEv.cppis alreadyextern "C" void.src/_ZN14dScMgD3DBase_c19OnHitFromUnderneathEv.cis alreadyvoid.So the declaration is the outlier, not the bodies.
Three declarations, not one
This is the part worth checking if you'd rather write it yourself. Four headers declare the name; three are in this hierarchy and have to move together or the overrides stop overriding and mwcc numbers a new slot:
include/dActor_c.h:145is deliberately not touched — it is the parallel hierarchy, it takes adActor_c &, and it is a different function that merely shares an index.include/QuestionBlock.h:75is downstream of that one and likewise untouched.What this does not claim
The ROM is neutral at slot 28 and nothing here says otherwise. The slot stays UNPINNED.
intandvoidcompile to the same bytes — your own comment says so, and I re-measured it.voidis chosen because it is the only spelling both definitions can honour, not because the cartridge prefers it. I've written that reasoning into the header so the next reader doesn't mistake it for a measurement, and left the note that if a caller is ever found consuming this slot, that is what should change it back — notdActor_c.h.I'm being explicit about this because I retracted a "slot 29 is void, proven by the build" claim of my own earlier in this campaign. A green build is neutral on return type. This is an internal-consistency fix, not a discovery.
Measured on the merge result
Identical to the figures in your PR body. Nothing moves.
One thing I fixed while in there
ed243e83ccorrected the "nothing loads vtable+0x70" claim in the header's evidence block, but the third copy of it is still insrc/_ZN12dScMgSlot1_c19OnHitFromUnderneathEv.cpp. It now says what was actually measured — three dispatch sites, all discarding — plus a short note on why the original was wrong, since a bareldrat +0x70 is a field read and only the load/blxpair is a dispatch. That felt worth keeping as a record rather than silently deleting.Why now
#2099 is the bottom of a twelve-PR stack (#2100 → #2102 → #2106 → #2107 → #2108 → #2110 → #2112, and #2114 → #2115 → #2116 → #2118). Nothing in the 18–35 range can land until it moves, and I've been the one holding it for four rounds over two words. This is me clearing my own block rather than asking for a fifth round.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WdCK1xgrJdiJzPCh3bAJfQ