Promote daObjHatenaSwitch_c to a genuine C++ translation unit - #2062
Conversation
✅ PR validation — PassedCommitted merge passes; 7 contributor credit reassignment(s) noted, not a blocker. Full merge validation
Contributor credit moved (7)
Byte-verified means the range carries Warnings: contributor attribution changed, not a blocker (7 changed, 0 lost -- src/actors/daObjHatenaSwitch_c.cpp: tangosdev -> github-actions[bot]; src/actors/daObjHatenaSwitch_c.cpp: tangosdev -> github-actions[bot]; src/actors/daObjHatenaSwitch_c.cpp: tangosdev -> github-actions[bot]; +4 more); 13 address range(s) left the byte-verified set while enrolled totals held steady: ov002:0x020b4ed8-0x020b4f44, ov002:0x020b4f44-0x020b4fc4, ov002:0x020b4fc4-0x020b4fd0, ov002:0x020b4fd0-0x020b503c, ov002:0x020b503c-0x020b50a0, +8 more; 12 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 6 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
added
the
attribution-override
Conflict diagnosed: one file, and it's the non-goal one. Here's the exact resolution.This went What actually conflictsOne file, and it's What landed under you
How to resolve
|
8488803 to
08401d6
Compare
Correction to my own comment: the rebase target I named is already staleI told you to rebase onto Neither of the two commits after The validator failure is the conflict, not a gateFor the record, so nobody reads That is the validator refusing to construct a merge it cannot construct. It is not a byte, coverage, ratchet, or reference finding — there is no verdict on your work in there at all, in either direction. It should clear on the rebase. If it comes back red for any other reason afterward, that is the one worth reading closely. What I am doing meanwhileI have your substance under review in parallel — the TU shape, the delink/symbol edits, and the One thing worth saying plainly since it will affect your re-measurement: |
Changes requested — the 8 backslide exceptions are avoidable. Ten comment lines make them unnecessary.The rebase landed cleanly. Measured just now: head They are green because this PR silences the ratchet. It drops CONVERTED from 2,568 to 2,560 and adds 8 backslide-exception rows to cover the drop. I measured that it does not have to. What the ledger actually saysTen out, two back. The missing eight are exactly the eight rows added to the exceptions ledger. Why it happens — and it is not the code
/* ROM ordinal 7 -- Render, 0x020b51ac, size 0x30. */
s32 daObjHatenaSwitch_c::Render()
{
mModelAnim.Render(0);
return 1;
}That body is character-for-character what The two members that do survive — Measured, not arguedI extracted your merge tree The criteria move exactly where the diagnosis predicts: Zero backslide. All eight exception rows become unnecessary. The markers, one line immediately above each definition:
These are comments. The same lane got this right an hour later#2064 ( On the exception rows themselvesSeparately from whether they survive: all eight carry the same copy-pasted reason, and it does not describe what is being excused.
This part is on the docs, not on you
What I need
Nothing here asks you to change a byte of decomp work, and none of it is a criticism of the TU itself — the boundary, the manifest and the byte claims all look right to me, and I have a second review running on the substance which I will post separately. I will not merge a PR that lowers the CONVERTED count while I can demonstrate it does not have to. Green here means the exceptions worked, not that the ratchet held. |
The rename half of this PR is ROM-proven correct, and it is more valuable than the PR saysSeparate from the blocker above — I want this on the record, because it turned out to matter across the whole queue.
I walked that address in
Why this is the load-bearing part of the PRI measured every That is #2066, which spent a full destructor migration before the promotion turned out to be impossible. The pattern holds across everything that has landed: of the 23 manifests on So ov002 is also where the disagreements concentrate — 35 of the 259 live there, more than any other module, This does not change the blockerThe eight rows added to |
Now DIRTY, but only trivially — one file#2064 and #2069 landed, so this branch needs a refresh. The conflict set is small;
Your three commits ( The eight |
andrewboudreau
left a comment
andrewboudreau
left a comment
There was a problem hiding this comment.
Conflict resolved — pushed the merge, with the arithmetic for each of the three files
This branch had been DIRTY against main for a while and the three conflicting files are all ones where picking a side gives a wrong answer, so I resolved each by measurement rather than by choosing --ours/--theirs. Merge commit is on the branch now.
config/converted-baseline.json
The conflict was the count scalar only — the converted array itself merged cleanly, which is exactly the case where taking either side's count silently corrupts the ratchet. Read from the three merge stages:
base count=2568 len=2568
ours (PR) count=2560
theirs (main) count=2571
added by PR (O-B): 2 removed by PR (B-O): 10
added by main (T-B): 23 removed by main (B-T): 20
overlap of the two removal sets: 0
Correct three-way set size is (B∩O∩T) | (O−B) | (T−B) = 2563, and 2568 − 10 − 20 + 2 + 23 = 2563 confirms it. That's the value written. tiers_ratchet.py --check passes on it (baseline 2563, current 2564 — one gained, not yet banked).
Worth stating explicitly because it reads alarming otherwise: main's 20 "removals" are not backslides. They are TU-promotion renames — src/_ZN10dScMgBSC_c6RenderEv.cpp becomes src/actors/dScMgBSC_c.cpp#_ZN10dScMgBSC_c6RenderEv. Every one has a paired addition.
attribution.json
The real map is nested under overrides, not at top level. Audited there:
base 1005 ours 1018 theirs 1074
added ours 13 added theirs 73 add-overlap 0
deleted ours 0 deleted theirs 4
value changed: ours 0 theirs 0 both 0 <- zero true conflicts
Result is 1087 entries, ours-order preserved with theirs' additions appended, indent taken from the base blob so the diff stays reviewable. Main's 4 deletions are honoured — all tangosdev, all promotion renames (dScMgCard_c and dScMgMCarlo2_c D0/D1).
notes/cpp-tu-current-state.md
Generated file (tools/cpp_tu_state.py, header says so), so it was regenerated rather than resolved. --write-note then --check-note → current.
One number in that note needs context before anyone blames this branch
The regenerated note moves "Existing promotion paths that disagree with delinks" from 0 to 4. That is not caused by this branch, and I checked before saying so rather than after.
origin/main at 2f62cd870 fails its own cpp_tu_state.py --check-note — main's committed copy of that note is stale. cpp_tu_state is wired into no workflow and no hook (grep -rn 'cpp_tu_state' .github/workflows/ tools/hooks/ is empty), which is how it drifted. I built a clean control worktree at origin/main and measured live: promotion_mismatch_entries is byte-identical on main and on this merge tree. The four are ov006/dScMgBSC_c, dScMgCard_c, dScMgMCarlo2_c, dScMgMCarlo_c — all main's own promotions.
So regenerating the note doesn't introduce those four, it stops hiding them. Real live main is 25 enrolled / 4 disagreements, not the committed 29 / 0. Filed separately; it should not gate this PR.
Verification on the merge result
premerge_check --base origin/main <merge>
converted-ratchet pass -> pass langmode-ratchet pass -> pass
dead-references pass -> pass layout-check pass -> pass
duplicate-sources pass -> pass src-tu-refs pass -> pass
header-offsets pass -> pass source-coverage pass -> pass
RESULT: nothing goes green -> red
Static gates were never the risk here, though. This merge lands two independent TU-promotion sets into the same ov006 delinks, and a clean text merge proves nothing about the link — so I also ran a full rombuild --no-rom on the merge result:
intact TU gates: dsd modules PASS, zero new symbol errors, storage aliases exact
source-built functions: 11,088 reproducing: 11,088 mismatching: 0
module fidelity: 106/106 exact, 100.000000% of compared bytes
ROM data from source: 529 verified, 192 partial, 6 differ
The intact-TU line is the one I was actually watching, and it passes.
What this does not resolve
My earlier CHANGES REQUESTED still stands and is a separate matter from the conflict. The branch removes 10 src/_ZN14QuestionSwitch*.cpp rows from the converted baseline and re-adds only 2 (daObjHatenaSwitch_c D0/D1). That is an 8-row backslide, and unlike main's 20 it has no paired additions — those functions stop being credited. I deliberately did not paper over it in this merge; the merge preserves your side of that difference exactly as you wrote it, so the fix is still yours to make and the ratchet arithmetic above will absorb it cleanly when you do.
andrewboudreau
left a comment
andrewboudreau
left a comment
There was a problem hiding this comment.
The 8 backslide waivers are covering a missing convention, not a real regression — measured
The conflict half is resolved and premerge is 8/8 pass→pass on the real merge tree, so config/converted-backslide-exceptions.jsonl is the only thing left between this and a merge. I dug into it rather than just accepting or rejecting the rows, and the result inverts the fix.
The finding
src/actors/daObjHatenaSwitch_c.cpp carries zero // @symbol markers. Every promoted TU on origin/main has them:
32 src/actors/dScMgCard_c.cpp 16 src/actors/daBrq_c.cpp
21 src/actors/dScMgMCarlo_c.cpp 8 src/actors/dScMgBase_c.cpp
21 src/actors/dScMgMCarlo2_c.cpp 7 src/actors/daObjPushblock_c.cpp
17 src/actors/dScMgBSC_c.cpp 3 src/actors/daObjKm3_Kurumajiku_c.cpp
Without them tiers.score_member() cannot bound a member's text, so every member gets charged the file-level mangled extern "C" block at lines 43-79 and the mangled call sites. That fails no_mangled_refs and no_codegen_trick for bodies that contain neither.
The bodies did not change. HandleClsn is character-for-character what it was at the merge-base as src/_ZN14QuestionSwitch10HandleClsnER8dActor_c.cpp — real names, no offsets, no _Z. It was CONVERTED then and it reads as CONVERTED now.
Measured, at this PR's exact head (7848aa2)
tiers.score_member(path, text, symbol, repo_root) for each waived symbol, as-is and with // @symbol <mangled> inserted before each definition — no other change:
_ZN19daObjHatenaSwitch_c17AfterClsnCallbackEP4dBgWP8dActor_cS3_ False -> True
_ZN19daObjHatenaSwitch_c10HandleClsnER8dActor_c False -> True
_ZN19daObjHatenaSwitch_c6RenderEv False -> True
_ZN19daObjHatenaSwitch_c16CleanupResourcesEv False -> True
_ZN19daObjHatenaSwitch_c20UpdateModelTransformEv False -> True
_ZN19daObjHatenaSwitch_c19UpdateClsnTransformEv False -> True
_ZN19daObjHatenaSwitch_c15UpdateClsnStateEv False -> True
_ZN19daObjHatenaSwitch_c15OnGroundPoundedER8dActor_c False -> True
Eight for eight. End-to-end, with the ten markers added and all 8 waiver rows deleted:
as-is: CONVERTED ratchet PASS baseline 2563 current 2564 (+1)
marked: CONVERTED ratchet PASS baseline 2563 current 2572 (+9)
So the waivers aren't buying the gate a pass it needs — the gate passes either way. They're buying it at the cost of eight CONVERTED members this PR is currently forfeiting, and of a permanent record in a tracked config file saying those eight regressed when they didn't.
Byte-neutral, same tree, full build:
source-built functions: 11,088 reproducing: 11,088 mismatching: 0
module fidelity: 106/106 exact, 100.000000%
ROM-build analysis: PASS
(one recompile of the TU, still exact — markers are comments, but I measured rather than assumed).
Ask 1 — add the markers, drop all 8 rows
Ten definitions, at these lines in the current file:
113 AfterClsnCallback 120 HandleClsn 135 InitResources
179 Behavior 281 Render 288 CleanupResources
302 UpdateModelTransform 316 UpdateClsnTransform 327 UpdateClsnState
343 OnGroundPounded
and delete the eight daObjHatenaSwitch_c.cpp#... lines from config/converted-backslide-exceptions.jsonl.
Two members still score False with markers in place, and correctly so:
InitResources fails no_mangled_refs
Behavior fails no_codegen_trick, no_mangled_refs
Neither needs a row — the ratchet passes without one, which is the measurement proving they weren't in the CONVERTED baseline before promotion either. Leave them honest.
Ask 2 — the reason strings describe a symbol that isn't among them
All eight rows carry a byte-identical reason, whose second clause is:
"...and Behavior needs the volatile stack value to avoid a 17-word codegen mismatch..."
Behavior is not one of the eight. It's the one member that genuinely does carry a codegen trick — and it has no row and needs none. So a rationale written for Behavior got pasted onto eight members it doesn't describe.
A waiver file is exactly the place where the reason has to be per-symbol: it's the only record a future reader gets of why a regression was accepted, and eight copies of someone else's reason is worse than no reason. If any row survives this review it needs its own text.
Where that leaves the merge
This is not a "fix forward" for me, because the rows are the artifact — landing them writes a false regression into tracked config that nothing later will re-examine. But it's a small, mechanical, measured fix, and the numbers above are the whole argument. Push the markers and drop the rows and I'll re-read and merge; everything else on this PR is clear.
Happy to push the change to your branch myself if you'd rather — say the word and I will, otherwise it's yours.
andrewboudreau
left a comment
andrewboudreau
left a comment
There was a problem hiding this comment.
Heads-up from the merge lane, not a content objection: this PR went DIRTY when #2103 and #2125 landed. Nothing about the review verdict changes.
git merge-tree --write-tree origin/main <head> on the current tips:
#2062 CONFLICT (content): notes/cpp-tu-current-state.md
#2104 CONFLICT (content): attribution.json
CONFLICT (content): notes/cpp-tu-current-state.md
Both files are the standing serial chokepoint for this lane — every TU promotion rewrites them, so any two TU PRs in flight collide there regardless of how disjoint their source is.
Do not hand-resolve notes/cpp-tu-current-state.md. It carries its own instruction at the top:
<!-- Generated by tools/cpp_tu_state.py. Do not hand-edit live counts. -->
Take either side to clear the marker, then regenerate and check:
python tools/cpp_tu_state.py --write-note
python tools/cpp_tu_state.py --check-note
Hand-merging it produces a file that is textually plausible and numerically wrong — the counts are derived from the post-merge tree, not from either parent, so neither side's numbers are right and a hunk-by-hunk resolution splices two stale snapshots. --check-note is what catches that.
attribution.json is the other one: it conflicts three-way, not as a key union. Resolving it by taking the union of keys silently drops the third parent's edits. Attribution is a stated non-goal here, so if it fights you, take main's and move on rather than spending time on it.
Once you push the restack I'll re-run the gates on the new merge tree and pick the review back up where it stands.
7848aa2 to
1db3ee8
Compare
|
Refreshed and reviewer blocker resolved on head
Fresh validation from a clean wired worktree:
The attribution-override label remains appropriate for the ROM-proven |
1 participant
Promotes the 13-function ov002 HatenaSwitch packet into one genuine daObjHatenaSwitch_c C++ translation unit using the ROM-proven RTTI class identity.
The compiler now owns D1, D0, RTTI, ZTS, and the vtable; it emits no D2 or forcing helper. QuestionSwitch_Spawn and SpawnInfo remain narrow evidence-bounded C ABI aliases because the original factory spelling does not survive and natural new selects the wrong allocator.
Validation: