* [PATCH 0/2] PCRE1 cleanup @ 2019-08-25 18:22 Carlo Marcelo Arenas Belón 2019-08-25 18:22 ` [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 Carlo Marcelo Arenas Belón 2019-08-25 18:22 ` [PATCH 2/2] grep: refactor and simplify PCRE1 support Carlo Marcelo Arenas Belón 0 siblings, 2 replies; 6+ messages in thread From: Carlo Marcelo Arenas Belón @ 2019-08-25 18:22 UTC (permalink / raw) To: git; +Cc: cbailey32, avarab, gitster This is a reroll of [1] which is mostly equivalent to the original RFC but rebased on top of ab/pcre-jit-fixes. The first patch fixes an inconsistency from 685668faaa (grep: stop using a custom JIT stack with PCRE v1, 2019-07-26) where NO_LIBPCRE1_JIT will only affect versions of pcre1 >= 8.32, while support for JIT was added to pcre1 with version 8.20. Technically this is a change of behaviour as originally it was not possible to use JIT with those older versions, but the restriction was somehow arbitrary and caused by the use of JIT fast path. The second patch is mainly refactoring and to make sure the solution from 2fff1e196d (grep: fix NO_LIBPCRE1_JIT to fully disable JIT, 2017-11-12) is working as expected. Carlo Marcelo Arenas Belón (2): grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 grep: refactor and simplify PCRE1 support Makefile | 9 ++------- grep.c | 16 ++++++++++------ grep.h | 11 ----------- 3 files changed, 12 insertions(+), 24 deletions(-) [1] https://public-inbox.org/git/20190726202642.7986-1-carenas@gmail.com/ base-commit: c581e4a7499b9e1089847dbbc057afbef1ed861e -- 2.23.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 2019-08-25 18:22 [PATCH 0/2] PCRE1 cleanup Carlo Marcelo Arenas Belón @ 2019-08-25 18:22 ` Carlo Marcelo Arenas Belón 2019-08-26 18:54 ` Junio C Hamano 2019-08-25 18:22 ` [PATCH 2/2] grep: refactor and simplify PCRE1 support Carlo Marcelo Arenas Belón 1 sibling, 1 reply; 6+ messages in thread From: Carlo Marcelo Arenas Belón @ 2019-08-25 18:22 UTC (permalink / raw) To: git; +Cc: cbailey32, avarab, gitster e87de7cab4 ("grep: un-break building with PCRE < 8.32", 2017-05-25) added a restriction for JIT support that is no longer needed after pcre_jit_exec() calls were removed. Reorganize the definitions in grep.h so that JIT support could be detected early and NO_LIBPCRE1_JIT could be used reliably to enforce JIT doesn't get used. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Makefile | 9 ++------- grep.h | 4 +--- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index f58bf14c7b..3f78ef942f 100644 --- a/Makefile +++ b/Makefile @@ -34,13 +34,8 @@ all:: # library. Support for version 1 will likely be removed in some future # release of Git, as upstream has all but abandoned it. # -# When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1 -# library is compiled without --enable-jit. We will auto-detect -# whether the version of the PCRE v1 library in use has JIT support at -# all, but we unfortunately can't auto-detect whether JIT support -# hasn't been compiled in in an otherwise JIT-supporting version. If -# you have link-time errors about a missing `pcre_jit_exec` define -# this, or recompile PCRE v1 with --enable-jit. +# When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if you want to +# disable JIT even if supported by your library. # # Define LIBPCREDIR=/foo/bar if your PCRE header and library files are # in /foo/bar/include and /foo/bar/lib directories. Which version of diff --git a/grep.h b/grep.h index c0c71eb4a9..1a044c501e 100644 --- a/grep.h +++ b/grep.h @@ -3,14 +3,12 @@ #include "color.h" #ifdef USE_LIBPCRE1 #include <pcre.h> -#ifdef PCRE_CONFIG_JIT -#if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32 #ifndef NO_LIBPCRE1_JIT +#ifdef PCRE_CONFIG_JIT #define GIT_PCRE1_USE_JIT #define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE #endif #endif -#endif #ifndef GIT_PCRE_STUDY_JIT_COMPILE #define GIT_PCRE_STUDY_JIT_COMPILE 0 #endif -- 2.23.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 2019-08-25 18:22 ` [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 Carlo Marcelo Arenas Belón @ 2019-08-26 18:54 ` Junio C Hamano 2019-08-27 1:34 ` Carlo Arenas 0 siblings, 1 reply; 6+ messages in thread From: Junio C Hamano @ 2019-08-26 18:54 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, cbailey32, avarab Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > e87de7cab4 ("grep: un-break building with PCRE < 8.32", 2017-05-25) > added a restriction for JIT support that is no longer needed after > pcre_jit_exec() calls were removed. I was initially puzzled by this statement, until I realized that the removal of pcre_jit_exec() happens in the topic still in flight that this patch builds on top of, namely 685668fa ("grep: stop using a custom JIT stack with PCRE v1", 2019-07-26). So the logic is that because we do no longer call pcre_jit_exec() that weren't available between 8.20 and 8.32, these slightly older versions can now do JIT just like the ones post 8.32? Thanks. Queued. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 2019-08-26 18:54 ` Junio C Hamano @ 2019-08-27 1:34 ` Carlo Arenas 0 siblings, 0 replies; 6+ messages in thread From: Carlo Arenas @ 2019-08-27 1:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, cbailey32, avarab On Mon, Aug 26, 2019 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > e87de7cab4 ("grep: un-break building with PCRE < 8.32", 2017-05-25) > > added a restriction for JIT support that is no longer needed after > > pcre_jit_exec() calls were removed. > > I was initially puzzled by this statement, until I realized that the > removal of pcre_jit_exec() happens in the topic still in flight that > this patch builds on top of, namely 685668fa ("grep: stop using a > custom JIT stack with PCRE v1", 2019-07-26). sorry about that, I thought I had mentioned it in the cover letter (since the hash is likely to change and so is not fit for a commit message) but it is not there either. how could this be tracked more effectively? > So the logic is that because we do no longer call pcre_jit_exec() > that weren't available between 8.20 and 8.32, these slightly older > versions can now do JIT just like the ones post 8.32? exactly; but also because it is no longer using the JIT fast path which skipped UTF-8 validation, will need a way to disable that or risk a regression as I mentioned in [1] was planning in proposing a fix for PCRE1 based on [2] but wasn't sure if it could be part of this series, an independent one that is also based on ab/pcre-jit-fixes, and like this one, is mostly a consequence of 685668fa ("grep: stop using a custom JIT stack with PCRE v1", 2019-07-26) or something else, specially considering that Ævar dismissed it as a non issue in his commit message. Carlo [1] https://public-inbox.org/git/CAPUEspj4BJLjXorUXMiZnFtNcmhym_2QL5GUqeaGaCoxk=zjtw@mail.gmail.com/T/#m6acc8f68c398951457da469530bafa7e18811366 [2] https://public-inbox.org/git/20190721183115.14985-1-carenas@gmail.com/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] grep: refactor and simplify PCRE1 support 2019-08-25 18:22 [PATCH 0/2] PCRE1 cleanup Carlo Marcelo Arenas Belón 2019-08-25 18:22 ` [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 Carlo Marcelo Arenas Belón @ 2019-08-25 18:22 ` Carlo Marcelo Arenas Belón 2019-08-26 18:57 ` Junio C Hamano 1 sibling, 1 reply; 6+ messages in thread From: Carlo Marcelo Arenas Belón @ 2019-08-25 18:22 UTC (permalink / raw) To: git; +Cc: cbailey32, avarab, gitster The code used both a macro and a variable to keep track if JIT support was desired and relied on the fact that a non JIT enabled library will ignore a request for JIT compilation (as defined by the second parameter of the call to pcre_study) Cleanup the multiple levels of macros used and call pcre_study with the right parameter after JIT support has been confirmed and unless it was requested to be disabled with NO_LIBPCRE1_JIT Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- grep.c | 16 ++++++++++------ grep.h | 9 --------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/grep.c b/grep.c index 76088784e3..05d31c2bcc 100644 --- a/grep.c +++ b/grep.c @@ -374,6 +374,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt) const char *error; int erroffset; int options = PCRE_MULTILINE; + int study_options = 0; if (opt->ignore_case) { if (!opt->ignore_locale && has_non_ascii(p->pattern)) @@ -388,15 +389,18 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt) if (!p->pcre1_regexp) compile_regexp_failed(p, error); - p->pcre1_extra_info = pcre_study(p->pcre1_regexp, GIT_PCRE_STUDY_JIT_COMPILE, &error); - if (!p->pcre1_extra_info && error) - die("%s", error); - -#ifdef GIT_PCRE1_USE_JIT +#if defined(PCRE_CONFIG_JIT) && !defined(NO_LIBPCRE1_JIT) pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on); if (opt->debug) fprintf(stderr, "pcre1_jit_on=%d\n", p->pcre1_jit_on); + + if (p->pcre1_jit_on) + study_options = PCRE_STUDY_JIT_COMPILE; #endif + + p->pcre1_extra_info = pcre_study(p->pcre1_regexp, study_options, &error); + if (!p->pcre1_extra_info && error) + die("%s", error); } static int pcre1match(struct grep_pat *p, const char *line, const char *eol, @@ -425,7 +429,7 @@ static int pcre1match(struct grep_pat *p, const char *line, const char *eol, static void free_pcre1_regexp(struct grep_pat *p) { pcre_free(p->pcre1_regexp); -#ifdef GIT_PCRE1_USE_JIT +#ifdef PCRE_CONFIG_JIT if (p->pcre1_jit_on) pcre_free_study(p->pcre1_extra_info); else diff --git a/grep.h b/grep.h index 1a044c501e..ff620d784a 100644 --- a/grep.h +++ b/grep.h @@ -3,15 +3,6 @@ #include "color.h" #ifdef USE_LIBPCRE1 #include <pcre.h> -#ifndef NO_LIBPCRE1_JIT -#ifdef PCRE_CONFIG_JIT -#define GIT_PCRE1_USE_JIT -#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE -#endif -#endif -#ifndef GIT_PCRE_STUDY_JIT_COMPILE -#define GIT_PCRE_STUDY_JIT_COMPILE 0 -#endif #else typedef int pcre; typedef int pcre_extra; -- 2.23.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] grep: refactor and simplify PCRE1 support 2019-08-25 18:22 ` [PATCH 2/2] grep: refactor and simplify PCRE1 support Carlo Marcelo Arenas Belón @ 2019-08-26 18:57 ` Junio C Hamano 0 siblings, 0 replies; 6+ messages in thread From: Junio C Hamano @ 2019-08-26 18:57 UTC (permalink / raw) To: Carlo Marcelo Arenas Belón; +Cc: git, cbailey32, avarab Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > diff --git a/grep.h b/grep.h > index 1a044c501e..ff620d784a 100644 > --- a/grep.h > +++ b/grep.h > @@ -3,15 +3,6 @@ > #include "color.h" > #ifdef USE_LIBPCRE1 > #include <pcre.h> > -#ifndef NO_LIBPCRE1_JIT > -#ifdef PCRE_CONFIG_JIT > -#define GIT_PCRE1_USE_JIT > -#define GIT_PCRE_STUDY_JIT_COMPILE PCRE_STUDY_JIT_COMPILE > -#endif > -#endif > -#ifndef GIT_PCRE_STUDY_JIT_COMPILE > -#define GIT_PCRE_STUDY_JIT_COMPILE 0 > -#endif > #else > typedef int pcre; > typedef int pcre_extra; That's a patch pleasant to read ;-) Will queue. Thansk. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-27 1:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-25 18:22 [PATCH 0/2] PCRE1 cleanup Carlo Marcelo Arenas Belón 2019-08-25 18:22 ` [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1 Carlo Marcelo Arenas Belón 2019-08-26 18:54 ` Junio C Hamano 2019-08-27 1:34 ` Carlo Arenas 2019-08-25 18:22 ` [PATCH 2/2] grep: refactor and simplify PCRE1 support Carlo Marcelo Arenas Belón 2019-08-26 18:57 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).