Grow ELF string tables for partition aliases - #2126
Conversation
✅ PR validation — Passednoverify: no source/build-data changes in this PR Each changed |
c2f3dc5 to
ab3956d
Compare
andrewboudreau
left a comment
There was a problem hiding this comment.
Gated to the standard I published on #2103: because this changes tools/objisolate.py — the code every promoted TU on main links through — static gates are not enough, so I built the ROM twice.
The pair
python tools/rombuild.py -j16 --no-rom, same worktree, same invocation, same shared objcache. Baseline is origin/main @ 9005acd67; the target is the real merge commit 7ad601f9a (= main + this PR), not the branch tip.
main 9005acd67 |
main + #2126 7ad601f9a |
|
|---|---|---|
| compile | 10808 reused, 0 compiled | 10808 reused, 0 compiled |
| intact TU gates | dsd modules PASS, zero new symbol errors, storage aliases exact | identical |
| source-built functions | 11,088 | 11,088 |
| reproducing / mismatching | 11,088 / 0 | 11,088 / 0 |
| source-owned data claims | 3 (3 reproducing, 0 mismatching) | 3 (3 reproducing, 0 mismatching) |
| module fidelity | 106/106 exact, 100.000000% | 106/106 exact, 100.000000% |
| ROM-build analysis | PASS | PASS |
| ROM data | 529 verified, 191 partial, 6 differ, 522 unnamed (7,549 records) | 529 / 191 / 6 / 522 (7,549) |
Byte-identical on every reported metric. The compile step being fully cached is expected and is not a gap in coverage — rebias_object_symbols runs at link time via tu_production.py:64 and :357, which is uncached, and the manifest currently carries 30 promoted TUs (plus 3 link-verified), so the modified function was exercised thirty times per build in both runs.
Static: premerge_check.py 2126 on merge tree eea1820807d7, 8/8 pass->pass.
Why the risk is where the build actually looked
The in-place arm is untouched: len(new_name) <= len(donor_bytes) still takes byte-for-byte the pre-existing path, so nothing that works today can change behaviour. The only new arm is the one that used to return
"error": f"{label} {new_name} does not fit donor {donor_name}'s string-table slot"i.e. a case that previously produced no object at all. A new arm that only fires where the old code refused cannot regress a passing build by construction.
That leaves the three new unconditional guards in rebias_object_symbols as the sole theoretical regression surface, since those run on every TU including the in-place ones:
symtab.sh_entsize != 16- symtab
sh_linknot a valid STRTAB - any
st_shndx == SHN_XINDEX
These are exactly what a full build clears, and it did: 30 promoted TUs through the real path, zero refusals, storage aliases exact.
I also read the growth routine against its own claims. It refuses non-ELF32/non-ET_REL, program headers present, e_ehsize != 52, e_shentsize != 40, SHN_XINDEX shstrndx, a present SHT_SYMTAB_SHNDX, a bad or non-STRTAB link, non-byte or NUL-bearing names, a string table outside the file, missing boundary NULs, a malformed section-header table, insertion inside the section headers, non-power-of-two alignment, payload outside the file, overlap with the string table, a section growth would split, section/header overlap, and ELF32 limits — then re-parses and re-verifies section count, e_shoff, eight header keys, every offset and size, every non-NOBITS payload, and that the new string data is exactly old + addition. Fail-closed throughout, and the alignment is taken as the max sh_addralign over sections at or after the insertion point, so nothing downstream loses its alignment. The post-growth re-fetch of elf/symtab/string_table and the recomputation of every relocation_rewrites fileOffset as sh_offset + entryOffset + 8 matches main's RELA-addend convention at line 1243, and base = symtab.header["sh_offset"] is correctly read after the re-fetch.
Why this is worth landing rather than working around
This is the enabling half of a blocker I raised myself. On #2096 I asked for the coined FlyGuy_VT7/FlyGuy_VT14 aliases to be dropped in favour of the neutral exact config symbols — and the author did, to data_ov070_02123184 / data_ov070_021231a0. Those are 19 characters against 18-character donors, which is precisely the refusal this PR removes. Without it, the correct-naming fix is unimplementable and the pressure goes back onto inventing short names, which is the thing that produces coined names the ROM later contradicts.
One review note, non-blocking
The pre-existing suffix-overlap case (old line 789) changed from policy to long_alias, so the in-place path's string-table boundary refusal is no longer asserted anywhere — the new assertNotIn("string-table growth refused", ...) proves ordering instead, which is a different property. Worth restoring a small in-place boundary case at some point; not a reason to hold this.
The equivalence test itself is the right shape: it drives both paths on the same object and asserts identical relocations, identical symbol records except the two name offsets, identical section headers and payloads modulo the shift, and new_strings[:len(old_strings)] == old_strings.
Verdict
Clear to merge. Static 8/8, full ROM build byte-identical to main on every metric, the risky surface is exercised 30x per build, and it unblocks a naming correction I asked for. Merging now.
(For the record, per repo convention: I cannot press Approve — one shared account — so this comment is the sign-off.)
andrewboudreau
left a comment
There was a problem hiding this comment.
For the record on the rebase: the head moved from c2f3dc580 to ab3956d6a between my gate run and this comment, so I re-checked rather than assume it was a no-op.
git diff c2f3dc580 ab3956d6a -- tools/is empty —objisolate.pyandtest_objisolate.pyare byte-identical to what I read and built.- The whole old->new tree delta is the Kaitendai TU content, i.e. the base advancing over #2125, not new PR content.
origin/main..ab3956d6ais one commit touching exactly the twotools/files. git merge-tree --write-tree origin/main ab3956d6a->eea1820807d759e5c80ca6055e1645382f6207d1, which is bit-for-bit7ad601f9a^{tree}— the tree I actually compiled. Same tree, same result.
So the measurements in my previous comment apply unchanged to the current head. origin/main is still 9005acd67, all seven checks green. Merging.
Stacked on #2103 and intentionally separate from the FlyGuy source promotion.\n\nExtends objisolate's fail-closed vtable partition-symbol rewrite when a required neutral symbol name is longer than its exact dead donor's string slot. Fitting names keep the existing in-place path; longer names are appended to the linked ELF32 ET_REL .strtab with NUL padding that preserves later-section alignment. Only later sh_offset values and e_shoff move. Section/symbol indices, links, relocations, and non-string payloads remain unchanged and are reparsed/rechecked before success.\n\nAll existing donor provenance, uniqueness, dead/unreferenced state, boundary, shared-slot, and suffix-overlap checks remain mandatory. Nonstandard headers, program headers, extended indices, malformed/overlapping layouts, and bad string-table boundaries refuse.\n\nValidation at c2f3dc5:\n- independent wired objisolate/tu_production/tubuild suites: 119/119 passed\n- focused regression uses both real 19-character names data_ov070_02123184 and data_ov070_021231a0 with distinct safe 18-character donors\n- exact FlyGuy integration: 27/27 TU text contributions; full production 11,088/11,088 functions, 3/3 source-owned data claims, 106/106 modules, zero mismatches, exact stock ROM\n- strict control: 11,038/11,038, exact stock ROM, same nine pre-existing symbol diagnostics\n- port references 405/405; diff clean\n\nOnly tools/objisolate.py and tools/test_objisolate.py change. No source, claims, attempt-ledger, or dashboard artifacts are included.