Add fail-closed compiler-owned C++ TU promotion - #2001
Conversation
✅ PR validation — PassedCommitted merge introduces no reconstruction or attribution regression. Full merge validation
Byte-verified means the range carries 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. |
a497b4e to
235fa01
Compare
732b017 to
56a2611
Compare
Review: KEEP AND FIXReviewed at Scope, first, because the raw diff is misleading
The parking note's premise is half right
The factual half checks out: The reasoning half is wrong. Main has three multi-function enrolled sources today, and one of them — Running this PR's The gate does pass — but it passes because of the new identity-upgrade branch at the top of One live regression: this defeats #2007's promoted-move rescue
Measured on #1993's tree ( Eight paths that main's ratchet correctly reads as moves become eight extra One latent cliffThe identity upgrade fires only when every member of the path is CONVERTED ( The strongest argument for this PR is not in its description
Under main's tool, a promoted entry that owns data reports its data as source-linked code and silently drops 284 bytes of real Relatedly, Also in favour of the scheme: One process assumption that is no longer trueThe "the validator restores To make this safe to land, in order
And rebase — the branch base is two commits stale. Read-only review: no worktree, no build. |
|
Gatekeeper follow-up. I independently verified the two claims that decide this PR's fate, and both hold. 1. The
|
| consumer | what it does |
|---|---|
tools/validate_merge.py:219 |
the PR validator itself |
tools/chaos_db_ci.py:62 |
progress DB |
tools/rombuild_check.py:184, rombuild_diag.py:131 |
106/106 and its diagnostics |
A promotion that gives one complete entry both a .text and a .data range arms it — and #2004 is exactly that shape (284 .text bytes dropped, 200 .data bytes counted as code). The gate that would catch the mistake is the gate that has the bug. That belongs in this PR, and it is the strongest argument in favour of merging it — stronger than anything currently in the description.
For reference, #2013's tools/source_coverage.py gets this right (cur.sections.append(...), every section kept) and documents why: .init is kind:code, 305 entries, 125,712 B that a .text-only tool silently drops.
2. The path#symbol regression is the blocker, and it has a deadline
The measurement from the review above matches my own reading of classify_missing: the move branch requires dest in current, and a promoted multi-member TU's plain path is never in current under the new scheme. Main rescues 8 moves on #1993's tree; this PR rescues 1.
Eight extra --update --reason runs means eight permanent fake backslide rows in config/converted-backslide-exceptions.jsonl — rows that record a regression that never happened, on a file whose union merge driver already duplicates entries. That is not a cost I want #1993 to pay.
Two ways out, and I do not mind which:
- Land the two-line widening in this PR (accept
f"{dest}#{sym}"membership in the move branch), and the ordering constraint disappears entirely. - Or leave it, and this PR lands strictly after [SUPERSEDED — reference only] TU: promote 27 reconstructed translation units (restack of #1880 -> #1882 -> #1884 -> #1914) #1993.
Merge-order call
#1993 is the last thing that touches config/converted-baseline.json. It regenerates that file wholesale and is the queue chokepoint. #2004's 1957-entry baseline must never merge ahead of it — a whole-file resolution there silently drops 608 banked entries, and because a shrunken baseline is permissive, no gate would ever say so.
So: #2002 → #2005 → #1995 → #1993 → (#2001 + #2004 as one unit). #2004 cannot land alone regardless, since main's rombuild_check.py would mis-measure its own delinks entry — which is bug 1 above, closing the loop.
Keeping this open. It is doing real work; it just needs the move-branch fix and a restack.
Two corrections to context you may have been given, since both were mine and both were wrong:
- The converted ratchet is a SET ratchet, not a count ratchet.
--checkfails only when a banked path leaves the set; the printed count is informational and there is no floor number to defend. A TU-promotion batch is expected to lower it. I circulated "the floor is 2565" to several agents and it was wrong in both the number and the concept. Correct two stale claims in tiers_ratchet.py's docstring #2014 corrects the docstring that said so. - The validator no longer restores
tools/from base — Validation now runs the pull request's own tools/ #1994 reversed that on 2026-08-30. A PR now exercises its own tool change. That matters most here, since this PR rewritesrombuild.pyandrombuild_check.py, the tools that produce its own 106/106. Reproduce the headline build numbers under main's tools before it merges.
56a2611 to
6a884b2
Compare
6a884b2 to
ef16a30
Compare
|
Gatekeeper review. Keep and fix — the core control is sound and I want it landed, but two The design is right. Requiring the production ROM to be bit-identical to a control ROM Blocking1. Two supported invocations use intact objects with no ROM comparison at all. if intact_link_verification:
comparison = intact_rom_comparison(rom_sha256, intact_link_verification)
if not comparison["identical"]: raise BuildError(...)
This is the failure this whole queue exists to catch: an intact range silently refilled from 2. if baseline:
return TrueMain used Recommended3. 4. Add the elif disposition == "deadstrip" and symbol[:4] in ("_ZTI", "_ZTS", "_ZTV"):
errors.append(f"{source}: {symbol} is a vtable/typeinfo record banked as a plain "
f"deadstrip, which is never compared against the cartridge. Drop the "
f"row, or rename the class to the cartridge's RTTI spelling so it "
f"resolves as deadstrip-data")
5. Split the 6. Confirmed good
Landing order#2004 is purely dependent (
|
Gatekeeper re-review — still blocked, but for a new reason, and it is not visible on this PRHead has not moved since my 16:33 review ( The blocker: merging this into current main turns the converted-ratchet gate redBuilt read-only with The four are
Root cause. This also refutes the "per-member scores are monotonically >= the file-wide score" claim in the 15:43 follow-up: Preferred fix. The mapping already exists in the manifest: # instead of: dest_member_ids.issubset(current)
sym = legacy_symbol_for(rel, dest) # from the manifest's legacy_source
if dest in current or (sym and f"{dest}#{sym}" in current):All four banked paths then rescue, and a member that genuinely regresses still fails. Please do not take the fallback ( Why the green on this PR is not evidence
What I verified is clean — no erosion anywhereI recomputed the enrolled-range union directly from every
Identical, and matching the validator's absolute row on both PRs. Also confirmed: this PR touches no Note also that #2019 landed the Still open from 16:33
Test wiring, for the record
VerdictRestack, do not re-cut. The infrastructure is sound and the earlier objections are closed. Order:
Stays a draft until 1-5 land. #2004's content is clean and needs no changes; its landing sequence is unchanged and I have posted it there. |
The move-rescue fix, written and proven — apply this verbatim if you wantThe branch has been static for an hour and this is the only thing standing between it and a merge, so rather than leave you to derive it from my last comment I built and tested it. Nothing was pushed to your branch — this is a patch for you to take or reject. Reproduction, against current mainThe branch is now 43 behind. Merge result built read-only ( The manifest makes the defect plain —
The fixRescue a moved path against its own member identity. Three hunks, additive — it can only ever rescue more, never fewer, and a member that genuinely regresses still fails because its own identity drops out of --- a/tools/tiers_ratchet.py
+++ b/tools/tiers_ratchet.py
@@ -203,6 +203,33 @@
return moves
+def promoted_move_symbols(root=None):
+ """`legacy_source` -> the symbol that path defined, for every PROMOTED TU entry.
+
+ `promoted_moves` answers WHERE a banked path went; this answers WHICH function it
+ took with it. The rescue in `partition_missing` needs the second one: a promotion
+ is lossless for a given banked path when THAT path's own code is still CONVERTED
+ at its new address, not when every unrelated member of the destination is. The two
+ are different questions and only the manifest can tell them apart.
+ """
+ out = {}
+ try:
+ data = tu_manifest.load(root)
+ except (OSError, ValueError):
+ return out
+ for entry in data.get("entries") or []:
+ if entry.get("status") != "promoted":
+ continue
+ dest = entry.get("promoted_source")
+ if not dest:
+ continue
+ for fn in entry.get("functions") or []:
+ legacy, symbol = fn.get("legacy_source"), fn.get("symbol")
+ if legacy and symbol and legacy != dest:
+ out[legacy] = symbol
+ return out
+
+
def source_text(rel):
@@ -389,6 +416,7 @@
if ownership is None:
ownership = tiers.srcpath.source_definition_index()
+ move_symbols = promoted_move_symbols()
absorbed_clean, backslid = [], []
@@ -407,7 +435,9 @@
} if len(dest_members) > 1 else set()
+ own_symbol = move_symbols.get(rel)
if (dest in current
+ or (own_symbol and f"{dest}#{own_symbol}" in current)
or (dest_member_ids and dest_member_ids.issubset(current))):
absorbed_clean.append(rel)
continueProof it works, on the merge resultNo exception rows. No Two things to add on top
Where that leaves the PRItems 1-5 from my 17:25 review still stand; this closes item 2 and hands you the code. #2019's Verified read-only throughout — nothing was pushed, and the exported trees were built with |
Correction and an addition to the fix I postedTwo things, one of them my error. 1. The function is --- a/tools/tiers_ratchet.py
+++ b/tools/tiers_ratchet.py
@@ -372,6 +372,26 @@
return "; ".join(tiers.CRITERION_LABEL[k] for k in failed)
+
+def promoted_move_symbols(root=None):
+ """`legacy_source` -> the symbol that path defined, for every PROMOTED TU entry."""
+ out = {}
+ try:
+ data = tu_manifest.load(root)
+ except (OSError, ValueError):
+ return out
+ for entry in data.get("entries") or []:
+ if entry.get("status") != "promoted":
+ continue
+ dest = entry.get("promoted_source")
+ if not dest:
+ continue
+ for fn in entry.get("functions") or []:
+ legacy, symbol = fn.get("legacy_source"), fn.get("symbol")
+ if legacy and symbol and legacy != dest:
+ out[legacy] = symbol
+ return out
+
def classify_missing(missing, current, tracked, moves, ownership=None):
"""Split banked identities into clean ownership transitions and backslides.
@@ -389,6 +409,7 @@
"""
if ownership is None:
ownership = tiers.srcpath.source_definition_index()
+ move_symbols = promoted_move_symbols()
absorbed_clean, backslid = [], []
for rel in missing:
members = ownership.get(rel) or []
@@ -407,7 +428,9 @@
dest_member_ids = {
f"{dest}#{symbol}" for symbol in dest_members
} if len(dest_members) > 1 else set()
+ own_symbol = move_symbols.get(rel)
if (dest in current
+ or (own_symbol and f"{dest}#{own_symbol}" in current)
or (dest_member_ids and dest_member_ids.issubset(current))):
absorbed_clean.append(rel)
continue2. The fix's validity depends on #2023 dropping an unrelated baseline swap. #2023 Composed three ways, measured, not predicted:
So: the patch is right, and it is sufficient provided #2023 lands without that swap. I Nothing else in my 17:25 review changes. Items 1 and 3-6 are still open. |
Blocking: this PR is green everywhere and red on the tree that doesn't exist yetHead Nothing in CI would have caught this. Why it fires
This PR changes So this is not a bug in your patch. It is your patch doing its job on a file that landed after you branched. The disposition is yours, and I'd take the firstOption A — name Option B — add a Option C — soften the per-member check. I'd argue against it. The check is correct; weakening it to get this PR green trades away the thing the PR is for. Whichever you pick, merge Two other things
#2004 does not actually merge. I am holding both as drafts until #2001's merge result is green. Ping me when main is merged in and a disposition is picked and I will re-gate. |
|
| member | real_name |
|---|---|
dBase_c::~dBase_c (D1, D0) |
PASS |
func_02013edc |
FAIL |
dBase_c::Spawn |
PASS |
dBase_c::AfterInitResources |
PASS |
One failure, one criterion, no hidden second offender — the other four criteria are
file-scoped and already pass. Re-scored after the change: 5/5 on every member.
Files that must move in the SAME commit
include/dBase_c.h(declaration; the comment block also spells the raw address)src/actors/ActorDerived.cpp:28src/_ZN8dScene_c16SpawnIfNecessaryEv.cpp:14,23src_tu/scene/Scene.cpp:761,770— the pre-push hook compilessrc_tufor every
branch, so if this lags, every push on every branch is refusedconfig/arm9/symbols.txt:463config/tu_manifest.d/arm9/ActorDerived.json:42plus the prose at:12/:104that
currently asserts the veneer is unnamedattribution.json:755— re-key…ActorDerived.cpp#func_02013edc→
…#_ZN7dBase_c9SpawnRootEjji, or that member's credit is silently dropped
config/arm9/delinks.txt needs no change — the file is delinked by address range
(0x02013e80–0x02013f28), not by symbol.
Merge-order note
This PR no longer merges into current main (2eb1d87fe) — real conflicts in
attribution.json and config/converted-baseline.json from the TU-promotion lane that
landed ahead of it. Resolve those as a three-way merge, not a key union: a
promotion branch deletes keys, and union resurrects rows for files that no longer
exist. Nothing goes red when you get that wrong, so check it by hand — every key naming
a src/** path must resolve to a file that exists in the merge result.
1 participant
Adds the tools/metadata prerequisite for promoting a proven C++ translation unit with compiler-emitted lifecycle metadata and owned non-text sections.
The intact-TU admission is fail-closed:
Control evidence remains content-bound to the demoted scratch config, extracted ROM input tree, linked ELF, DSD, linker, and judging Python tools. Reports retain the current control ROM, historical bootstrap ROM, and deterministic SHA-256 over all 106 executable-module images.
The review follow-up is included:
Also restores the known legacy credits for Kurumajiku Init/Cleanup (lunavyqo) and RickshawBs_Spawn (tangosdev), so the dependent source promotion transfers them unchanged.
Verified after rebasing onto current main: 244 focused TU/ROM tests passed with 1 environment skip; 89/89 manifest TUs compile; 405/405 port references resolve; the CONVERTED ratchet passes; no new dead references. The dependent Kurumajiku integration proof is 11,087/11,087 reproducing, 106/106 exact modules, zero new symbol errors, and a production ROM bit-identical to its admitted independent control.