Repository navigation
verify-baseline: resync the x86 decoder at symbol starts (fixes x64-musl lane on main) - #41549
Conversation
The x86-64 scan was one linear sweep over all of .text. JSC's LLInt puts a 4-byte opcode id in front of every llint_op_* label, so the sweep decoded garbage after each one, and what that garbage decoded as depended on the bytes that followed (a %rip-relative displacement, i.e. link layout). On main's current layout it came out as `tlbsync` inside llint_op_wide16 and failed the x64-musl lane; the allowlist already carried three llint_op_* entries added as earlier layouts tripped. Every symbol start is a real instruction boundary: an instruction that would straddle one is dropped and decoding restarts at the symbol. Same allowlisted hits and stale set as before on the failing binary, minus the false positive; the three llint_op_* blanket entries go.
|
Updated 4:05 AM PT - Sep 6th, 2026
@dylan-conway, your commit e084649 is building: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe x64 scanner now resynchronizes decoding at symbol boundaries. Documentation describes the behavior and updates related references. The JSC LLInt data-in-text allowlist entries were removed. Changesx64 scanner resynchronization
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The decoder behavior documentation and README references are updated consistently. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
scripts/verify-baseline-static/CLAUDE.md— nit: The README edit added three lines to the data-in-.textsection, and this PR updated theREADME.md:53-62reference on line 36 accordingly, but the sibling referenceREADME.md:61-74on line 47 was left unchanged — it now points into the middle of the rewritten paragraph instead of the MSVC jump-table / auto-filter block it describes. Fix: bump this reference by +3 toREADME.md:64-77so it matches the shifted content.Extended reasoning...
README.md previously had the MSVC-inlines-jump-tables paragraph starting at line 61; after the +3-line insertion in this PR that paragraph starts at line 64 and the filtered-automatically block ends at 74 (with the "Not filtered" note running to 77). CLAUDE.md line 36 was updated from 53-59 → 53-62 for the same shift, but line 47 was not touched, leaving a stale cross-reference. Purely a doc pointer drift; no runtime effect.
Verification: nit — doc cross-reference drift only, no runtime effect. The README edit inserted a net +3 lines into the data-in-
.textparagraph (base lines 55-59 → HEAD lines 55-62). On base, the MSVC jump-table paragraph started at README.md:61 and the block ran through line 74; on HEAD it starts at line 64 and the equivalent content runs through line 77. CLAUDE.md line 36 was updated in this same diff for…
There was a problem hiding this comment.
LGTM — clean structural fix that replaces layout-dependent allowlist entries with decoder resync at symbol boundaries.
What was reviewed:
- Resync logic in
scan_x86_64:startsis filtered to(sec_addr, sec_end), sorted/deduped;next_startcursor is monotone; resync target is always> ipand< sec_end, soset_positioncan't fail and the loop can't stall. - Straddle check runs before the
is_invalid()skip, so an INVALID decode that crosses a symbol still resyncs rather than drifting byte-by-byte. - Removed
llint_op_*allowlist entries are exactly the case the resync now handles (4-byte opcode id immediately preceding each label); README/CLAUDE.md line refs match the reflowed section.
Extended reasoning...
Overview
This PR improves the verify-baseline-static CI tool's x86-64 linear-sweep disassembler by resynchronising the iced decoder at every symbol start. Previously, data embedded in .text (notably JSC LLInt's 4-byte opcode IDs preceding each llint_op_* label) could desync the variable-length x86 decode and produce spurious post-baseline instruction hits whose exact shape shifted with link layout. The fix collects all symbol addresses strictly inside the section, and whenever a decoded instruction would straddle one, discards it and restarts decoding at that boundary. Three brittle llint_op_* allowlist entries (with a long explanatory comment) are removed as no longer needed, and README.md / CLAUDE.md are updated to describe the new bounded-desync behaviour.
Security risks
None. This is an offline CI verification tool that reads a locally-built binary's .text section and symbol table and prints a report. It touches no network, no user input, no runtime code paths, and no auth/crypto. The only new arithmetic is (resync - sec_addr) as usize, which is bounded by the a > sec_addr && a < sec_end filter, so set_position cannot be handed an out-of-range offset (and the .expect is unreachable in practice).
Level of scrutiny
Low-to-moderate. This is internal build tooling, not shipped runtime code. The core invariant — "a symbol start is always a real instruction boundary" — is sound for compiler- and offlineasm-emitted code, and even if it were violated the failure mode is a dropped instruction in a static checker, not a runtime fault. I traced the loop for forward progress: decode_out always advances ≥1 byte, next_start only increments, and any resync jumps strictly forward (resync > ip), so there is no stall or regression path. The straddle check is placed before the is_invalid() continue, which is the right ordering — an invalid decode that happens to span a symbol still triggers resync instead of creeping past it one byte at a time.
Other factors
The change is a textbook "fix the class, not the symptom" improvement per REVIEW.md: instead of accumulating llint_op_* allowlist entries as link layout shifts the desync between siblings, it removes the desync's ability to leak past a symbol boundary at all. Docs are updated consistently; the follow-up commit corrected the README line-range references in CLAUDE.md. No CODEOWNERS entry covers scripts/verify-baseline-static/. No outstanding CHANGES_REQUESTED reviews.
2 participants
What does this PR do?
Fixes the
x64-musl verify-baselinefailure on main (build 110907):llint_op_wide16 [INVLPGB] (1 insns)— a false positive, not a real post-baseline instruction.The static scanner decoded all of
.textin one linear sweep. JSC's LLInt emits a 4-byte.int <opcode id>in front of everyllint_op_*label (LLINT_EMBEDDED_OPCODE_ID, x86_64), so after each of those the sweep decoded a few bytes of garbage before falling back into sync, and what the garbage decoded as depended on the following bytes — here a%rip-relative displacement, which moves with link layout. That is why the PR that last touched this passed and the squash onto a newer main did not, and whyallowlist-x64.txthad accumulated threellint_op_*blanket entries "as they trip".Now the decoder restarts at every symbol start: an instruction that would straddle a symbol boundary is discarded and decoding resumes at the symbol. The three whack-a-mole entries are removed; README/CLAUDE.md describe the new behaviour.
How did you verify your code works?
Ran the old and new scanner on the exact failing artifact (
bun-linux-x64-musl-profile/bun-profilefrom build 110907): old → the CI failure; new → PASS, with the identical set of allowlisted symbols/instruction counts and the identical stale list otherwise (265 fewer garbage decodes overall).