threads / patch / 51669

patchgrep: under --debug, show whether PCRE JIT is enabled

Subject: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

## tl;dr

8 messages between Aug 18, 2019 and Aug 27, 2019. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Beat Bolli· Aug 18, 2019, 20:17 UTC · lore
This information is useful and not visible anywhere else, so show it.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>

--- This applies on top of 'ab/pcre-jit-fixes', currently in pu.

 grep.c | 7 +++++++
 1 file changed, 7 insertions(+)
Show changes to grep.c +7 −0
diff --git a/grep.c b/grep.c
index 9bc589720b..96272b3cfc 100644
--- a/grep.c
+++ b/grep.c
@@ -433,6 +433,8 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
 
 #ifdef GIT_PCRE1_USE_JIT
 	pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);
+	if (opt->debug)
+		fprintf(stderr, "pcre1_jit_on=%d\n", p->pcre1_jit_on);
 #endif
 }
 
@@ -534,6 +536,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	}
 
 	pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
+	if (opt->debug)
+		fprintf(stderr, "pcre2_jit_on=%d\n", p->pcre2_jit_on);
 	if (p->pcre2_jit_on) {
 		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
 		if (jitret)
@@ -559,6 +563,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			BUG("pcre2_pattern_info() failed: %d", patinforet);
 		if (jitsizearg == 0) {
 			p->pcre2_jit_on = 0;
+			if (opt->debug)
+				fprintf(stderr, "pcre2_jit_on=%d: (*NO_JIT) in regex\n",
+					p->pcre2_jit_on);
 			return;
 		}
 	}

-- 
2.21.0.1020.gf2820cf01a
Junio C Hamano· Aug 19, 2019, 22:23 UTC · re: Beat Bolli · lore

Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

Beat Bolli <dev+git@drbeat.li> writes:
Show 7 quoted lines
> This information is useful and not visible anywhere else, so show it.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> ---
> This applies on top of 'ab/pcre-jit-fixes', currently in pu.
Thanks.

We saw a few people exchanging patches on the list and discussing topics aroud PCRE and especially JIT, but the discussion petered out during the prerelease freeze. I'd like to see the topics solidify early in this cycle.

IIRC, some key points/issues addressed by various patches we saw during the last cycle were:

 - A binary may be prepared to be JIT capable, but on a particular
   system (e.g. SELinux) JIT may not work.  Should we write off such
   a configuration as "broken"?  Should we just fall back on non-JIT?
   Should we fall back with loud warning?
 - JIT and non-JIT codepath may validate UTF-8 differently without
   care, but we should make sure JIT codepath behave identically to
   non JIT (only faster).
 - We should not be validating strict UTF-8 when we do not even know
   if the payload is UTF-8.  What mechanism, if any, do we have to
   let us say "this must be UTF-8 or otherwise it is an error" with
   confidence?  Should we error out in the middle of "git log"
   session upon seeing a binary haystack while looking for UTF-8
   needle (I think not)?
There may be others I am missing.

Is ab/pcre-jit-fixes a good base to collectively work on to finish the topics floated around PCRE during the last cycle?

I'll queue this debugging aid on top in the meantime.  Thanks.
Carlo Arenas· Aug 24, 2019, 12:57 UTC · re: Junio C Hamano · lore

Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

On Mon, Aug 19, 2019 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> There may be others I am missing.

should we still support PCRE1? I think in this case the problem is compounded by the fact that unless we do something like [1], the real fix for those UTF-8 validation issues will require a yet unreleased version of PCRE2 and will never be available for PCRE1, making the user experience suboptimal.

and explained in [1] there was a series to cleanup (both for maintainability and to mitigate regressions) the PCRE1 code that is yet to be formally reviewed in [2]

there is also the question of if we should provide knobs so users can "tune" their pcre library to workaround some of the quirks or if we should do more work ourselves to handle those quirks and improve the error reporting.

one example of that is as you pointed out JIT, but also applies to other things like PCRE's stack size, or depending on our solution for PCRE1, accepting the risk (which already exist anyway) to accept problems with matching because of corrupted UTF-8 in the haystack

> Is ab/pcre-jit-fixes a good base to collectively work on to finish
> the topics floated around PCRE during the last cycle?

