Skip to content

Enable jitting in pcre2 #11195

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Feb 21, 2024
Merged

Enable jitting in pcre2 #11195

merged 7 commits into from
Feb 21, 2024

Conversation

addisoncrump
Copy link
Contributor

This corresponds to an issue on PCRE2 regarding fuzzer improvements w.r.t. JIT: PCRE2Project/pcre2#317

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 7, 2023

addisoncrump is a new contributor to projects/pcre2. The PR must be approved by known contributors before it can be merged. The past contributors are: joycebrum, PhilipHazel, devtty1er, inferno-chromium, Dor1s, ssbr (unverified), mikea (unverified), kcwu (unverified)

@addisoncrump addisoncrump marked this pull request as ready for review November 9, 2023 17:09
@addisoncrump
Copy link
Contributor Author

The linked PR has now been merged, so this should be ready to go. CI should probably be re-run as the latest changes from upstream need to be tested.

@addisoncrump addisoncrump marked this pull request as draft November 9, 2023 17:41
@addisoncrump
Copy link
Contributor Author

There was an issue in the original PR. Please wait before merging! :)

@addisoncrump addisoncrump mentioned this pull request Nov 14, 2023
Merged
@addisoncrump
Copy link
Contributor Author

Blocked by PCRE2Project/pcre2#322 which should be merged relatively soon.

@addisoncrump addisoncrump marked this pull request as ready for review February 16, 2024 11:57
@addisoncrump
Copy link
Contributor Author

Upstream has updated, can this PR be evaluated?

@addisoncrump
Copy link
Contributor Author

Build failures are due to known abort()s in the fuzzer target. I can temporarily disable these, or put them behind a build flag for now.

@addisoncrump addisoncrump mentioned this pull request Feb 16, 2024
Merged
@PhilipHazel
Copy link
Contributor

This pull request looks OK to me.

PhilipHazel
PhilipHazel approved these changes Feb 16, 2024
View reviewed changes
Copy link
Contributor

@PhilipHazel PhilipHazel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem fine.

@addisoncrump
Copy link
Contributor Author

Pipeline should be fully re-evaluated as upstream has been fixed to address the checks.

@addisoncrump
Copy link
Contributor Author

good to know: rebasing with GitHub creates unverified commits, very annoying

@addisoncrump
Copy link
Contributor Author

This PR is now ready to merge if CI checks pass.

@addisoncrump
Copy link
Contributor Author

Apologies for the issue. It seems my local brace expansion differs from CI; I have made this explicit, so this should now work as expected.

@DavidKorczynski
Copy link
Collaborator

Apologies for the issue. It seems my local brace expansion differs from CI; I have made this explicit, so this should now work as expected.

Restarted the CI

@DavidKorczynski DavidKorczynski merged commit a9caa2d into google:master Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DavidKorczynski DavidKorczynski DavidKorczynski approved these changes

+1 more reviewer

@PhilipHazel PhilipHazel PhilipHazel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@addisoncrump @PhilipHazel @DavidKorczynski