[PATCH 0/2] PCRE1 cleanup

[PATCH 0/2] PCRE1 cleanup

[Carlo Marcelo Arenas Belón]

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

[PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1

[Carlo Marcelo Arenas Belón]

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

[PATCH 2/2] grep: refactor and simplify PCRE1 support

[Carlo Marcelo Arenas Belón]

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

Re: [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1

[Junio C Hamano]

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.

Re: [PATCH 2/2] grep: refactor and simplify PCRE1 support

[Junio C Hamano]

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.

Re: [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1

[Carlo Arenas]

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/