Yarr: lookbehind JIT (incl. unicode), alternation factoring, first-character dispatch, and Boyer-Moore search selection#299
Conversation
Patterns containing a lookbehind previously fell back entirely to the bytecode interpreter (JITFailureReason::Lookbehind at compile time, and a pre-check in RegExp::compile that never even attempted the JIT). A single lookbehind therefore sent an otherwise cheap pattern to the interpreter; the isbot user-agent regex (~110 alternatives, four of them lookbehinds) ran ~40x slower than with the assertions JIT'd. A lookbehind body matches right-to-left ending at the assertion point. Matching backward over the input is equivalent to matching a term-reversed copy of the body forward over the reversed prefix, so each body is compiled as a mirrored copy (terms reversed, input positions reassigned with the same width rules as setupAlternativeOffsets) driven through the existing forward machinery. The original YarrPattern is untouched, keeping the bytecode fallback valid. Each op carries a MatchDirection; in a backward body the index register is the leftward-moving left frontier, and only the primitives flip: input claims subtract (branchSub32 on the borrow's sign bit), characters are read at input[index + (k - 1)], end-of-input is index == 0, greedy consumption and give-back reverse sign, captures write the real position with start/end slots swapped (backward captures record end-first, matching the interpreter), and BOL/EOL/word-boundary anchors get mirrored edge tests. Nested lookbehinds are mirrored on demand. Still handled by the interpreter, exactly as before: surrogate-pair decoding (unicode 16-bit), backreferences inside a body, lookaheads nested inside a body, and quantified groups with maxCount > 1 inside a body. Character fusion (multi-char wide loads) is disabled inside backward bodies since ascending memory holds descending pattern positions. collectBoyerMooreInfoFromTerm now treats a parenthetical assertion as zero-width (like BOL/EOL/word-boundary) instead of aborting collection: an assertion consumes no input, so the leading-character sets remain a valid over-approximation.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change enables supported lookbehind patterns in the Yarr RegExp JIT through mirrored backward execution, adds direction-aware matching and dispatch infrastructure, updates alternation optimization, and introduces stress coverage comparing JIT and interpreter results. ChangesLookbehind JIT support
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JSTests/stress/regexp-lookbehind-jit.js`:
- Around line 7-12: Update shouldBe to compare expected and actual values
recursively using Object.is, including each array element, so undefined captures
remain distinct from null. Preserve the existing mismatch error reporting and
message behavior while replacing the JSON.stringify-based equality check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5f1f314-6f67-4953-aba9-cd18c8087599
📒 Files selected for processing (3)
JSTests/stress/regexp-lookbehind-jit.jsSource/JavaScriptCore/runtime/RegExp.cppSource/JavaScriptCore/yarr/YarrJIT.cpp
💤 Files with no reviewable changes (1)
- Source/JavaScriptCore/runtime/RegExp.cpp
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a substantial JIT codegen change — new backward-matching primitives, mirrored-disjunction construction, and direction-aware addressing across many term types — so it warrants a human reviewer familiar with Yarr.
What was reviewed:
- Backward input-claim/read/end-of-input primitives and the
negativePositionOffset→index + (k-1)addressing transform. - Capture start/end slot swapping in backward frames vs. the interpreter's end-first recording.
mirrorDisjunctionForLookbehindfallback paths (backrefs, nested lookaheads, quantified groups >1, surrogate decoding) — each still sets aJITFailureReasonso the interpreter path remains reachable.- The Boyer-Moore change treating parenthetical assertions as zero-width — checked that ignoring the assertion keeps the leading-char sets an over-approximation.
Extended reasoning...
Overview
This PR adds lookbehind-assertion support to the Yarr regex JIT. Previously, any pattern containing a lookbehind was rejected wholesale (both by a pre-check in RegExp::compile/compileMatchOnly and by JITFailureReason::Lookbehind) and fell back to the bytecode interpreter. The change compiles each lookbehind body as a term-reversed mirrored copy driven through the existing forward machinery, with a per-op MatchDirection that flips the input primitives (claim, read, end-of-input, consume/rewind, capture slot order, and BOL/EOL/word-boundary edge tests). It touches ~500 lines in YarrJIT.cpp, removes two guard lines in RegExp.cpp, and adds a new stress test.
Security risks
Regex JIT is security-sensitive: it emits machine code that indexes into string buffers. The new backward addressing (input[index + (k-1)], branchSub32 on the sign bit for input claims) must never read outside [input, input+length). The PR's argument that index and claim counts are both < 2^31 (so a borrowing sub sets the sign bit) looks sound given containsUnsignedLengthPattern() gates the JIT, and the k==0 peek case (input[index-1]) is only reached by anchors that first check index != 0. I did not find an OOB read path, but this is exactly the kind of invariant a Yarr expert should sanity-check across all the term-type call sites.
Level of scrutiny
High. This is new codegen logic in a JIT compiler with a novel coordinate transform (mirrored/virtual input positions) threaded through many existing generate/backtrack functions. The correctness surface is large — every direction-branched primitive (fixed/greedy/non-greedy pattern-char and char-class, anchors, word boundaries, nested-alternative check-adjust, parenthetical-assertion begin/end) has to agree on what index and inputPosition mean in a backward frame. The differential fuzzing (6000 patterns × 25 inputs × all flags, byte-identical vs. interpreter) and test262 lookBehind suite passing in both modes are strong evidence, but not a substitute for a design-level read by someone who owns this code.
Other factors
- The bug-hunting system found no issues.
- The mirrored-copy approach deliberately leaves the original
YarrPatternuntouched, so unsupported bodies (backrefs, nested lookaheads,maxCount > 1groups, surrogate decoding) cleanly setm_failureReasonand fall through to the pre-existing interpreter path — I verified each of those bailout branches is reachable and sets a reason before returning. - The Boyer-Moore change (
return cursorforParentheticalAssertion) is independently reasonable: assertions are zero-width, so skipping them keeps the leading-character sets a valid over-approximation. - The PR description explicitly flags follow-up refactors (moving mirroring into
YarrPatternConstructor, reversed packed constants) as out of scope, which is a design decision worth a maintainer's nod.
Given the scope and the codegen/security surface, deferring to a human reviewer.
…group A large top-level alternation is matched by attempting each alternative in turn at every candidate position, so cost grows linearly with the alternative count regardless of how quickly each alternative fails. Two pattern-level rewrites cut the number of alternatives entered. Alternation prefix factoring: alternatives are tried leftmost-first, but their order is only observable between alternatives that can match at the same starting position. Alternatives that must begin with different literal characters have disjoint starting points, so a maximal run of consecutive alternatives that each begin with a fixed, case-sensitive literal character is stably sorted by that character (stability keeps same-first-character alternatives in source order) and each group sharing its leading literal is merged into one alternative whose common prefix is hoisted over a non-capturing group of the suffixes: /aq|bx|ar|by/ becomes /a(?:q|r)|b(?:x|y)/. Any alternative that does not begin with such a character (class, group, anchor, optional atom, the empty alternative) is a barrier no reordering crosses, and the rewrite recurses into the factored suffixes. Group folding: the body's repeated (non-once-through) alternatives, when numerous, are folded into a single alternative holding one non-capturing group -- /X|Y|Z/ is exactly /(?:X|Y|Z)/ -- whose capture range brackets the captures it absorbs. Nested alternatives claim and release their own input, giving each alternative an index-neutral entry that a following change dispatches on the first character. Both rewrites are engine-neutral pattern equivalences (the interpreter and the JIT see the same factored form) and run before setupOffsets(), which lays the synthesized groups out like hand-written ones. Controlled by useRegExpAlternationFactoring / regExpAlternationGroupThreshold.
Preview Builds
|
Trying each alternative of a nested disjunction in sequence costs an entry attempt (claim, character load, compare, release, jump) per alternative at every candidate position, so a large alternation scales linearly with its alternative count even when each alternative fails on its first character. For a once-quantified fixed group whose alternatives all consume at least one character, compute per alternative a first-character set (an over-approximation of the characters a match can start with; Latin-1 tracked precisely, wider characters and the empty prefix widen to "any"). Every alternative's first character sits at the group's start frame position, which is inside input the enclosing alternative already claimed, so it is read once with no bounds check. A binary decision tree over that character's value then jumps to the ordered chain of exactly the alternatives whose set contains it; an "any" alternative appears in every chain, so leftmost-alternative-wins ordering is preserved. When no alternative can start with the character the group fails immediately. Each chain is a sequence of stubs that store the address of the next stub into a new frame slot (BackTrackInfoParenthesesOnce::chainResume, patched at link time) before jumping into the alternative; a failing alternative releases its claim and continues its own chain through that slot rather than falling to a fixed successor, and an exhausted chain joins the group's failure flow. Entry into an alternative is now a labelled chain target rather than a fallthrough, and backtracking back into a matched alternative reuses the still-valid resume address. Combined with the alternation prefix factoring, the isbot user-agent regex over the 361,829-string corpus goes from ~620 ms (factoring alone) to ~120 ms; the sequential JIT was ~750 ms and the interpreter ~32,000 ms. Controlled by useRegExpAlternationDispatch / regExpAlternationDispatchThreshold; interpreter behaviour is unchanged and JIT results match it byte-for-byte across differential corpora.
The prefix-factoring pass previously ran only over the body's top-level alternatives, so a hand-written group such as /\b(?:about|above|after)\b/ kept re-comparing its shared "a" prefix in every alternative. Recurse into the disjunctions of source-written (non-copy, forward) groups after factoring each level, so nested alternations are factored the same way. Groups synthesized by the pass were already factored when built; revisiting them is a no-op.
Fixes found in review of the factoring/folding passes:
- Do not restructure a nested disjunction whose group was already
committed to a specialized code shape by checkForTerminalParentheses.
Factoring an isStringList group turned /^(?:xa|xb|y)/ into
[x(?:a|b), y]; the JIT's string-list path treats a non-last
alternative's final character as a full match, and the merged
alternative's "final character" is only the shared prefix, so
/^(?:GET|POST|PUT|SEND)/.test("Pzz") wrongly returned true. Terminal
groups are skipped for the same reason.
- Clear parentheses.isTerminal on every alternative absorbed into a
synthesized group (prefix suffixes and folded body alternatives). A
terminal parenthesis is only valid as the last term of a body
alternative; once nested, TerminalEnd's empty-backtrack-state
assertion no longer holds. The general once/greedy path is used.
- Compute a synthesized group's capture span from the terms it contains
rather than per-alternative bookkeeping: sorting reorders which
alternative is first, and the parser leaves the last body
alternative's m_lastSubpatternId unset, so the old span could exclude
captures the group holds (or invert subpatternId/lastSubpatternId).
- Leave small alternations unfactored (runs below the group
threshold). The sequential JIT path there already has fused compares,
the two-alternative SIMD scan, and frame-free inlinable code, which
factoring would forfeit for no gain; measured /agggtaaa|acccttta/
and /on|off/ regressions are gone.
dylan-conway
changed the title
A dispatched alternative that is a fixed-length literal string (only fixed-count pattern characters) can never be backtracked into: it matches its exact characters or it does not. Emit such alternatives as inline compare blocks inside the chain instead of entering them through the frame-slot resume mechanism: claim the alternative's extra input, compare its characters at their frame offsets, and on mismatch release the claim and fall statically to the next chain element. On a full match the block joins the group's success path after the End op's own return-address store, having stored a return address pointing at an unwind thunk (release the claim, continue the chain) so that a later failure can still backtrack into the matched literal and try the following alternatives. This removes the per-alternative frame store, frame load, and indirect jump for literal alternatives; complex alternatives keep the existing resume mechanism at chain boundaries.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/yarr/YarrJIT.cpp`:
- Around line 5198-5229: Make the dispatch backtrack handling in the
op.m_dispatch branch explicitly enforce the Forward-direction invariant before
adjusting m_regs.index, or replace the unconditional sub32 with the
direction-aware adjustment used by the non-dispatch alternative path. Anchor the
change at the op.m_checkAdjust block and preserve the existing frame-resume jump
behavior.
In `@Source/JavaScriptCore/yarr/YarrPattern.cpp`:
- Around line 2608-2647: Update the repeated-alternative eligibility check
before constructing groupDisjunction so repeatedCount == 0 returns immediately,
while preserving the existing threshold behavior for non-empty ranges. Ensure
the loop that populates groupDisjunction and its final last() access only
execute when at least one repeated alternative exists.
- Around line 2637-2664: Mark the synthesized alternative returned by
body->addNewAlternative(...) as the last alternative by setting
wrapped->m_isLastAlternative to true before appending the group term, while
preserving the existing terminal marker handling for
groupDisjunction->m_alternatives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de1af30c-ea73-43b9-8a5c-31469f281caa
📒 Files selected for processing (5)
Source/JavaScriptCore/runtime/OptionsList.hSource/JavaScriptCore/yarr/Yarr.hSource/JavaScriptCore/yarr/YarrJIT.cppSource/JavaScriptCore/yarr/YarrPattern.cppSource/JavaScriptCore/yarr/YarrPattern.h
Correctness:
- YarrPattern::atomParenthesesEnd bubbled m_startsWithBOL from a group
whose alternatives all start with ^ onto the enclosing alternative even
when the group was not the alternative's first term or was a negative
assertion; and quantifyAtom kept the flag after a zero minimum count.
optimizeBOL then marked the body once-through and the JIT tried only
index 0. Enabling lookbehind in the JIT exposed this: /a(?<!x)(^)?$/
.exec("xa") went from ["a",null] to null. Set the flag only for a
first-term, non-inverted group (mirroring assertionBOL) and clear it
when the first term becomes optional. This also fixes the pre-existing
JIT-only /(?:^)?a/, /(^)*x/, /\B(?:^)?/ family on trunk.
- Decide the lookbehind interpreter fallback per pattern (eitherUnicode)
rather than per subject encoding (m_decodeSurrogatePairs), so the
8-bit and 16-bit specializations of one RegExp always run the same
engine and give the same captures.
- Cap inlined literal alternatives at 32 characters: the inline path
unrolls one compare per repeat, so /(?:a{200000000}|b|c|d)/ drove a
compile-time OOM. Long/counted literals keep the entered path, whose
fixed-count loop runs at match time.
- Never fold fewer than two repeated alternatives into a group;
regExpAlternationGroupThreshold=0 previously dereferenced an empty
synthesized group on all-once-through bodies.
Cleanup:
- Add the missing WTF_MAKE_TZONE_ALLOCATED_IMPL(DispatchInfo).
- Drop the unreachable entryBound/entries direct-jump path (every stub
target is a forward reference), the never-read DispatchInfo::sets
member, and the provably-empty endOp->m_jumps append under dispatch.
- Correct the regExpAlternationGroupThreshold description (it gates
prefix factoring at every level as well as the top-level fold).
Adds the reviewer's reproducers to regexp-lookbehind-jit.js.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Source/JavaScriptCore/yarr/YarrPattern.cpp (1)
2613-2680: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
wrappedalternative never getsm_isLastAlternative = true.The prior crash-on-empty-group finding (
repeatedCount < 2 ||guard) is fixed here. However, the new body-level alternative created at Line 2669 (wrapped) is the only remaining alternative afteralternatives.shrink(firstRepeated)appends it, yet nothing setswrapped->m_isLastAlternative = true. Every absorbed alternative had this flag explicitly cleared (Line 2652), and the codebase's consistent convention (checkForTerminalParentheses,atomParenthesesEnd,factorAlternatives,mergeSharedPrefix, and this same function's owngroupDisjunction->m_alternatives.last()->m_isLastAlternative = trueat Line 2660) confirmsaddNewAlternativedoes not default this to true. Net effect: after this transform, no alternative inbody->m_alternativesis marked as last.🐛 Proposed fix
PatternAlternative* wrapped = body->addNewAlternative(hasCaptures ? firstCaptureId - 1 : m_pattern.m_numSubpatterns); wrapped->m_lastSubpatternId = groupLastSubpatternId; wrapped->m_containsBOL = containsBOL; wrapped->m_startsWithBOL = startsWithBOLCount == groupDisjunction->m_alternatives.size(); + wrapped->m_isLastAlternative = true; groupDisjunction->m_parent = wrapped;#!/bin/bash # Confirm the default value of m_isLastAlternative and how it's consumed for body-level alternatives. rg -n 'm_isLastAlternative' Source/JavaScriptCore/yarr/YarrPattern.h Source/JavaScriptCore/yarr/YarrJIT.cpp Source/JavaScriptCore/yarr/YarrInterpreter.cpp🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/yarr/YarrPattern.cpp` around lines 2613 - 2680, Mark the newly created body-level alternative as terminal in wrapAlternativesForDispatch by setting wrapped->m_isLastAlternative to true after addNewAlternative. Preserve the existing terminal marking for the nested group’s final alternative.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Source/JavaScriptCore/yarr/YarrPattern.cpp`:
- Around line 2613-2680: Mark the newly created body-level alternative as
terminal in wrapAlternativesForDispatch by setting wrapped->m_isLastAlternative
to true after addNewAlternative. Preserve the existing terminal marking for the
nested group’s final alternative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 159f500d-9431-4417-ae9e-a88a6e336ff1
📒 Files selected for processing (4)
JSTests/stress/regexp-lookbehind-jit.jsSource/JavaScriptCore/runtime/OptionsList.hSource/JavaScriptCore/yarr/YarrJIT.cppSource/JavaScriptCore/yarr/YarrPattern.cpp
There was a problem hiding this comment.
Thanks — the fixup commit addresses the two functional items from the last pass (the repeatedCount < 2 guard in wrapAlternativesForDispatch and WTF_MAKE_TZONE_ALLOCATED_IMPL(DispatchInfo)). No new issues found on this revision, but this is a large change to Yarr JIT codegen and pattern rewriting that should get a human review.
Checked this round:
- Mirrored-frame primitives (claim/release,
negativePositionUntilInputBeginaddressing, capture start/end swap, BOL/EOL/\b) for forward/backward symmetry. wrapAlternativesForDispatch: the concern that the synthesized body alternative lacksm_isLastAlternativewas examined and ruled out — the flag is not load-bearing on the body disjunction path.tryPrepareDispatchgating (Forward-only, FixedCount 1:1, non-terminal/non-string-list,m_minimumSize > 0) vs. the assumptions the dispatch generate/backtrack paths make.- The remaining open inline comment (duplicate comment sentence) is cosmetic only.
Extended reasoning...
Overview
This PR adds three substantial features to the Yarr regex engine across ~1,500 lines of new logic in YarrJIT.cpp and YarrPattern.cpp: (1) JIT compilation of lookbehind assertions via term-reversed "mirrored" bodies with a per-op MatchDirection that flips the input-claim / character-addressing / anchor / capture primitives; (2) an engine-neutral pattern rewrite pass (factorAndWrapAlternatives) that stably sorts literal-leading alternatives, hoists shared prefixes into synthesized (?:…) groups, and folds large top-level alternations into a single group; and (3) a first-character dispatch scheme in the JIT that reads one character, routes through a binary decision tree to per-character chains, and threads chain continuation through a new BackTrackInfoParenthesesOnce::chainResume frame slot. It also removes the blanket m_containsLookbehinds JIT bailout in RegExp.cpp, adds four new Options, bumps YarrStackSpaceForBackTrackInfoParenthesesOnce 2→3, and fixes a pre-existing m_startsWithBOL bubbling bug in atomParenthesesEnd/quantifyAtom.
Prior review and current state
My previous run flagged three items. The fixup commit (b9615a1) addressed the two functional ones: wrapAlternativesForDispatch now guards repeatedCount < 2 so --regExpAlternationGroupThreshold=0 on an all-once-through body no longer reaches .last() on an empty vector, and WTF_MAKE_TZONE_ALLOCATED_IMPL(DispatchInfo) is now present. The third (a duplicated comment sentence) remains and is purely cosmetic. This run's finder/verifier pass raised and refuted the "body left with no m_isLastAlternative" concern for wrapAlternativesForDispatch — the body-disjunction path in the JIT does not rely on that flag the way nested alternatives do. No other findings.
Security risks
Regex JIT codegen is security-sensitive: it emits machine code that reads attacker-controlled input strings against attacker-controlled patterns. The new backward addressing (input[index + (k-1)] with k==0 mapping to index-1), the signed borrow-based bounds check, the frame-slot indirect jump through chainResume, and the pattern-tree restructuring (reparenting, capture-span recomputation, terminal-mark clearing) all have OOB-read / control-flow-hijack surface if any invariant is off by one. Nothing concrete was found, and the PR's differential harnesses (JIT vs. interpreter over hundreds of thousands of cases) plus the full stress/test262 pass are strong evidence — but this is exactly the kind of change where a second pair of expert eyes on the index arithmetic and frame layout is warranted.
Level of scrutiny
High. This is production-critical engine code (every regex in every Bun program), the diff is large, it introduces new JIT control-flow patterns (patched frame stores driving indirect jumps), and it changes observable regex semantics for a family of BOL-anchored patterns (a bug fix, but still a behavior change). It is well outside the "simple/mechanical" bar for auto-approval.
Other factors
The verification story is unusually thorough (randomized differential fuzzing against both the interpreter and node, enumerated small-alternation coverage, full stress + test262), and everything is behind runtime options with sensible defaults. The design is internally well-documented. Outstanding non-blocking bot comments remain (CodeRabbit's defensive-assert suggestion on the dispatch backtrack path, the test helper's undefined-vs-null comparison, and my comment-cosmetics nit). None of those block merge, but the scope alone means this needs a human reviewer familiar with Yarr.
…f narrowing BOL bubbling
Revert the m_startsWithBOL edits from the previous commit and gate instead.
An alternative's m_startsWithBOL is not just an anchoring hint: optimizeBOL
uses it to exclude the alternative from the loop copy, and copyTerm's
filterStartsWithBOL silently drops BOL-anchored sub-alternatives when
copying. That dropping is only semantics-preserving because the eager
bubbling in atomParenthesesEnd guarantees such alternatives never reach
the loop copy. Narrowing the bubbling (as the previous commit did) let
/.(^)X/ reach the loop copy, where the (^) group was deleted and the
pattern over-matched ("aX" against /.(^)X/) -- worse than the bug it
was addressing. So restore trunk's bubbling exactly.
The lookbehind JIT still must not newly route these patterns into the
JIT's stricter once-through handling (a pre-existing JIT-only defect the
interpreter tolerates). Record when a group's alternatives all start
with BOL (m_containsBOLGroupBubble) and keep lookbehind patterns with
such a bubble on the interpreter, i.e. their exact previous behaviour.
Bare ^ alternatives (as in the isbot pattern) do not set the flag, so
the target case still JITs.
The (?:^)?a family without lookbehind is a pre-existing upstream JIT
defect (interpreter and V8 agree); the stress test now pins its
per-tier output rather than deleting the cases.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Source/JavaScriptCore/yarr/YarrPattern.cpp (1)
2665-2673:⚠️ Potential issue | 🟠 MajorRestore the terminal marker on the synthesized body alternative.
wrappedis created withoutm_isLastAlternative, while the previous terminal marker was cleared from the moved alternatives. The body can therefore have no terminal alternative. Setwrapped->m_isLastAlternative = truebefore appending the group.Proposed fix
PatternAlternative* wrapped = body->addNewAlternative(hasCaptures ? firstCaptureId - 1 : m_pattern.m_numSubpatterns); wrapped->m_lastSubpatternId = groupLastSubpatternId; +wrapped->m_isLastAlternative = true; wrapped->m_containsBOL = containsBOL;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Source/JavaScriptCore/yarr/YarrPattern.cpp` around lines 2665 - 2673, Set wrapped->m_isLastAlternative to true immediately after creating the synthesized alternative in the surrounding group-wrapping logic, before appending the ParenthesesSubpattern group, so the synthesized body retains the terminal-alternative marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@JSTests/stress/regexp-lookbehind-jit.js`:
- Around line 148-160: Update the pre-existing JIT defect comment above the
useRegExpJIT assertions to include a link or identifier for a tracking issue
covering the three divergent regular-expression cases. Keep the existing
per-tier expectations and test logic unchanged.
---
Duplicate comments:
In `@Source/JavaScriptCore/yarr/YarrPattern.cpp`:
- Around line 2665-2673: Set wrapped->m_isLastAlternative to true immediately
after creating the synthesized alternative in the surrounding group-wrapping
logic, before appending the ParenthesesSubpattern group, so the synthesized body
retains the terminal-alternative marker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: beb7c340-33af-444d-b0b3-9869b71be2aa
📒 Files selected for processing (4)
JSTests/stress/regexp-lookbehind-jit.jsSource/JavaScriptCore/yarr/YarrJIT.cppSource/JavaScriptCore/yarr/YarrPattern.cppSource/JavaScriptCore/yarr/YarrPattern.h
There was a problem hiding this comment.
Thanks for the fixes in b9615a1 — the repeatedCount < 2 clamp and WTF_MAKE_TZONE_ALLOCATED_IMPL(DispatchInfo) are both in place now. I didn't find any new issues in this revision, but given the scope (backward-matching JIT primitives, engine-neutral pattern rewrites, first-character dispatch codegen and backtracking) this really wants eyes from someone who knows Yarr well.
Checked this run: the new m_containsBOLGroupBubble fallback keeps BOL-group lookbehind patterns on the interpreter without narrowing which non-lookbehind patterns reach the JIT; the synthesized wrapper alternative's unset m_isLastAlternative was examined and found harmless for body-level consumers; the test's JSON.stringify comparator masking undefined vs null was noted as a test-quality nit only. The remaining open inline notes (duplicate comment line, dispatch-backtrack direction assert) are cosmetic.
Extended reasoning...
Overview
This PR adds ~1,500 lines to YarrJIT.cpp and ~340 to YarrPattern.cpp, spanning three interacting features: (1) JIT compilation of lookbehind bodies via term-reversed mirroring with a per-op MatchDirection that flips input-claim/read/anchor/capture primitives; (2) an engine-neutral pattern rewrite that stably sorts and factors literal-leading alternatives and folds large top-level alternations into a synthetic (?:...) group; and (3) a first-character dispatch JIT path that reads one character, routes through a binary decision tree to per-character chains, and threads failure through a new chainResume frame slot. It also adds four runtime options, grows BackTrackInfoParenthesesOnce by one slot, adds a m_containsBOLGroupBubble pattern flag, and includes a stress test.
Security risks
This is JIT code generation for attacker-controlled input (regex patterns and subject strings) in a JavaScript engine. The backward-matching path introduces new signed-arithmetic index handling and a new BaseIndex addressing mode; the dispatch path stores and indirect-jumps through code addresses in the match frame; the pattern rewrite restructures the AST that downstream passes (setupOffsets, terminal/string-list optimizations, JIT op-compile) consume. Any of these could plausibly harbor OOB reads, index-register corruption, or backtracking-state confusion that only manifests on specific pattern shapes. The differential harnesses described in the PR are strong evidence of correctness, but this class of change is exactly where subtle exploitable bugs hide.
Level of scrutiny
High. This is neither mechanical nor small; it touches production-critical, security-sensitive codegen with many interacting invariants (frame offsets, checked-input accounting, direction state, capture-span bookkeeping, terminal/string-list marks). It is well outside the "approve without human review" bar.
Other factors
Since my previous inline comments the author pushed a fix commit addressing the threshold=0 crash and the missing TZone _IMPL, plus a follow-up (1e122871) that replaces the earlier BOL-bubbling narrowing with a per-pattern m_containsBOLGroupBubble flag so lookbehind patterns of that shape stay on the interpreter — a more conservative fix. The bug-hunting system found nothing this run; verifiers specifically examined and refuted the "wrapper alternative not marked m_isLastAlternative" concern and the test comparator's undefined/null conflation. The remaining open inline items (a duplicated comment sentence in wrapAlternativesForDispatch, and CodeRabbit's suggestion to assert Forward direction in the dispatch backtrack) are non-blocking cosmetics. Deferring to a human Yarr reviewer.
…nery
Lookbehind bodies are compiled as term-reversed mirrored copies through the
forward generator with Backward ops, so unicode (surrogate-pair) lookbehinds
no longer fall back to the interpreter. This adds leftward decoding
(readUnicodeCharBackward, matching the interpreter's tryReadBackward),
backward astral literals and character classes for every quantifier form,
backreferences and quantified/nested groups inside mirrored bodies, and
nested forward assertions renumbered from their mirrored parent position.
The lookbehind compile gate and its options are removed.
Correctness work required to make that sound:
- General real-position tests for ^ and \b (index == checked - pos)
replace the "not at position 0" shortcuts, which are wrong for an anchor
whose alternative is numbered from a nonzero origin; ^ inside a lookbehind
never marks startsWithBOL, and quantifying a BOL group {0} withdraws the
anchor before the term is elided.
- A nested forward assertion's body is deep-copied for the mirror so
renumbering never mutates the pattern the interpreter fallback reuses.
- The NonGreedy paren "try one more iteration" guard is direction-aware.
- The dotAll any-character class rejects a dangling surrogate in unicode
compiles like every other class path.
- The first-non-BMP-character register is written only by the match-start
read (ambient-off, enabled while emitting that one term), so loop peeks
and backtrack re-reads can't leave a stale astral flag that over-advances
the next start; the whole optimization is additionally disabled for
patterns that can match zero-width, since a zero-width alternative can
legitimately succeed at a mid-surrogate-pair position that the skip drops.
- Quantified splits (X{min,max} -> mandatory copy + optional {0,max-min}
copy sharing X's capture ids): the optional copy's abandoned iteration
clears the shared capture, and control re-emerges through the mandatory
group's End, so each paren End emitter re-establishes a copy owner's
entry-side capture endpoint from that iteration's saved start index
(parentheses.hasCopySibling). Backward parens now take this same path.
- A leftward pair-lead borrow undoes its failed borrow before joining the
failure jumps so a negative frontier is never left live for a sibling op.
There was a problem hiding this comment.
Beyond the two inline nits, I also checked the open coderabbit concern about wrapped->m_isLastAlternative in wrapAlternativesForDispatch — m_isLastAlternative is only read for {Simple,}Nested/StringList alternative ops (YarrJIT.cpp:4792/4826), never for body alternatives, so leaving it unset on the synthesized body alternative is harmless. The earlier regExpAlternationGroupThreshold=0 and missing WTF_MAKE_TZONE_ALLOCATED_IMPL(DispatchInfo) findings are addressed in the current diff.
Extended reasoning...
The two new inline findings are cosmetic (a decimal-vs-hex mismatch in a verboseRegExpCompilation dump, and two empty #if/#endif pairs left after removing their bodies). The separately-raised concern that wrapAlternativesForDispatch leaves the body with no m_isLastAlternative marker was verified not to matter: that flag is consulted only in the nested/string-list alternative codegen paths, and the synthesized alternative is a body alternative compiled via BodyAlternativeBegin/Next/End, which never reads it.
dylan-conway
changed the title
- Print non-printable first-set characters in hex in the verboseRegExpCompilation dump; the \x prefix was followed by a decimal value. - Remove the two #if/#endif pairs left empty when the ambient first-non-BMP-character scopes were deleted, and a duplicated draft comment line in wrapAlternativesForDispatch. - In regexp-lookbehind-jit.js, compare results with a strict deep equality that distinguishes an undefined array element (a non-participating capture) from null. JSON.stringify maps both to null, so a wrongly-nulled capture could have passed the capture-clearing assertions. No-Verification-Needed: comment, verbose-dump, and test-only edits; no product code path changes
| if (m_direction == Backward && m_decodeSurrogatePairs) { | ||
| // Backward + unicode: read the code point that ends at the term's | ||
| // position (its last unit is at frame offset checkedOffset - position). | ||
| // The class match runs first and leaves the frontier alone, preserving | ||
| // `character` for the width test just as the forward path does; | ||
| // beginIndex (stored above) restores the frontier on backtrack. | ||
| readUnicodeCharBackward(negativeOffsetIndexedAddress(op.m_checkedOffset - term->inputPosition, character), character, scratch); | ||
| matchCharacterClassTermInner(term, op.m_jumps, character, scratch); | ||
| // A static-width class was laid out at its true width (2 units when its | ||
| // members are all astral) and its claim already covers the whole pair. | ||
| // Only a variable-width class was budgeted a single unit, so only it | ||
| // grows the leftward claim by one when the code point was a pair | ||
| // (failing when the frontier is exhausted). | ||
| if (!term->isFixedWidthCharacterClass()) { | ||
| MacroAssembler::Jump isBMPChar = m_jit.branch32(MacroAssembler::LessThan, character, MacroAssembler::TrustedImm32(0x10000)); | ||
| claimBackwardPairLead(op.m_jumps); // claim the pair's lead unit | ||
| isBMPChar.link(&m_jit); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Nit: readUnicodeCharBackward is defined inside #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) (L1525–L1715), but the five backward-unicode call sites here and at L3848, L3990, L4089, L4162 are not wrapped in that guard — only the sixth caller (readCharacterForDirection, L1743) is. m_decodeSurrogatePairs is a runtime const bool, so the compiler still resolves the name; on a target with ENABLE(YARR_JIT) but without ENABLE_YARR_JIT_UNICODE_EXPRESSIONS (only ARM64/X86_64/RISCV64 set it) these fail to compile. Bun's x64/arm64 CI is unaffected — same class as the earlier TZONE_MALLOC finding — but wrapping each block in the guard would match the file's convention (the immediately adjacent forward-path m_decodeSurrogatePairs blocks all use it).
Extended reasoning...
What the bug is
readUnicodeCharBackward() is defined at YarrJIT.cpp:1564, inside the #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) block that opens at L1525 and closes at L1715. This PR adds six call sites; one of them — in readCharacterForDirection (L1743–1749) — is correctly wrapped in #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) / #endif. The other five are not:
- L3744 (
generateCharacterClassOnce) — sits between#endifat L3736 and#ifat L3763 - L3848 (
generateCharacterClassFixed) - L3990 (
generateCharacterClassGreedy) - L4089 (
backtrackCharacterClassGreedy) - L4162 (
backtrackCharacterClassNonGreedy)
Each is inside a runtime if (m_direction == Backward && m_decodeSurrogatePairs) { ... } block, but m_decodeSurrogatePairs is declared as const bool m_decodeSurrogatePairs : 1; (L9022) — a runtime member bitfield, not a compile-time constant — so the compiler must still perform name lookup on readUnicodeCharBackward regardless of whether the branch is ever taken.
The code path that triggers it
Per PlatformEnable.h:949–951, ENABLE_YARR_JIT_UNICODE_EXPRESSIONS is defined only for CPU(ARM64) || CPU(X86_64) || CPU(RISCV64), whereas ENABLE_YARR_JIT (L924–925) is on for any ENABLE(JIT) port. On a target where ENABLE(YARR_JIT) is on but ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) is off (e.g. ARM_THUMB2 in an upstream WebKit config), readUnicodeCharBackward is preprocessed away, and these five call sites fail to compile with an undeclared-identifier error.
Why nothing prevents it
The runtime early-bail at L8435–8439 (which returns JITFailureReason::DecodeSurrogatePair when m_decodeSurrogatePairs under !ENABLE(YARR_JIT_UNICODE_EXPRESSIONS)) does not help: that guarantees the branch is never taken at runtime, but this is a compile-time name-lookup failure. The file's own convention is to wrap every m_decodeSurrogatePairs block that calls a unicode-only helper in the guard — the immediately adjacent forward-path blocks at L3724–3736, L3763–3770, L3811–3835, L4007–4010, L4102–4105, L4178–4181 all do so, and the sixth backward call site (L1743–1749) does too. The five character-class sites are the only ones that break the convention.
Step-by-step proof
- Build with
ENABLE(YARR_JIT)=1andENABLE(YARR_JIT_UNICODE_EXPRESSIONS)=0(any CPU other than ARM64/X86_64/RISCV64 with the JIT enabled). - The preprocessor removes L1525–1715, so
readUnicodeCharBackwardhas no declaration. - Compiling
generateCharacterClassOncereaches L3738:if (m_direction == Backward && m_decodeSurrogatePairs) {. Sincem_decodeSurrogatePairsis a non-constexpr member, the compiler must compile the body. - L3744 references
readUnicodeCharBackward, which is undeclared → compilation error. - The same happens at the other four sites.
Contrast with L1741–1749 in the same PR: readCharacterForDirection wraps its identical if (m_direction == Backward && m_decodeSurrogatePairs) { readUnicodeCharBackward(...); ... } block in #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) / #endif and compiles cleanly.
Impact
None on Bun's shipped builds: Bun's CI targets x64/arm64 only, both of which enable YARR_JIT_UNICODE_EXPRESSIONS; and this fork's PlatformEnable.h forces ENABLE_JIT=0 on 32-bit, so no default configuration in this repo actually breaks. This is the same class of issue as the WTF_MAKE_TZONE_ALLOCATED_IMPL(DispatchInfo) finding already accepted and fixed in this PR — a divergence from the file's defensive-guard convention that breaks a non-default (or upstream) build configuration.
Fix
Wrap each of the five if (m_direction == Backward && m_decodeSurrogatePairs) { ... } blocks in #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) / #endif, matching readCharacterForDirection and the adjacent forward-path blocks. (Alternatively, moving the readUnicodeCharBackward definition outside the guard would also work, but the per-site guard is the file's established convention.)
…t unicode subjects The JIT's Boyer-Moore search reads one character and skips a stride of positions when that character cannot start a match. Choosing the range to test only ever scored maximal runs of positions under a candidate-count limit, so a short and highly selective range was thrown away whenever an adjacent position widened its union. /zalpha|qbravo|xcharlie/ is led by three rare characters, but merging the positions behind them pulls in most of the alphabet; the merged set then matches almost every character, scores below the 50% cutoff, and the search was dropped altogether rather than falling back to testing the leading characters alone. Score every sub-range instead. The candidate-count eligibility is kept, since a position matching many characters hits far more often than its sampled frequency suggests once the pattern itself matches often. Extending a range only adds characters, so a range is abandoned as soon as its match probability turns negative: a longer stride can only scale an already-negative score further down. A range whose union is empty is skipped, because the search needs at least one character to test and a frequency of zero would otherwise outscore every real range; an 8-bit compile of a class holding only characters above Latin1 produces exactly that, and no alternative can match there at all. The search was also refused for every unicode pattern, because a stride counts code units and could land inside a surrogate pair. A Latin1 subject holds no surrogates whatever the pattern's flags, and the collection code already keys off m_decodeSurrogatePairs, so gate the search on that rather than on the flag. This enables it for /u and /v patterns matched against 8-bit subjects and leaves 16-bit unicode subjects, where the hazard is real, exactly as they were. Alternation scans over 1.5MB of prose speed up by 2.4-5.9x, and the unicode penalty on 8-bit subjects disappears: /zalpha|qbravo|xcharlie/gu goes from 4.8ms to 1.0ms, matching its non-unicode compile. Compile time grows 4-6% on the longest patterns. Matches are unchanged -- the JIT and the interpreter stay byte-identical across 52,000 randomized differential cases and three 6MB corpora.
|
|
||
| #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) && ENABLE(YARR_JIT_UNICODE_CAN_INCREMENT_INDEX_FOR_NON_BMP) | ||
| // Prevent word boundary assertion reads from corrupting firstCharacterAdditionalReadSize. | ||
| SetForScope useOptimizationScope(m_useFirstNonBMPCharacterOptimization, false); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
🟡 Nit: the fixup commit 4d91f2c removed the empty #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) && ENABLE(YARR_JIT_UNICODE_CAN_INCREMENT_INDEX_FOR_NON_BMP) / #endif pairs, but the blank lines that flanked each guard were both kept, leaving two consecutive blank lines here (between scratch = m_regs.regT1; and MacroAssembler::Jump atBegin;) and again in matchBackreference at ~L2677-2678 (between unsigned duplicateNamedGroupId = ...; and MacroAssembler::Label loop(&m_jit);). Purely cosmetic — drop one blank line at each site.
Extended reasoning...
What
An earlier review comment on this PR pointed out that removing the SetForScope useOptimizationScope(...) bodies had left two empty #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) && ENABLE(YARR_JIT_UNICODE_CAN_INCREMENT_INDEX_FOR_NON_BMP) / #endif pairs. The fixup commit 4d91f2c deleted those two guard lines at each site — but each guard had been separated from its neighbours by a blank line above and a blank line below, and both of those blank lines were context (unchanged) in the fixup. With the guard gone, the two flanking blanks are now adjacent.
Where
generateAssertionWordBoundary, YarrJIT.cpp:2596-2597 — betweenconst MacroAssembler::RegisterID scratch = m_regs.regT1;andMacroAssembler::Jump atBegin;.matchBackreference, YarrJIT.cpp:2677-2678 — betweenunsigned duplicateNamedGroupId = m_pattern.hasDuplicateNamedCaptureGroups() ? ... : 0;andMacroAssembler::Label loop(&m_jit);.
Step-by-step
- Before 4d91f2c,
generateAssertionWordBoundaryread:const MacroAssembler::RegisterID scratch = m_regs.regT1; #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) && ENABLE(YARR_JIT_UNICODE_CAN_INCREMENT_INDEX_FOR_NON_BMP) #endif MacroAssembler::Jump atBegin;
- The fixup deleted the two
#if/#endiflines. The blank line above and the blank line below were both context, so both survive. - Post-fixup source (verified with
cat -A) shows lines 2596 and 2597 are both empty:const MacroAssembler::RegisterID scratch = m_regs.regT1;$ $ $ MacroAssembler::Jump atBegin;$ - The identical edit shape appears in
matchBackreference;cat -Aon lines 2677-2678 likewise shows two consecutive$-only lines.
Why nothing prevents it / impact
Consecutive blank lines are legal C++ and have no effect on compilation or generated code, so no compiler, linter, or test in CI flags them. This is purely cosmetic editing debris introduced by this PR's own fixup commit — no functional impact whatsoever.
Fix
Delete one blank line at each of the two sites (two lines total).
…pear The JIT has two Boyer-Moore searches: a vector one reading sixteen characters per iteration, and a scalar one reading a character per iteration. The vector search was preferred wherever it applies, but leaving its loop costs a vector to general purpose register transfer, so it pays that price once per candidate that fails to match. It wins by 8x on a subject holding no candidate at all and loses by 6.5x once half the characters are candidates, and nothing measured which side of that it was on. How often a candidate fails is not known when compiling, so use the frequency the subject sampler already collects for choosing the search range. The sampler scales its count to a rate, which a subject shorter than the sample cannot resolve -- one candidate in four characters reads as a rate of 32 -- and such a subject says little about the later ones a compile is reused for, since a RegExp keeps the first one's code. Count the candidates instead in that case, as an absent sample already does, so an incidental first subject cannot decide the search for the whole process. That choice then replaces the candidate-count limit on which positions the range search may test. The limit stood in for "this set matches too often", but the sampled frequency measures that directly, and the cost it was really standing in for was the vector loop's. It also rejected the best filters: /([a-z0-9])([A-Z])/ over source text was left with no search at all because both of its positions hold more characters than the limit allows, though upper case runs at nearly zero frequency there and rejects every position at once. Positions matching every character still have to be excluded, for a different and load-bearing reason: setAll() records them in the count alone and leaves the map empty, so merging one drops every character it matches out of the union and the search skips positions that can start a match. The count limit had been hiding that, since such positions count as the whole map. Test it directly instead. /([a-z0-9])([A-Z])/ over source text goes from 2.2ms to 0.1ms, keyword alternation scans get 1.8-2.4x faster, and no case measured across a 26 benchmark suite, 20 library patterns and a scan suite gets slower. Matches are unchanged: the JIT and the interpreter stay byte-identical over 34,000 randomized differential cases and three 6MB corpora, and test262 and the regexp stress tests are unmoved on both tiers. The crossover was measured on arm64.
dylan-conway
changed the title
There was a problem hiding this comment.
No new issues found in this pass, but this is a large change to the Yarr JIT — new backward-matching codegen for lookbehinds (including unicode), pattern-level alternation rewrites, first-character dispatch, frame-layout growth, and a Boyer-Moore search rewrite — so it needs a human reviewer.
What was reviewed:
- Direction-aware input primitives and
negativeOffsetIndexedAddressbackward mapping — offsets stay within claimed input and the negative-frontier borrow is undone on failure. factorAlternatives/wrapAlternativesForDispatch— stable sort preserves leftmost-first ordering within same-first-character groups; terminal/string-list shapes are left untouched.- Frame-size bumps in
Yarr.hline up with the newBackTrackInfoParenthesesOnce::chainResumeandBackTrackInfoBackReference::backwardSpanEdgeslots. findBestCharacterSequencerewrite — empty-union ranges are skipped so an all-above-Latin1 class can't score as the best window.
Extended reasoning...
Overview
The PR spans seven files but the substance is in YarrJIT.cpp (~1.8k new lines) and YarrPattern.cpp (~400 new lines). It: (1) removes the compile-time lookbehind gate in RegExp.cpp and JIT-compiles lookbehind bodies via a term-reversed mirror driven through the existing forward machinery, with a per-op MatchDirection flipping the input-claim / addressing / end-of-input / capture-slot primitives; (2) adds a leftward unicode code-point reader and backward variants of every character-class and pattern-character quantifier form; (3) adds engine-neutral alternation prefix factoring and a body-folding wrapper in the pattern constructor; (4) adds first-character dispatch for once-groups in the JIT (binary decision tree over Latin-1 → per-character chain of applicable alternatives, with a new chainResume frame slot); (5) rewrites the Boyer-Moore sub-range scoring and adds a subject-frequency gate on the SIMD search loop; and (6) fixes several pre-existing correctness bugs (BOL/word-boundary position tests, firstCharacterAdditionalReadSize scoping, quantified-split capture clearing, dotAll dangling-surrogate rejection).
Security risks
Regex JIT is memory-safety–critical. The new backward addressing path computes input[index + (k-1)] from a leftward-moving frontier and relies on signed-branch underflow detection for input claims; an off-by-one in the offset mapping or a missed borrow-undo would be an OOB read against attacker-controlled subject strings. The dispatch machinery stores and later jumps through a code address in the runtime frame (chainResume), which is the same shape as the existing returnAddress slot but is a new indirect-jump surface. The alternation-factoring rewrite mutates the pattern tree (reparenting, capture-range recomputation) before offset assignment; a mis-bracketed capture span would corrupt the output vector. I did not find a concrete flaw in any of these paths, and the PR's differential and fuzz coverage is extensive, but the surface area is exactly the kind that has historically produced exploitable JIT bugs.
Level of scrutiny
High. This is production-critical engine code, not sandbox or tooling, and it removes a safety fallback (lookbehinds previously always ran in the interpreter). The mirroring design, the frame-layout changes shared with the interpreter, and the pattern-rewrite semantics are all architectural decisions a maintainer should sign off on.
Other factors
The verification story is strong — full test262 RegExp on both tiers, three JIT-vs-interpreter differential corpora, a large enumerated small-alternation sweep, and randomized fuzzing across quantifiers/lookarounds/unicode — and the added regexp-lookbehind-jit.js stress file pins the reproducers. All prior review threads are resolved; the two inline nits still open (unguarded readUnicodeCharBackward call sites on non-x64/arm64/riscv64 builds, and a cosmetic double blank line) don't affect Bun's shipped configs. Net: nothing blocking from automated review, but scope and risk class put this well outside auto-approval.
The search may not span a position that matches every character: setAll() records one in its count alone and leaves its map empty, so merging it drops every character it matches out of the range's union and the search then skips positions that can start a match. Nothing states that invariant, and nothing tested it -- all eight existing bm-search tests pass against a build that violates it, while "bab".match(/.(a)./) returns null. Every case expects a match, because each failure in this class is a false negative and an expectation of null would be exactly what a broken search produces. Each is written with single dots rather than a quantified count, since a count aborts collection before any position is recorded and would leave nothing to get wrong: of an earlier draft's assertions, only the unquantified ones failed against the violating build. The remaining cases cover the search now being available to unicode patterns over Latin1 subjects. No-Verification-Needed: test-only; the engine change it covers landed with its own verification
There was a problem hiding this comment.
No new issues found this pass, but this is a ~2,000-line change to the Yarr JIT's core codegen and pattern layer (mirrored lookbehind compilation, direction-aware primitives, alternation prefix factoring/reordering, first-character dispatch chains, Boyer-Moore range scoring), so it needs a human reviewer.
What was reviewed:
- Direction-aware input-claim/addressing/backtrack primitives and the mirrored-body offset assignment — checked that forward paths are byte-identical when
m_direction == Forward. - Alternation factoring's stable sort + barrier rules and
wrapAlternativesForDispatch— the compile-timealternationFactoringMinRun = 8guard makes the empty-.last()path unreachable. - Boyer-Moore
findBestCharacterSequencerewrite — theisAllSet()break and empty-mapcontinuecover the any-character / above-Latin1-only cases the tests pin. - Frame-slot growth (
YarrStackSpaceForBackTrackInfo*) matches the newBackTrackInfo*struct fields.
The two open inline nits (missing #if ENABLE(YARR_JIT_UNICODE_EXPRESSIONS) around five readUnicodeCharBackward call sites; a stray double blank line) are cosmetic / non-Bun-target only.
Extended reasoning...
Overview
This PR adds lookbehind JIT compilation to Yarr (previously an interpreter-only fallback), engine-neutral alternation prefix factoring and top-level folding in YarrPattern.cpp, first-character dispatch for wide alternations in YarrJIT.cpp, and reworks Boyer-Moore search-range scoring and SIMD-vs-scalar loop selection. It touches YarrJIT.cpp (~1,700 net lines), YarrPattern.cpp (~450 net lines), YarrPattern.h, Yarr.h, YarrJIT.h, RegExp.cpp, and adds two stress tests.
Security risks
Regex JIT is security-sensitive: it emits machine code from attacker-controllable patterns and subjects. The new backward-frame addressing (negativeOffsetIndexedAddress Backward branch, claimBackwardPairLead, checkNotEnoughInputBackward), the dispatch chain's patched frame stores (chainResume), and the pattern-level alternative reordering all create new surfaces for out-of-bounds reads or backtracking-state corruption if an invariant is violated. The PR's own verification (test262, differential JIT-vs-interpreter fuzzing, 52k BM cases) is substantial, but the design decisions — mirroring via deep-copied disjunctions, per-op direction stamping, forward-only dispatch — deserve a human reviewer familiar with Yarr's frame layout and backtracking model.
Level of scrutiny
High. This is core-engine JIT codegen with correctness and memory-safety implications, plus several large-scale design choices (mirrored-body compilation strategy, always-on pattern rewrites that reorder alternatives, frame-slot growth affecting the interpreter). It is well outside the "simple, mechanical, or obvious" bar for auto-approval.
Other factors
All prior review threads on this PR are resolved except two of my own cosmetic nits (unguarded readUnicodeCharBackward call sites for non-x64/arm64 targets, and a leftover double blank line); neither affects Bun's shipped builds. The bug-hunting pass on the current head found nothing new. Preview builds are green across all commits. No human has reviewed yet.
1 participant
Summary
Motivating case: oven-sh/bun#5197 — the
isbotuser-agent regex (~110 alternatives, four lookbehinds). This makes JSC faster than V8/node on it, improves large alternations generally, JIT-compiles lookbehinds — including unicode (/u,/v) patterns — teaches the Boyer-Moore search to serve alternations and unicode patterns, and fixes several pre-existing Yarr correctness bugs found along the way.Benchmark (the issue's own script, 361,829 user-agent strings, through the bun binary):
Unicode lookbehinds, which previously always fell back to the interpreter, no longer do: a
/iulookbehind alternation went from ~2.2 s to ~106 ms, an isbot-style negative lookbehind from ~750 ms to ~58 ms.What changed
Lookbehinds are JIT-compiled. Patterns containing a lookbehind previously fell back entirely to the interpreter. Each lookbehind body is now compiled as a term-reversed mirrored copy driven through the existing forward machinery, with a per-op
MatchDirectionflipping only the primitives (input claim, character addressing, end-of-input, greedy signs, capture start/end slots, mirrored^ $ \b). This covers unicode patterns too — a leftward code-point reader (matching the interpreter'stryReadBackward), backward astral literals and character classes in every quantifier form, backreferences, quantified/nested groups, and nested forward assertions inside a mirrored body. The compile-time lookbehind gate is gone; lookbehinds always JIT.Large alternations are cheaper. Engine-neutral pattern rewrites factor shared prefixes (
/aq|bx|ar|by/→/a(?:q|r)|b(?:x|y)/, recursing into groups) and fold large repeated bodies into one non-capturing group. In the JIT, a group's alternatives dispatch on the first character via a binary decision tree (routing to the ordered chain of alternatives that can start with it), and short literal alternatives compare inline with no frame traffic. SpecializedisStringList/isTerminalgroup shapes are left alone, and small alternations aren't restructured. The rewrites and dispatch are always on (no options); the sizing thresholds are compile-time constants.The Boyer-Moore search reaches patterns it used to skip. The JIT reads one character ahead of the body alternatives and skips a stride when it cannot start a match. Three things kept that from working:
/zalpha|qbravo|xcharlie/is led by three rare characters, but merging the positions behind them pulls in most of the alphabet, and the search was dropped altogether instead of testing the leading characters alone. Every sub-range is now scored./uand/vpatterns over 8-bit subjects and leaves 16-bit unicode subjects, where the hazard is real, as they were.That in turn replaced the candidate-count limit on which positions the search may test — the sampled frequency measures "matches too often" directly, and the limit rejected the best filters, leaving
/([a-z0-9])([A-Z])/over source text with no search at all because both its positions hold more characters than the limit allowed, though upper case runs at nearly zero frequency there. Positions matching every character are still excluded, for a load-bearing reason the count limit had been hiding:setAll()records them in the count alone and leaves the map empty, so merging one drops every character it matches out of the union and the search skips real match starts.Measured on 1.5 MB subjects, against the branch without these changes: keyword-alternation scans 1.8–5.9x,
/zalpha|qbravo|xcharlie/gu4.8 ms → 1.0 ms (the unicode penalty on 8-bit subjects disappears entirely, matching the non-unicode compile exactly), case-insensitive alternations 1.7x. Regex compile time grows 4–6% on the longest patterns. No case measured across a 26-benchmark suite, 20 library patterns and a scan suite gets slower.How much the broad-class case gains depends on how rare that class actually is in the subject, so measured against released bun 1.3.14 with a fresh process per point (the RegExp cache keys on source and flags, so one process would compile
/([a-z0-9])([A-Z])/against whichever subject came first and reuse it): 30x when the subject holds no upper case at all, 2.3x at 1%, 1.4x from 10% up. It is never slower than the released build, but a class the subject is full of is a filter that rejects nothing, and the search then only adds its own cost.Correctness fixes (mostly pre-existing bugs the new coverage exposed):
^and\bnow test the real input position instead of the "not at position 0" shortcut, which was wrong for an anchor inside a nested body; a^inside a lookbehind never marksstartsWithBOL, and BOL bubbling from groups matches trunk exactly.firstCharacterAdditionalReadSizeis written only by the match-start read (a stale astral flag could previously over-advance the next start, e.g./😀|-?a/u.exec("-😀")returnednull), and the astral-start skip is disabled for patterns that can match zero-width, since a zero-width alternative can legitimately match mid-surrogate-pair.{0,n}copy that ran zero iterations no longer clears capture ids it shares with its mandatory sibling; and forward parens with a non-zero minimum stay a single native term, so deeply nestedmin>0quantified groups no longer grow the pattern exponentially.Verification
built-ins/RegExp: all 3,754 tests pass on both the JIT and the bytecode-interpreter tier (fresh process per test), plus the 676String.prototyperegex-method tests; JSTests regexp stress files and the LayoutTests js/regexp + fast/regex sets pass on both tiers./vset operations, astral and lone-surrogate subjects, all flag combinations — no JIT-vs-interpreter divergences from this change, no crashes or hangs (including deeply nested quantified groups, forward and inside lookbehinds).JSTests/stress/regexp-lookbehind-jit.js(extended with every review reproducer) passes on both tiers and matches node./u,/v) byte-identical JIT-vs-interpreter-vs-node; test262 and the regexp stress suite unmoved on both tiers; the isbot match count is asserted, not just its timing — it is the canary that catches a search skipping real match starts, and two of the three corpora miss that failure.Known follow-ups (deferred)
{n}class or character, so/\d{4}-\d{2}/gets no search even though the pattern layer gives its-the exact offset 4. Hand-expanding the counts measures iso-date 1.7 ms → 0.9 ms and uuid 1.1 → 0.8. Implemented and reverted: the extra positions let the score pick a worse range for/#(?:[0-9a-fA-F]{3}|[0-9a-fA-F]{6})\b/, because the score rewards stride and penalizes hit rate but never models the cost of the match attempt the search exists to avoid. A cutoff sweep has no solution (that pattern needs ≤52 while keyword scans need ≥56); the model needs an attempt-cost term first.m_minimumSizeignores a quantified group's repeat count:/(?:ab){3}c/reports 1 where/abababc/reports 7. Safe, being an underestimate, but it caps the search's length at 1 for such patterns — the hand-expanded ipv4 is 4.2 ms against 9.4 ms for the{3}form.