Normalize partitioned C++ vtable symbols

[andrewboudreau]

Tools-only prerequisite for compiler-owned class TUs.\n\n- normalize undefined ABI vtable imports in partitioned and production paths\n- support manifest-proven interior vtable partition symbols using policy-owned dead ELF slots\n- bind split metadata to the stock-control ELF and preserve content/relocation bytes\n\nValidation: py_compile; 35 objisolate tests; 56 tubuild tests; 18 tu_production tests; independent verifier READY. No src/, docs dashboard, claims, or attempt-ledger changes.

[tangos-validator]

✅ PR validation — Passed

noverify: no source/build-data changes in this PR

_{Each changed src/*.c|*.cpp is compiled and its relocated bytes compared to the binary data on a private build box. Passing requires every changed file to reproduce the ROM byte-for-byte with correct relocation targets — this catches WRONG-DEST relocations and non-reproducing near-misses that ledger-scoped linkcheck skips.}

[andrewboudreau]

CHANGES REQUESTED (posted as a comment: this repo refuses a formal request-changes review from the account that opened the PR).

Requesting changes on the tests and the body, not on the code. I could not find anything wrong with the 712 lines of promotion machinery here — I tried hard to, and the result is a stronger endorsement than an approval would have been, so that part goes first. What I am blocking on is that the regression defense this change needs almost entirely does not execute in CI, and I measured that rather than inferring it: the wired suite goes 514 → 515. Five of your six new tests are inert on the runner, including all four that cover the guard you loosened.

Reviewed against the merge tree, not the branch: PR head e5169b6baa97 fast-forwards from origin/main@5c24b731940c, merge tree de3c12a63a49. 6 files, +878/−173.

The code is proven behaviour-neutral — by build, not by argument

I did not want to take "this is a refactor" on faith for a change that touches the intact-TU production path, so I ran the whole thing both ways.

tools/rombuild.py -j 16      base (origin/main)        merge (e5169b6baa97)
source-built functions       11,088 / 11,088 / 0       11,088 / 11,088 / 0
source-owned data claims     2 (reproducing 2)         2 (reproducing 2)
module fidelity              106/106 exact, 100.000000%   106/106 exact, 100.000000%
verified data bytes          35,372                    35,372
ROM data from source         465 / 252 / 7 / 522       465 / 252 / 7 / 522
intact TU gates              dsd modules PASS, zero new symbol errors, storage aliases exact  (both)
ROM-build analysis           PASS                      PASS

Normalizing only timings and cache-reuse counts, the two logs are identical line for line, 65 of 65. This is not a vacuous comparison: tu_production._prepare_one is the real production path, it runs in every build, and the two promoted TUs that still retain a _ZTV (ov047/daObjKm3_Kurumajiku_c, ov070/daBrq_c) drive your new normalize_undefined=True / normalized_undefined_vtables=True arguments on every one of those runs. The change is exercised and it moves nothing.

The wired unittest suite is green on the merge tree too — Ran 515 tests ... OK (skipped=3), exit 0.

Two reachability facts that should reassure you, and that belong in the body

The storage_alias donor widening has zero exposure on today's tree. policy_donor_ok accepts a four-way set:

compiler_only_output : deadstrip | deadstrip-duplicate | deadstrip-data
externalized_output  : canonical-import

The pre-existing #2019 guard accepted plain deadstrip only. The tree has exactly one storage_alias — ov047/daObjKm3_Kaitendai_c, donor _ZN20daObjKm3_Kaitendai_cD2Ev — and its disposition is plain deadstrip. So the three added dispositions are reachable by nothing that exists today. That is good news, and it is also precisely why it needs saying out loud: the next TU that aliases against a canonical-import RTTI row is the first one this widening decides, and by then nobody will remember it was widened here.

linkcheck --partitioned is reachable on 0 of 98 TUs. I enumerated the manifest rather than assuming:

98 entries    status: text-verified 65, promoted 29, link-verified 3, none 1
 4 retain a _ZTV in `data`
 3 carry a non-.text section claim  (the --partitioned precondition)
 0 actually accept --partitioned

All three refuse before reaching any of your changed code:

ov047/daObjKm3_Kaitendai_c    REFUSED: neighbour daObjKm3_Kurumajiku_c (0x021111a0..0x02112320)
                                       straddles the TU boundary
ov047/daObjKm3_Kurumajiku_c   REFUSED: path is already a delinks entry; .data claim
                                       0x02112258..0x02112320 overlaps its own entry
ov070/daBrq_c                 REFUSED: same, 0x02120570..0x021232f4
ov036/daObjRcCarpet_c         REFUSED: text-only TU, no non-.text claim

I mention this because I nearly published a false proof off it. Running linkcheck --partitioned on base and merge gives byte-identical output — but both refuse at step [1/8], so that identity proves unreachability, not equivalence. Anyone re-deriving this should not stop where I first did; the rombuild comparison above is the one that actually exercises the change.

The blocking ask: five of six new tests do not run in CI

tool-tests.yml runs python -m unittest over an explicitly enumerated list of 28 modules, deliberately not a glob. Where your tests landed:

file	new tests	in the CI list?	why not	effective in CI
test_tu_production.py	1	yes	—	runs
test_objisolate.py	1	no	workflow records it self-skips all 34 without the toolchain	inert
test_tubuild.py	4	no	pytest-style: 54 bare def test_*, zero unittest.TestCase	inert

Measured, not deduced — the same enumerated invocation on both trees:

origin/main        Ran 514 tests in 55.2s   OK (skipped=3)
merge e5169b6baa97 Ran 515 tests in 66.7s   OK (skipped=3)

+1. Exactly one of your six tests entered CI, and it is the one covering tu_production. The four in test_tubuild.py — 204 of your 373 new test lines, and the ones that cover policy_donor_ok, the interior-partition symbol synthesis, and the new split_symbols_ok link gate — contribute nothing. python -m unittest tools.test_tubuild prints Ran 0 tests ... OK and exits 0. pytest is not installed on any runner (header-offsets.yml:67, src-tu-refs.yml:66 both say so explicitly), so there is no second path that picks them up.

This matters because of the zero-exposure finding above, not despite it. You have loosened a guard whose blast radius is empty today and will not be empty later, and the compensating control you point at — policy_donor_ok re-checking caller-side — is exactly what those four dead tests assert.

The fix is cheap, and I checked that it is. I executed all four directly:

test_object_audit_licenses_only_validated_manifest_vtable_partitions      toolchain_gated=False  PASS
test_partitioned_vtable_interior_symbols_require_exact_policy_and_baseline toolchain_gated=False  PASS
test_intact_link_gate_requires_final_vtable_split_symbol_fidelity          toolchain_gated=False  PASS
test_partitioned_nontext_normalizes_imports_and_rebiases_retained_vtables  toolchain_gated=True   PASS

Three of the four are pure mock/dict/pure-function tests that need no compiler and no ROM. They are wirable today, unchanged in substance, by moving them into a module the list already names — test_tu_production.py is right there and already carries your fourth passing test — as unittest.TestCase methods. That is a mechanical edit, not a rewrite.

Sub-ask: the fourth one adds a 17th vacuous pass

test_partitioned_nontext_normalizes_imports_and_rebiases_retained_vtables opens with

if not _toolchain():
    return

taking test_tubuild.py from 16 such sites to 17. The workflow comment is unusually direct about this shape:

A bare return guard is strictly worse than a skip: pytest -rs cannot see it and neither can a reviewer reading a green log.

I have an open task on the existing 16; I am not asking you to fix those. I am asking that the new one not be a 17th. raise unittest.SkipTest(...) (or pytest.skip(...)) costs one line and reports honestly. Adding to a shape the tree has already written down as a defect is the part I would rather not wave through.

Second ask: the PR body

The body is a single line containing literal \n escape sequences, for a 712-line change to promotion machinery. Please rewrite it, and state the donor-disposition widening in it explicitly — deadstrip → {deadstrip, deadstrip-duplicate, deadstrip-data, canonical-import}, currently zero-exposure, motivated by the STB_LOPROC/STT_OBJECT RTTI rows. That widening is a live change to shipping behaviour and it is currently discoverable only by reading policy_donor_ok.

For the record, the donor gate itself moved from reject unless global function:

or donor["st_info"]["bind"] != "STB_GLOBAL" \
or donor["st_info"]["type"] != "STT_FUNC" \

to reject only if local:

or donor["st_info"]["bind"] == "STB_LOCAL" \

which is the right shape for accepting STB_LOPROC / STT_OBJECT RTTI donors, and the got check further down keeps its strict STB_GLOBAL + STT_OBJECT assertion (re-indented, not relaxed — I diffed it). No objection to either.

What I liked

Answering a loosened callee-side check with a stricter caller-side policy, instead of trying to encode manifest policy inside the ELF layer, is the right decomposition — objisolate.py has no business knowing what a canonical-import is. And linkcheck_pipeline_ready taking split_symbols_ok as a required conjunct rather than an advisory field is the fail-closed shape this tree keeps needing and not always getting.

Reproduction

git merge-base --is-ancestor origin/main e5169b6baa97   # fast-forward; merge tree de3c12a63a49
tools/rombuild.py -j 16                                 # base and merge, logs identical (65/65 normalized)
python -m unittest <the 28 modules tool-tests.yml names>  # 514 -> 515, OK, exit 0 both
python tools/tu_manifest.py list                        # 98 entries; 4 retain _ZTV; 3 non-.text; 0 accept --partitioned

premerge_check.py 2091 --fetch — all eight static gates pass -> pass:

gate                base     merge    verdict
converted-ratchet   pass     pass     ok
dead-references     pass     pass     ok
duplicate-sources   pass     pass     ok
header-offsets      pass     pass     ok
langmode-ratchet    pass     pass     ok
layout-check        pass     pass     ok
src-tu-refs         pass     pass     ok
source-coverage     pass     pass     ok

Nothing in the diff touches src/, include/ or config/, which is consistent with the byte-for-byte identical rombuild above.

[andrewboudreau]

CHANGES REQUESTED still stands — re-measured against the new head 0b0e31942531, not carried forward from the old one.

The head moved from e5169b6baa97 to 0b0e31942531 since my review. Nothing in that new commit addresses the wiring ask, and it makes the shape one step worse. Here is what actually changed:

file	tests added by this PR at e5169b6	at 0b0e319	in the tool-tests.yml list?
test_tu_production.py	1	1	yes
test_objisolate.py	1	1	no
test_tubuild.py	4	4 (+29 lines)	no

Re-derived the enumerated list off origin/main rather than trusting my earlier count: 28 modules, and neither tools.test_tubuild nor tools.test_objisolate is among them. test_tubuild is not an oversight — the workflow header says so in its own words:

test_tubuild_owned_relocs above is NOT a step toward wiring test_tubuild: it is a separate module precisely so [...]

and, of test_tubuild on a toolchain-free tree, "28 real pass / 15 vacuous / 2 fail." So there is no path by which those four tests run in CI, and none is coming.

The new commit loosens the guard whose tests are the inert ones

0b0e319 Accept linked module sections for vtable partitions drops section from the baseline comparison in validated_vtable_partition_symbols:

     metadata = (baseline.get("address"), baseline.get("size"),
                 baseline.get("binding"), baseline.get("type"),
-                baseline.get("visibility"), baseline.get("section"))
+                baseline.get("visibility"))
     if metadata != (address, size, "STB_GLOBAL", "STT_OBJECT",
-                    "STV_DEFAULT", policy.get("section")):
+                    "STV_DEFAULT"):

On the substance I have no objection. linked_symbol_rows reads section off the linked ELF, where the output section name is not the manifest's, so that equality was comparing two different vocabularies and would reject correct input. The sectionIndex conjunct immediately below survives and is the stronger tie — it binds the baseline row to the vtable's own section rather than to a name. The configured-side check against policy.get("section") is untouched. This looks right to me.

On the process it is exactly the problem. The one test covering it went into test_tubuild.py, so a live loosening of a validation path landed with a regression test that will never execute. That is the third guard in this PR in that position. The commit also carries an empty body, so the reasoning I just reconstructed by reading linked_symbol_rows is not written down anywhere a future reader would find it.

The ask has not changed, and it is still small

Move the three toolchain-free tests into test_tu_production.py as unittest.TestCase methods — that module is already in the list and already carries your fourth passing test. I executed all four directly last round and three need no compiler and no ROM:

test_object_audit_licenses_only_validated_manifest_vtable_partitions       toolchain_gated=False  PASS
test_partitioned_vtable_interior_symbols_require_exact_policy_and_baseline toolchain_gated=False  PASS
test_intact_link_gate_requires_final_vtable_split_symbol_fidelity          toolchain_gated=False  PASS
test_partitioned_nontext_normalizes_imports_and_rebiases_retained_vtables  toolchain_gated=True   PASS

The fourth needs the toolchain; raise unittest.SkipTest(...) instead of the bare return keeps it honest without wiring it. Whatever the new 29 lines assert about the section change should ride along.

Still holding: please rewrite the PR body (currently one line of literal escape sequences for a 712-line change) and state the donor-disposition widening in it — deadstrip to {deadstrip, deadstrip-duplicate, deadstrip-data, canonical-import}, zero-exposure today, motivated by the STB_LOPROC/STT_OBJECT RTTI rows. Add the section-name relaxation above to that same paragraph; it is the second live behaviour change in here that is discoverable only by reading the diff.

Standing findings from the previous round, unchanged at this head

The code is still proven behaviour-neutral by build — rombuild -j 16 logs identical line for line, 65 of 65 normalized, 11,088/11,088/0, 106/106 exact, 35,372 verified data bytes on both trees. premerge_check.py 2091 --fetch at this head: all eight static gates pass -> pass, merge tree 4bd687211c10. The storage_alias donor widening still has exactly one live alias to be reachable by (ov047/daObjKm3_Kaitendai_c, disposition plain deadstrip) and linkcheck --partitioned is still accepted by 0 of 98 TUs.

#2096 is stacked on this branch, so it inherits the block. That is the reason to spend the ten minutes on the move rather than another round here.

[andrewboudreau]

Merging. Lifting my own block, and writing down why — including a measurement that turned out to matter more for #2096 than for this PR.

The block is lifted, not satisfied

Two rounds of CHANGES REQUESTED, both asking for the same small thing, and it has not been done. I am lifting anyway rather than posting a third round, because the reason for the block was never the code, and because holding it is now costing more than it buys: #2096 is stacked on this branch and has been frozen behind a test-placement argument.

I will land the wiring myself, and I am treating it as a precondition on #2096 rather than on this PR — see the last section.

What I measured before merging

This PR cannot change a byte. Six files, all tools/, three of them tests. On the real merge tree (origin/main dedda0bbd3aa + 0b0e319, tree 9358106bbb1c):

mwccarm: 10812 enrolled source file(s)
  10812 reused from cache, 0 compiled      <- no compiled input differs from main's
source-built functions: 11,088
  reproducing: 11,088   mismatching: 0
module fidelity: 106/106 exact, 100.000000% of compared bytes
ROM-build analysis: PASS
ROM data from source: 465 verified, 253 partial, 6 differ, 522 unnamed

premerge_check.py 2091 --fetch: all eight static gates pass -> pass. All six PR checks SUCCESS.

And the new licensing path licenses nothing. I ran validated_vtable_partition_symbols over every manifest entry on the merge tree:

entries with non-empty rebias policies : 3 / 97
entries licensing >=1 partition symbol : 0 / 97
manifest partition_symbols rows        : 0

with a positive control on a synthetic entry that does fire, so the zero is a real negative. (My first control did not fire — I had the entry shape wrong; the fields are data/rodata/bss/init/ctor, not a nested ownedSymbols. Worth saying out loud: an unfired control would have let me report a false all-clear here.)

validated_vtable_partition_symbols, manifest_vtable_partition_rows, and audit_tu_object's validated_vtable_policies parameter do not exist on origin/main at all. This is entirely new machinery, and today it is dead weight — which is exactly what makes it safe to merge and exactly what makes it untested.

0b0e319 is a bugfix, and I can now prove it rather than argue it

Last round I said I had no objection to dropping section from the baseline comparison, reasoning from linked_symbol_rows. That reasoning was right, and here is the measurement behind it. The _ZTV symbols in the baseline linked ELF report these output section names:

_ZTV15daObjRcCarpet_c        section='OV036'
_ZTV20daObjKm3_Kaitendai_c   section='OV047'
_ZTV21daObjKm3_Kurumajiku_c  section='OV047'
_ZTV7daBrq_c                 section='OV070'

The manifest's section label is one of .rodata .init .ctor .data .bss. The two vocabularies never intersect. A/B, running the pre-0b0e319 function body side by side with this one:

baseline.section value  | PRE-0b0e319 (tight) | POST-0b0e319 (this PR)
  '.data'               | licensed            | licensed
  'OV070'               | REFUSED             | licensed
  'OV047'               | REFUSED             | licensed
  absent                | REFUSED             | licensed

'.data' is the only value that ever passed the tight form, and it is a value this pipeline never produces. So the tight form was not a stronger gate — it was an unconditional refusal, and the whole path was unreachable. 0b0e319 removes an impossible comparison. The real same-section invariant survives untouched and is enforced twice, by sectionIndex equality against the vtable's own section, in partition_vtable_rebiases (split_exact) and again in verify_linked_storage_aliases. Nothing was weakened.

Please put that in a commit message next time. The commit landed with an empty body, and this took me a linked-ELF dump and an A/B harness to reconstruct.

The thing that changes the stakes: #2096 is the first consumer

I went looking at #2096 while measuring this, and it introduces the first partition_symbols rows that have ever existed in the tree — FlyGuy_VT7 (0x02123184, 0x1c) and FlyGuy_VT14 (0x021231a0, 0x44) under _ZTV19daPropeller_Heyho_c.

So the picture is: this PR adds a fail-closed boundary that nothing exercises; the next PR in the stack is what first puts weight on it; and five of the six tests that describe its behaviour never execute in CI.

test_tu_production.py  1 test   module IS in tool-tests.yml   -> runs
test_objisolate.py     1 test   module NOT in the list        -> never runs
test_tubuild.py        4 tests  module runs, finds 0 tests    -> never run

(test_tubuild.py has bare def test_ functions and no TestCase, so python -m unittest tools.test_tubuild prints Ran 0 tests ... OK and exits 0 — the workflow's own comments say as much, and say test_tubuild_owned_relocs is deliberately not a step toward wiring it.)

That is a boundary going live with a decorative safety net, and it is the one real finding left in this stack.

What I am doing about it

I am landing the wiring myself, since I cannot push to this branch:

• move test_object_audit_licenses_only_validated_manifest_vtable_partitions (57 lines), test_partitioned_vtable_interior_symbols_require_exact_policy_and_baseline (117 lines) and test_intact_link_gate_requires_final_vtable_split_symbol_fidelity (8 lines) into test_tu_production.py as TestCase methods — I verified all three reference only tubuild.* and zero file-local helpers, so it is a mechanical move plus one import tubuild;
• convert test_partitioned_nontext_normalizes_imports_and_rebiases_retained_vtables's bare if not _toolchain(): return into raise unittest.SkipTest(...);
• wire tools.test_objisolate if it is green and TestCase-shaped on a toolchain-free tree;
• add a case pinning the sectionIndex invariant, so the check that 0b0e319 correctly left standing is the one holding the line, and is known to be holding it.

I am holding #2096 until that lands. Not as a penalty — as sequencing. The boundary should be under test before the first entry starts leaning on it, and that is a matter of days-not-weeks, on my branch, not yours.

Merged.