Scope TU span checks to text sections

[andrewboudreau]

Fix intact-TU span selection so neighboring data sections cannot make an adjacent text entry appear to straddle the candidate span. Selected entries still carry all of their sections, so a candidate entry with non-text content continues to fail closed. Adds focused regressions; the full intact-TU test suite passes (121 tests).

[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]

Reviewed at b65a75427003. The change is correct and I could not find a way to make it license anything it should not. I have one ask, and it is not about the code — it is about how this lands, because the branch carries more than the PR shows.

The change is a bug fix, and I checked it the hard way

The concern with any edit to span_entries is that narrowing what the envelope is computed from is indistinguishable, at a glance, from narrowing what gets refused. It isn't, here, and the reason is one line you left alone:

inside.append((idx, rel, secs))     # still ALL sections, not text_secs

Selection narrowed to .text; rejection still sees everything. The downstream fail-closed arm is byte-identical:

for name, rel in [(n, r) for _s1, _s2, r, n in tiles if n != ".text"]:
    reasons.append(f"entry {rel} declares a non-.text section {name}; ...")

Positive control. Your new test against the old tubuild.py:

E  AssertionError: ['entry src/neighbor.cpp (0x00000f00..0x00002010) straddles the TU boundary']

That is the exact false verdict the change is for — a neighbour whose .text ends at 0x1000, entirely outside the span, dragged across the boundary by .data at 0x2000. The test fails on the base and passes on the head, so it asserts the fix rather than restating it. Full suite in an extracted tree, base vs head:

BASE  4 failed, 60 passed
HEAD  3 failed, 61 passed

The delta is exactly the new case. The 3 constant failures need src/, which I did not extract; they are identical either side of the change.

The one path where it could have relaxed, measured

if not secs → if not text_secs means an entry with only non-text sections sitting inside the span is now skipped entirely rather than classified. Under the old code it landed in inside and tripped the non-.text refusal. So I checked whether anything still stops it:

src/one.c  .text 0x1000..0x1010
src/mid.c  .data 0x1010..0x1018     <- data-only, inside the span
src/two.c  .text 0x1018..0x1020

reasons: ['range gap/overlap at 0x00001010: next entry src/two.c starts at 0x00001018']

and if the manifest names it:

reasons: [... gap/overlap ...,
          'manifest names legacy source src/mid.c, which is not a delinks entry inside the span']

Two independent refusals. The skipped entry still occupies address space, so the tiling walk cannot close over it. Fail-closed survives. I could not construct an input that the old code refused and the new code licenses.

This is the same shape as the 0b0e319 question I settled earlier in this stack's history: the tight form was refusing real, correct input, and the fix restores the intended boundary rather than moving it.

The ask: this needs a re-cut, not a retarget

tools/tubuild.py and tools/test_tubuild.py are the only files your commit touches, and the change has nothing to do with daObjRcCarpet_c. My first instinct was to ask you to retarget the PR to main so it could land now instead of queueing behind a base I have blocked. That would have been wrong, and I want to be explicit about why, because it is the failure mode this repo punishes.

The PR's diff is 2 files. The branch is ten commits:

b65a75427  Scope TU span checks to text sections     <- the PR
ef612aca2  Update RcCarpet provenance source reference
140d875c2  Promote daObjRcCarpet_c intact translation unit
589dddc74  Clarify grouped module field wording
6f2f9dbf3  Resolve RcCarpet fixture through its manifest
89e6c2b73  Apply manifest section ordering to all TU paths
2828e68d7  Verify grouped relocation module sets exactly
186770859  Order intact TU data sections from manifests
28587e4e5  Promote daPropeller_Heyho_c intact translation unit
ff9152800  Reconstruct daPropeller_Heyho_c translation unit

Retargeting the base only changes which diff GitHub renders. The merge would still take all ten — including #2096 and #2104, both of which carry changes-requested from me over coined names for cross-overlay phantoms. Premerge against main shows the giveaway plainly: entries: 10814 -> 10782 (-32), which a two-file tools change cannot produce.

So: cherry-pick b65a75427 onto main as a fresh single-commit branch and open it as its own PR. I verified that works:

git merge-tree --write-tree --merge-base b65a75427^ origin/main b65a75427
-> 190b5d36a667   (exit 0, no conflicts)

tubuild.py differs by 224 lines between main and your base, so a clean textual apply is not by itself proof. I extracted that resulting tree and re-ran: the fail-closed arm is present (tubuild.py:1624), and your new test passes on it. The fix is genuinely independent of the stack, not just textually separable.

That would let this land immediately. As it stands it is behind four PRs, two of which are blocked on an author decision that has nothing to do with it.

Disclosure: this test runs in no CI job

I would rather you hear this from me than assume the green tick covers it. tool-tests.yml runs tools.test_tubuild_owned_relocs and tools.test_tubuild_vtable_partitions — not test_tubuild, which is deliberately excluded because its fixtures need the pinned compiler. src-tu-refs.yml mentions pytest tools/test_tubuild.py only inside a comment explaining why it isn't run. I scanned every workflow file to be sure.

So the case you added is a good one and it does assert the fix — but nothing in CI will notice if it later goes red. That is not an objection to this PR and not something to fix in it; it is context for how much the passing tools check is worth here. I ran it both directions locally, which is the coverage this actually has today.

Gates

vs origin/main @5e88bfb5ed0f   merge tree 7dce0707359a   8/8 pass -> pass

with the caveat above — that run merges the whole ten-commit branch, so its -32 entries line belongs to #2104, not to you. All six checks green, PR validation SUCCESS.

Summary

Content: approve. It fixes a real false refusal, it is positive-controlled, and the fail-closed behaviour is intact under the one input shape that could have slipped. My open task list has an entry for tubuild.py verify false-alarming PROMOTION REFUSED on intact-object TUs, and this looks like it may be that bug; if you have a real TU that reproduced it, saying so in the description would make the case stronger than the synthetic fixture does.

The only thing I want changed is where it lands. Re-cut it onto main and I have nothing else.

[andrewboudreau]

Agreed. Retargeting this PR would preserve the unrelated ancestry, so I am re-cutting only b65a75427 as a fresh single-commit branch from current origin/main. I will open the replacement and close this PR as superseded after the focused tools gates pass.

[andrewboudreau]

Superseded by #2121, which contains only the approved tools commit re-cut directly on current main as requested. The descendant class-TU stack will be rebased after the replacement lands so it does not retain this obsolete ancestry.