Inline base destructors: the shape the ROM's own sources had - #1369
Inline base destructors: the shape the ROM's own sources had#1369andrewboudreau wants to merge 1 commit into
Conversation
❌ PR validation — FailedValidation failed: full-ROM result regressed from the base commit; full-ROM validation failed Full merge validation
Warnings: 2 linkcheck result(s) have unresolved relocations; 3 affected source file(s) could not be fully link-checked. Per-file link-check detail3 of 390 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. |
|
Not merging this one: the byte gate is red on the full-ROM leg ( It is not a stale-branch collision. Since this branch's merge base, main changed exactly one line of Two enrolled TUs now define Both are enrolled, at I am not claiming that duplicate is the The design argument reads as right, for what it is worth: Left open and unmerged. Everything else from today is in: #1361, #1362, #1367, #1368, the sweep as #1360/#1363/#1364/#1365/#1366, and #1359 restoring the orphaned #1357. |
|
Update, and it points away from the duplicate being fatal. #1370 passed the full byte gate, including I merged current main into this branch (clean, no conflicts, langmode ratchet PASS) to trigger a fresh validation run. The duplicate is unchanged by that merge, as expected. If the rerun is green I will merge this and then #1370; if it fails the same way while #1370 stays green, the difference between the two runs is the thing to chase, and it is in the build path rather than in the source. |
|
More evidence, and it narrows to an interaction with main rather than anything in this branch. What builds and what does not.
#1370's tree contains every one of this branch's changes and builds clean, so the inline destructor and the duplicate It is not a plain compile error in the enrolled set. I compiled all 10,508 files in Prime suspects in main, all landed today, all touching the same ground: #1362 (eleven engine base destructors, and main's own I am not pushing anything further to your stack. Two options, your call: rebase the stack onto current main and rerun, or merge #1370 into this branch first, since Also worth knowing: #1371 fixed |
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
1cdb296 to
d4dbfd9
Compare
|
Rebased onto Reproduced the validator's exact procedure locally, with the compiler and ROM: merged this branch's old head into then-current main and ran Rebased the branch onto current main rather than leaving the two Rebuilt after the rebase: 106/106 exact, 0 mismatching. The whole stack was restacked on top of it and each level builds — see the comments on #1370, #1372 and #1375. One of them needed a real fix, and it was mine, not yours. |
|
Re-validated on the rebase, and it still fails — but the stack above it passes, which narrows this to "must not merge alone". Results on the current head SHAs, all against
That is the same shape as the earlier report — The likely mechanism, and it is a hypothesis, not a measurement: this PR changes What I could not do is reproduce the failure. On this exact commit, with the compiler and ROM, Recommendation: do not merge this one on its own. Either merge #1369 and #1370 as a unit, or move #1370's |
|
Superseded by #1370, which now targets This PR could not merge, and being the base of #1370 it blocked the whole stack. Rather than keep guessing at a failure I cannot reproduce, I moved #1370's base to Correcting the weight of my previous comment. I cited #1370 and #1375 passing as evidence, and each stacked PR validates only its own delta against its own base — so neither of them re-tested this commit against One more measurement, in case it helps whoever picks this up: the |
|
Closing: your commit is on main, and this PR's diff has now inverted into a revert.
Because #1370 squash-merged, git cannot see this branch's commit as already applied, and main has since taken #1372 and #1375 on top. So Reopen and rebase if I have misread anything. For the record, the failure here was never explained. |
|
Correcting myself twice, so you do not chase leads I have since disproved. Retracted 1: duplicate symbol definitions. I measured two enrolled TUs both defining Retracted 2: newly enrolled address ranges. I posted a table on #1377 pointing at the
No PR in this group enrolls a single new byte. Whatever the report's What is actually established, all measured rather than reasoned:
I have no further testable hypothesis, and I cannot bisect a real build: Stopping here rather than generating more guesses. The scripts are throwaway but the method is repeatable if useful: compile every enrolled file under its pin, collect each object's defined GLOBAL/WEAK symbols, and diff delinks by range rather than by path. |
2 participants
Inline base destructors: the shape the ROM's own sources had
Scene::~Scene stores TWO vptrs and then calls ActorBase's destructor:
A merely declared
virtual ~ActorDerived();cannot produce that. The compilerhas no body to inline and emits
bl _ZN12ActorDerivedD2Ev-- one store wherethe 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.
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
_ZTV12ActorDerivedis 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.
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.
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.
Independent of #1368, which measured this route and left it for its own PR.
🤖 Generated with Claude Code