Promote daObjKm3_Kaitendai_c as a compiler-owned C++ TU - #2111
Conversation
andrewboudreau
added
the
attribution-override
✅ PR validation — PassedCommitted merge passes; 4 contributor credit reassignment(s) noted, not a blocker. Full merge validation
Contributor credit moved (4)
Byte-verified means the range carries Warnings: contributor attribution changed, not a blocker (4 changed, 0 lost -- src/actors/daObjKm3_Kaitendai_c.cpp: tangosdev -> github-actions[bot]; src/actors/daObjKm3_Kaitendai_c.cpp: lunavyqo -> github-actions[bot]; src/actors/daObjKm3_Kaitendai_c.cpp: lunavyqo -> github-actions[bot]; +1 more); 5 address range(s) left the byte-verified set while enrolled totals held steady: ov047:0x021112bc-0x0211130c, ov047:0x0211130c-0x02111370, ov047:0x02111370-0x02111384, ov047:0x02111384-0x021113bc, ov047:0x021113bc-0x021113f8; 4 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. |
andrewboudreau
left a comment
andrewboudreau
left a comment
There was a problem hiding this comment.
Reviewed at bf06f1b025fd. I built this branch myself and every claim in the description reproduces. I also found something the PR does not know about itself: the one symbols.txt deletion — which is correct and required — costs this PR a false PARTIAL on one of its own new data records, for a reason that is a defect in romdata_check, not in your work. Details below, plus a census showing it affects 48 records tree-wide.
Approve on content. I am not asking you to change anything in the diff.
What I reproduced
Cut a worktree from your head, built it, and compared against a build of your base at b65a75427003.
source-built functions: 11,088 (2,067,148 / 2,211,124 code bytes, 93.49%)
reproducing: 11,088 mismatching: 0
source-owned data claims: 5 (reproducing 5, mismatching 0)
module fidelity: 106/106 exact, 100.000000% of compared bytes
ROM-build analysis: PASS
sha256(build/sm64ds_kt.nds) = d1506e90efae5e2d2cf119926a4ac2a291bd5ca78349d09d5024e1a918c478e8
That sha256 is the stock cartridge. I computed it from my own link, not from your report.
The baseline control in the same run is honest about what it does not cover: dsd check symbols --fail reports 9 errors (overlay_100, overlay_102, data_020ad524, data_020ad560, and five ITCM symbols). Those 9 lines are byte-identical in the no-substitution baseline and in your head build, so they belong to the tree and not to this TU.
The per-symbol data diff: a strict superset, no swap
The rule I hold TU promotions to is diff --data-json per symbol, never the summary — a promotion can raise the headline count while quietly trading a VERIFIED record for a PARTIAL one somewhere else. Over all 7,500-odd object records, deduplicated to unique (module, symbol, addr):
base 729 named records -> head 731
ONLY IN HEAD (2):
ov047 data_ov047_02112334 0x2112334 VERIFIED emitted 12 romExtent 12
ov047 data_ov047_02112358 0x2112358 PARTIAL emitted 28 romExtent 36
ONLY IN BASE (0)
VERDICT CHANGED on a common key (0)
Zero regressions anywhere in the tree. The three records the class already owned keep byte-identical verdicts, they just change hands from the two flat destructor TUs to the intact one:
base: src/_ZN20daObjKm3_Kaitendai_cD0Ev.cpp + D1Ev.cpp
head: src/actors/daObjKm3_Kaitendai_c.cpp
_ZTI20daObjKm3_Kaitendai_c ov047 0x02112328 emitted 12 romExtent 12 VERIFIED
_ZTS20daObjKm3_Kaitendai_c ov047 0x02112340 emitted 23 romExtent 24 PARTIAL
_ZTV20daObjKm3_Kaitendai_c ov047 0x0211237c emitted 128 romExtent 128 VERIFIED blind 0
_ZTV16daObjKaitendai_c — the ov002 base-class table — stays with _ZN16daObjKaitendai_c8BehaviorEv.cpp in both. The promotion does not reach across and claim the parent's vtable. Good.
None of the 7 remaining DIFFERS records is Kaitendai-related; all 7 are present unchanged on the base.
The symbols.txt deletion is right, and it costs you a false PARTIAL
Your one config deletion:
-data_ov047_02112374 kind:data(any) addr:0x02112374
_ZTV20daObjKm3_Kaitendai_c kind:data(any) addr:0x0211237c
0x02112374 == 0x0211237c - 8. That is the ABI preamble — offset-to-top, then &_ZTI — and dropping the row is required, because the preamble is not its own object. It is the first 8 bytes of the vtable's emitted section:
liveSections: [14]=12 [24]=23 [25]=28 [27]=136
_ZTV size 0x88 = 136 storage, rebased +8 to the 128-byte address point
linkedStorageAliases: alias data_ov047_02112374 (8 bytes) donor _ZN20daObjKm3_Kaitendai_cD2Ev -> vtable 0x0211237c
A symbols.txt row is a delink boundary; there is no section for that row to bind to, so it has to go. The linked object still carries the 8-byte alias, and the storage aliases exact gate checks it. Nothing is lost from the link — only from dsd's config view.
But romdata_check reads the config view. rom_data_index() sizes every data symbol as distance-to-the-next-symbol:
nxt = next((entries[j][0] for j in range(i + 1, len(entries)) if entries[j][0] > addr), None)
if nxt is not None:
index[(label, name)] = (addr, nxt - addr)With the row present (your base) data_ov047_02112358 ends at 0x02112374, extent 28. With it gone (your head) it runs to the address point at 0x0211237c, extent 36. You emit 28 — which your own manifest states correctly, "size": "0x1c" — so:
base: extent 28, would have been VERIFIED
head: extent 36, scores PARTIAL
The record is byte-perfect. It is marked partial because the PR deleted the symbol that used to bound it, and it deleted that symbol because it had to.
This is not specific to you. Census over every symbols.txt:
origin/main 540 _ZTV symbols, 18 with a symbol at V-8, 522 without
#2111 head 539 _ZTV symbols, 17 with a symbol at V-8, 522 without
In my head build, 48 of the 256 PARTIAL verdicts are partial for no reason but romExtent - padded_emitted == 8 with a _ZTV next — _ZTV10BowserTail (124/132), _ZTV10StarMarker (124/132), _ZTV12EnemySpawner (124/132), _ZTS13daObjSwdoor_c, _ZTS10dCapIcon_c, and so on. Your own _ZTS16daObjKaitendai_c is one of them (19 string + 1 pad + 8 preamble = 28).
The fix belongs in the tool, one line — subtract OI.VTABLE_PREAMBLE when the next symbol is a _ZTV. check_symbol twenty lines further down already applies exactly that correction on the emitted side and to reloc addends; rom_data_index simply never applies it to the predecessor's extent. And the fix has a built-in positive control: it recomputes data_ov047_02112358 as 28, which is precisely the number your base's explicit V-8 row produced. The 18 owned tables are the control group for the other 522.
Do not add config rows for this. 522 neutral data_ovNNN_<V-8> entries would be 522 rows that every future intact-object promotion then has to delete again — which is the exact thing you correctly did here. I will open the tool fix separately; it is not yours and it is not a condition of merging.
The naming restraint is the right call, and it is the contrast with the rest of this branch
From your manifest:
the vtable is emitted at storage 0x02112374 and rebased to the repository's public address-point symbol 0x0211237c without inventing a second name for the ABI preamble
That is the whole difference between this commit and the two below it. #2096 (FlyGuy_VT7/VT14) and #2104 (RcCarpet_VT22/VT29) both coin names for cross-overlay phantoms, and both carry changes-requested from me for it. Here the same situation arose — a word that needs to stop being an independent symbol — and the answer was to drop the row and record the bias, not to name something the ROM does not name. Worth saying out loud because it is the pattern the campaign should copy.
Why I am not asking for a re-cut, unlike #2109
The branch is eleven commits:
bf06f1b02 Promote daObjKm3_Kaitendai_c intact translation unit <- this PR
b65a75427 Scope TU span checks to text sections <- #2109
ef612aca2 Update RcCarpet provenance source reference <- #2104
140d875c2 Promote daObjRcCarpet_c intact translation unit <- #2104 BLOCKED
589dddc74 Clarify grouped module field wording
6f2f9dbf3 Resolve RcCarpet fixture through its manifest
89e6c2b73 Apply manifest section ordering to all TU paths <- #2103
2828e68d7 Verify grouped relocation module sets exactly
186770859 Order intact TU data sections from manifests <- #2103
28587e4e5 Promote daPropeller_Heyho_c intact translation unit <- #2096 BLOCKED
ff9152800 Reconstruct daPropeller_Heyho_c translation unit <- #2096
On #2109 I asked for a cherry-pick onto main because that change was genuinely independent. This one is not, and I checked rather than assumed:
grep -c nontextSectionOrder tools/tubuild.py
origin/main 0
b65a75427003 (base) 8
introduced by: 186770859, 89e6c2b73 (both #2103)
Your manifest records a live, non-identity use of that machinery:
nontextSectionOrder: .data original [14,20,24,25,27] -> desired [20,14,24,25,27]
So the tool this TU is produced by does not exist on main. I want to be precise about what that does and does not prove: section 20 is dropped by the externalization policy, and the kept sections keep the same relative order either way ([14,24,25,27]), so the swap may well be byte-neutral for the owned band. I did not build a cherry-pick, so I am not claiming it would fail. What I am claiming is that a re-cut here is not the cheap, obviously-safe move it was on #2109, and proposing one without building it would be exactly the mistake I nearly made there.
The clean path is the ordinary one: land the chain bottom-up. The only thing blocking it is the coined names in #2096 and #2104, which are an author decision on those two PRs and have nothing to do with this commit.
Three reporting things, none blocking
1. criteria and objectAudit contradict each other, and it is a tool defect not yours.
criteria.declared_function_set_equals_defined_function_set:
"FAIL-BY-DESIGN -- 10 unlicensed section/symbol(s), 0 compiler-only policy error(s)"
linkcheck.objectAudit:
{"counts": {"LICENSED": 10}, "nonLicensedSymbols": [], "unlicensedSections": []}
Same ten items. They are your ten externalized_output inherited vague RTTI copies (_ZTI7fBase_c, _ZTS7dBase_c, _ZTI8dActor_c, …), and compiler_only_output is empty, matching the "0 policy errors" half.
The cause is that two phases write the same file. verify (tubuild.py:1421-1424) computes n_unlicensed from the text-only audit, before any intact-object policy has run. linkcheck (tubuild.py:4896) re-runs audit_tu_object with validated_vtable_policies and licenses all ten. Nothing re-runs the criteria string afterwards, so the manifest permanently records a failure about symbols it also records as licensed. Of the 29 promoted manifests on main, 26 say plain PASS — they simply had n_unlicensed == 0 at verify time.
There is a landed precedent for the fix and it is one line. _keep_richer preserves a curated string whenever it starts with the same verdict token and says more, so an authored value survives future verify runs. One manifest on main already does this:
"FAIL-BY-DESIGN -- 6 inherited RTTI/type-name copies are externalized to their canonical addresses"
Writing the equivalent sentence for your ten would make the manifest self-consistent and would stick. Optional — the real fix is in the generator, and I will take that up separately.
2. linkcheck.phases.checkSymbols: false. The description's "independent verification reproduced …" sentence does not mention that the symbol-check phase was off. This is an established pattern — 8 of 29 promoted manifests on main have it — and the manifest itself is not hiding anything, so this is a one-clause disclosure ask, not a defect. I reproduced what it covers: the 9 symbol errors are identical with and without your TU.
3. The declaration-order comment is true but not the whole mechanism.
/* Keep this definition before the class header. mwcc emits these dedicated
* data sections in declaration order, and retail places this descriptor
* before the class type name, SpawnInfo, and vtable storage. */That part holds — section 14 does land ahead of 24/25/27. What it omits is that nontextSectionOrder still had to swap 20 ahead of 14. A reader reproducing this TU from the comment alone would not know the manifest-driven ordering is in play at all. One sentence.
Gates
vs origin/tools/tu-span-text-sections @b65a75427003 merge tree 9b1cb4629c0c 8/8 pass -> pass
entries: 10782 -> 10779 (-3)
vs origin/main @5e88bfb5ed0f merge tree 6b2fd8fccbd6 8/8 pass -> pass
entries: 10814 -> 10779 (-35)
The -35 against main is -32 from #2104's RcCarpet consolidation plus your own -3; five one-function sources collapse into the intact TU. Consolidation, not loss — bytes are flat and converted-ratchet passes both ways. mergeStateStatus: CLEAN, PR validation SUCCESS.
Summary
Approve on content. Nothing in the diff needs to change.
- Built it: 11,088/11,088 functions, 5/5 data claims, 106/106 modules at 100.000000%, stock ROM sha256.
- Per-symbol data diff is a strict superset — two new ov047 records, zero verdict regressions tree-wide.
- The preamble drop is correct. The
PARTIALit produces ondata_ov047_02112358is aromdata_checkdefect affecting 48 records tree-wide (522 of 540_ZTVsymbols lack aV-8bound). Tool fix is mine, not a merge condition. - Declining to name the preamble word is the right call and the thing that distinguishes this commit from the two blocked ones underneath it.
- Optional prose: reconcile the
criteriastring withobjectAudit, mentioncheckSymbols: false, and note thatnontextSectionOrderis doing work the declaration-order comment does not mention.
This is the cleanest TU promotion I have reviewed in this campaign. It is held up entirely by two PRs below it that are not yours to fix.
|
Thanks for the independent reproduction and careful audit. I will keep the |
|
One correction to my broad wording on the #2109 supersession: I will not re-cut this PR directly onto main. Your review establishes that |
a56eadd
into
tools/tu-span-text-sections
1 participant
Replace five legacy one-symbol sources with one intact daObjKm3_Kaitendai_c translation unit. mwccarm now emits the retail D1/D0 pair from the typed inline destructor, with no homeless D2, and owns the class RTTI, type name, vtable, lifecycle methods, resource descriptor, SpawnInfo, and factory in one verified object. Independent verification reproduced 11,088/11,088 functions, 5/5 data claims, 106/106 modules, the 212-byte data band, all 40 relocations, and the stock ROM SHA. Remaining C-linkage declarations are exact ABI/data/factory/helper seams. Attribution path#symbol overrides preserve member credit.