mruby-regexp: keep the pike VM's visited keys from wrapping - #7073
Merged
Conversation
A step reserves one visited key per epsilon pass rather than a single key, so `s.gen` climbs by up to `2 * pass_span` per input byte where it used to climb by up to 2. `gen` and `key_max` are `uint32_t`, so the counter now wraps near 430 MB of subject instead of near 2 GB. Past a wrap, `visited[]` still holds the large keys earlier steps wrote. `add_thread()` then returns at `s->visited[pc] >= key` for every `pc`, the closure adds nothing, and the match silently fails. A wrapped key can also land on `UINT32_MAX`, which `re_loop_back()` returns as its `RE_LOOP_STOP` sentinel, so an ordinary fork would read as a loop that has to stop. Move both advance sites into `advance_gen()`, which clears the marks and starts the keys over before the counter can reach either. The marks only ever describe the closure being built, so clearing them costs one `memset()` per wrap and loses nothing, and the same guard holds every live key below the sentinel. Forcing the restart on every step, rather than once per wrap, leaves `rake test` green and both differential sweeps against CRuby identical to the run without it, so the restart path answers the same as the counter it replaces.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Pike VM now centralizes generation advancement and visited-state clearing. Initialization starts generation tracking at zero, and match setup plus per-character processing use the new helper. ChangesPike VM generation tracking
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #7071, which landed before this could be folded in.
That change has a step reserve one visited key per epsilon pass rather than a
single key, so
s.genclimbs by up to2 * pass_spanper input byte where itused to climb by up to 2.
genandkey_maxareuint32_t, so the counterwraps near 430 MB of subject instead of near 2 GB.
Past a wrap,
visited[]still holds the large keys earlier steps wrote.add_thread()returns ats->visited[pc] >= keyfor everypc, the closureadds nothing, and the match silently fails. A wrapped key can also land on
UINT32_MAX, whichre_loop_back()returns as itsRE_LOOP_STOPsentinel, soan ordinary fork would read as a loop that has to stop.
Both advance sites move into
advance_gen(), which clears the marks and startsthe keys over before the counter can reach either.
visited[]only everdescribes the closure being built, so clearing it costs one
memset()per wrapand loses nothing; the guard also holds every live key below the sentinel.
Verification
rake testpasses.The wrap itself needs a subject too large to test directly, so the restart path
was exercised by lowering the threshold to fire on every step. With that build,
rake teststays green and both differential sweeps against CRuby 4.0.6 (the12870-row structured sweep and the 56000-row random sweep used in #7071)
produce output identical to the build without the guard. The threshold was then
restored and the sweeps re-run to confirm the shipped build is unchanged.
Summary by CodeRabbit