V3 of that (which was never sent) might be better IMHO, I had to also admit I was surprised to see the whole no-kwset series this depended on being dropped but would seem it was just partially merged with pcre-jit-fixes (which is missing the patches that address the UTF-8 issues with PCRE2's unreleased flag and that should be part of that V3)

it might be worth also rebasing pcre2-chartables-leakfix on top of this to avoid conflicts, eventhough I had to admit that I was expanding on integrating [3], to avoid having to squash a fix into René's patch (as he suggested) and that would be part of a reroll from that series.

Carlo

[1] https://public-inbox.org/git/CAPUEspgStVxL=0SoAg82vxRMRGLSEKdHrT-xq6nCW1sNq7nLsw@mail.gmail.com/ [2] https://public-inbox.org/git/20190726202642.7986-1-carenas@gmail.com/ [3] https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/

Johannes Schindelin· Aug 26, 2019, 14:28 UTC · re: Carlo Arenas · lore

Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

Hi Carlo,
On Sat, 24 Aug 2019, Carlo Arenas wrote:
Show 5 quoted lines
> On Mon, Aug 19, 2019 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > There may be others I am missing.
>
> should we still support PCRE1?

While Git for Windows has no problem to just drop PCRE1 support, I would like to take a longer road in Git. Like, deprecate it first (I don't know how to do that effectively, though, as packagers usually ignore compile warnings, maybe we need to add a Makefile knob YES_I_WANT_TO_BUILD_WITH_PCRE1_INSTEAD_OF_PCRE2 and YES_I_WILL_STATE_MY_REASONS_ON_THE_GIT_MAILING_LIST or something like that).

Ciao, Dscho

Carlo Arenas· Aug 26, 2019, 14:42 UTC · re: Johannes Schindelin · lore

Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

On Mon, Aug 26, 2019 at 7:29 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 10 quoted lines
>
> On Sat, 24 Aug 2019, Carlo Arenas wrote:
>
> > On Mon, Aug 19, 2019 at 3:23 PM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > There may be others I am missing.
> >
> > should we still support PCRE1?
>
> While Git for Windows has no problem to just drop PCRE1 support,

FWIW I don't want to drop PCRE1 support, I was not advocating for it, but in the contrary trying to find a way to keep it working as best as possible until we really can't.

Show 7 quoted lines
> I would
> like to take a longer road in Git. Like, deprecate it first (I don't
> know how to do that effectively, though, as packagers usually ignore
> compile warnings, maybe we need to add a Makefile knob
> YES_I_WANT_TO_BUILD_WITH_PCRE1_INSTEAD_OF_PCRE2 and
> YES_I_WILL_STATE_MY_REASONS_ON_THE_GIT_MAILING_LIST or something like
> that).

FWIW e6c531b808 (Makefile: make USE_LIBPCRE=YesPlease mean v2, not v1, 2018-03-11) put a "deprecattion" warning in the Makefile by Ævar, but last time this was discussed[1] Junio made an IMHO sound argument for why that should be removed instead but ab/pcre-jit-fixes and UTF-8 validation are likely to make that more difficult (even if it is a mostly self inflicted wound AFAIK)

Carlo
[1] https://public-inbox.org/git/xmqqlg4xkh28.fsf@gitster-ct.c.googlers.com/
Junio C Hamano· Aug 26, 2019, 16:02 UTC · re: Carlo Arenas · lore

Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

Carlo Arenas <carenas@gmail.com> writes:
> ... but
> ab/pcre-jit-fixes and UTF-8 validation are likely to make that more
> difficult (even if it is a mostly self inflicted wound AFAIK)

Hmm, in what way? Do you mean that we'd be invested even more in pcre1 in an effort to keep supporting, that the sunk cost would dissuade us from deprecating the support even more, or something?

Carlo Arenas· Aug 26, 2019, 16:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

On Mon, Aug 26, 2019 at 9:02 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
>
> Carlo Arenas <carenas@gmail.com> writes:
>
> > ... but
> > ab/pcre-jit-fixes and UTF-8 validation are likely to make that more
> > difficult (even if it is a mostly self inflicted wound AFAIK)
>
> Hmm, in what way?  Do you mean that we'd be invested even more in
> pcre1 in an effort to keep supporting, that the sunk cost would
> dissuade us from deprecating the support even more, or something?

on the contrary, PCRE1 works fine but our recent changes make it worst unnecessarily (IMHO)

 for example 685668faaa (grep: stop using a custom JIT stack with PCRE
v1, 2019-07-26) adds 2 regressions as discussed in [1]
* git grep -P will now throw an error if there are non UTF-8 documents
in the haystack (even if JIT is available)
* git grep -P '^([/](?!/)|[^/])*~/.*' will now fail with a cryptic
PCRE error instead of succeeding (but at least will be consistent and
show the same error with PCRE2)
Carlo
[1] https://public-inbox.org/git/CAPUEspgStVxL=0SoAg82vxRMRGLSEKdHrT-xq6nCW1sNq7nLsw@mail.gmail.com/
Junio C Hamano· Aug 27, 2019, 19:43 UTC · re: Carlo Arenas · lore

Re: [PATCH] grep: under --debug, show whether PCRE JIT is enabled

Carlo Arenas <carenas@gmail.com> writes:
Show 12 quoted lines
>> > ... but
>> > ab/pcre-jit-fixes and UTF-8 validation are likely to make that more
>> > difficult (even if it is a mostly self inflicted wound AFAIK)
>> ...
>  for example 685668faaa (grep: stop using a custom JIT stack with PCRE
> v1, 2019-07-26) adds 2 regressions as discussed in [1]
>
> * git grep -P will now throw an error if there are non UTF-8 documents
> in the haystack (even if JIT is available)
> * git grep -P '^([/](?!/)|[^/])*~/.*' will now fail with a cryptic
> PCRE error instead of succeeding (but at least will be consistent and
> show the same error with PCRE2)

Thanks. This was exactly the kind of thing I wanted to hear when I asked if ab/pcre-jit-fixes is a good base to further build on.

As far as I am concerned, ab/pcre-jit-fixes and anything pcre that are still in flight are fair game for a reboot during this cycle (I hope I did not merge any of them to 'next' by mistake yet). If there are bad apples queued on 'pu', let's make sure we eject them and rethink the way to address the issue(s) they wanted to address.

← back to recent threads