patchgrep: use custom JIT stack with pcre2

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) allocate a stack and assign it to a match context, but never pass it to pcre2_jit_match, using instead the default.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
This might have positive performance consequences (per the comments)
but haven't tested them; if there is no difference might be better
instead to remove the stack and match_context and save the related
memory, since it seems the default was working fine anyway.

 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

There's a couple of patches fixing mistakes in the JIT code I added for PCRE in <20190722181923.21572-1-dev+git@drbeat.li> and <20190721194052.15440-1-carenas@gmail.com>

This small series proposes to replace both of those. In both cases I think we're better off just removing the relevant code. The commit messages for the patches themselves make the case for that.

Ævar Arnfjörð Bjarmason (3):
  grep: remove overly paranoid BUG(...) code
  grep: stop "using" a custom JIT stack with PCRE v2
  grep: stop using a custom JIT stack with PCRE v1

 grep.c | 46 ++++++----------------------------------------
 grep.h |  9 ---------
 2 files changed, 6 insertions(+), 49 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I am not sure about the BUG() that practically never triggered so far (AFAICT, the check that guards the BUG() would trigger only if we later introduced a bug, calling the code to compile when we are not asked to do so)---wouldn't it be better to leave it in while there still are people who are touching the vicinity?

The other two I am perfectly OK with. It is easy to resurrect the support for v1 (which may not even be needed for long) and resurrect the support for v2 with Carlo's fix, if it later turns out that some users may need to use a more complex pattern.

Thanks.

On Wed, Jul 24 2019, Junio C Hamano wrote:

The BUG() in 1/3 is just checking if pcre2?_config() returns a boolean when promised, so it amounts to black-box testing of that library.

I think code in that style is overly paranoid and verbose, it's reasonable to just trust the library in that case.

I think the reason it ended up in the codebase in the first place was converting some first-draft implementation I wrote where I was being more paranoid about using the PCRE API as a black box.

Remove code that would trigger if pcre_config() or pcre2_config() was so broken that "do we have JIT?" wouldn't return a boolean.

I added this code back in fbaceaac47 ("grep: add support for the PCRE v1 JIT API", 2017-05-25) and then as noted in f002532784 ("grep: print the pcre2_jit_on value", 2019-07-22) incorrectly copy/pasted some of it in 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01).

Let's just remove this code. Being this paranoid about the pcre2?_config() function itself being broken is crossing the line into unreasonable paranoia.

Reported-by:  Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

As reported in [1] the code I added in 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) to use a custom JIT stack has never worked. It was incorrectly copy/pasted from code I added in fbaceaac47 ("grep: add support for the PCRE v1 JIT API", 2017-05-25), which did work.

Thus our intention of starting with 1 byte of stack at a maximum of 1 MB didn't happen, we'd always use the 32 KB stack provided by PCRE v2's jit_machine_stack_exec()[2]. The reason I allocated a custom stack at all was this advice in pcrejit(3) (same in pcre2jit(3)):

    "By default, it uses 32KiB on the machine stack. However, some
    large or complicated patterns need more than this"

Since we've haven't had any reports of users running into PCRE2_ERROR_JIT_STACKLIMIT in the wild I think we can safely assume that we can just use the library defaults instead and drop this code. This won't change with the wider use of PCRE v2 in ed0479ce3d ("Merge branch 'ab/no-kwset' into next", 2019-07-15), a fixed string search is not a "large or complicated pattern".

For good measure I ran the performance test noted in 94da9193a6, although the command is simpler now due to my 0f50c8e32c ("Makefile: remove the NO_R_TO_GCC_LINKER flag", 2019-05-17):

    GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE2=Y CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst' ./run HEAD~ HEAD p7820-grep-engines.sh

Just the /perl/ results are:

    Test                                            HEAD~             HEAD
    ---------------------------------------------------------------------------------------
    7820.3: perl grep 'how.to'                      0.17(0.27+0.65)   0.17(0.24+0.68) +0.0%
    7820.7: perl grep '^how to'                     0.16(0.23+0.66)   0.16(0.23+0.67) +0.0%
    7820.11: perl grep '[how] to'                   0.18(0.35+0.62)   0.18(0.33+0.65) +0.0%
    7820.15: perl grep '(e.t[^ ]*|v.ry) rare'       0.17(0.45+0.54)   0.17(0.49+0.50) +0.0%
    7820.19: perl grep 'm(ú|u)lt.b(æ|y)te'          0.16(0.33+0.58)   0.16(0.29+0.62) +0.0%

So, as expected there's no change, and running with valgrind reveals that we have fewer allocations now.

As noted in [3] there are known regexes that will fail with the lower stack limit, the way GNU grep fixed it is interesting, although I believe the implementation is overly verbose, they could make PCRE v2 handle that gradual re-allocation, that's what min/max memory is for.

So we might end up bringing this back, I'm more inclined to just kick such cases upstairs to PCRE maintainers as a bug, perhaps they'll add some overall "just allocate more then" flag to make this easier. In any case there's no functional change here, we didn't have a custom stack, so let's apply this first, we can always revert it later.

1. https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/
2. I didn't really intend to start with 1 byte, looking at the PCRE v2
   code again what happened is that I cargo-culted some of PCRE v2's
   own test code which was meant to test re-allocations. It's more
   sane to start with say 32 KB with a max of 1 MB, as pcre2grep.c
   does.
3. https://public-inbox.org/git/CAPUEspjj+fG8QDmf=bZXktfpLgkgiu34HTjKLhm-cmEE04FE-A@mail.gmail.com/

Reported-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 10 ----------
 grep.h |  4 ----
 2 files changed, 14 deletions(-)

On Fri, Jul 26, 2019 at 8:08 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

The part I liked about the grep implementation was how they went above and beyond to make sure there was no abstraction leak and at the end the end user doesn't even see a PCRE error message.

Presume thought that the end user we have is different, and might make sense to expose them to the underlying mechanism, but in that case we should also provide them with knobs to tweak (like the one I proposed to disable jit, and that in this case might be to set a stacksize)

agree, LGTM other than by the comment below

this return and brackets no longer needed

Carlo

Simplify the PCRE v1 code for the same reasons as for the PCRE v2 code in the last commit. Unlike with v2 we actually used the custom stack in v1, but let's use PCRE's built-in 32 KB one instead, since experience with v2 shows that's enough. Most distros are already using v2 as a default, and the underlying sljit code is the same.

Unfortunately we can't just pass a NULL to pcre_jit_exec() as with pcre2_jit_match(). Unlike the v2 function it doesn't support that. Instead we need to use the fatter pcre_exec() if we'd like the same behavior.

This will make things slightly slower than on the fast-path function, but it's OK since we care less about v1 performance these days since we have and recommend v2. Running a similar performance test as what I ran in fbaceaac47 ("grep: add support for the PCRE v1 JIT API", 2017-05-25) via:

    GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE1=Y CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst' ./run HEAD~ HEAD p7820-grep-engines.sh

Gives us this, just the /perl/ results:

    Test                                            HEAD~             HEAD
    ---------------------------------------------------------------------------------------
    7820.3: perl grep 'how.to'                      0.19(0.67+0.52)   0.19(0.65+0.52) +0.0%
    7820.7: perl grep '^how to'                     0.19(0.78+0.44)   0.19(0.72+0.49) +0.0%
    7820.11: perl grep '[how] to'                   0.39(2.13+0.43)   0.40(2.10+0.46) +2.6%
    7820.15: perl grep '(e.t[^ ]*|v.ry) rare'       0.44(2.55+0.37)   0.45(2.47+0.41) +2.3%
    7820.19: perl grep 'm(ú|u)lt.b(æ|y)te'          0.23(1.06+0.42)   0.22(1.03+0.43) -4.3%

It will also implicitly re-enable UTF-8 validation for PCRE v1. As noted in [1] we now have cases as a result where PCRE v1 is more eager to error out. Subsequent patches will fix that for v2, and I think it's fair to tell v1 users "just upgrade" and not worry about that edge case for v1.

1.  https://public-inbox.org/git/CAPUEsphZJ_Uv9o1-yDpjNLA_q-f7gWXz9g1gCY2pYAYN8ri40g@mail.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 28 +++++-----------------------
 grep.h |  5 -----
 2 files changed, 5 insertions(+), 28 deletions(-)

On Fri, Jul 26, 2019 at 8:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Haven't seen any responses from packagers but there was a report[1] of CentOS 6 users that would be affected, and would certainly make things more difficult to whoever is behind the git binary that comes with Xcode in macOS and that is linked against a system library (PCRE1) that is IMHO unlikely to be upgraded.

The minimum I would expect if you want to move forward with this, would be to make their git not randomly die because of non UTF-8 haystack by applying [2] and make clear we will also introduce stack size problems like the one in [3] (as it was done in the previous commit)

Feedback on the patchset[4] that applies on top of this to make sure JIT can be disabled at compile time and the logic is less messy also appreciated.

Let me know also how you want to keep it on sync, as IMHO makes more sense inside your branch instead of as an independent topic.

Carlo

CC +Brian

[1] https://public-inbox.org/git/20190615191514.GD8616@genre.crustytoothpaste.net/ [2] https://public-inbox.org/git/20190722144350.46458-1-carenas@gmail.com/ [3] https://public-inbox.org/git/CAPUEspjj+fG8QDmf=bZXktfpLgkgiu34HTjKLhm-cmEE04FE-A@mail.gmail.com/ [4] https://public-inbox.org/git/20190726202642.7986-1-carenas@gmail.com/

At the start of this function we do:

    p->fixed = opt->fixed;

It's less confusing to use that variable consistently that switch back & forth between the two.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Fri, Jul 26, 2019 at 8:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

at the end of this series we have:

  if (p->fixed || p->is_fixed)

which doesn't make sense; at least with opt->fixed it was clear that what was meant is that grep was passed -P

maybe is_fixed shouldn't exist and fixed when applied to the pattern means we had determined it was a fixed pattern and overridden the user selection of engine.

that at least will give us a logical way to fix the pattern reported in [1] and that currently requires the user to know git's grep internals and know he can skip the "is_fixed" optimization by doing something like :

  $ git grep 'foo[ ]bar'

Carlo

[1] https://public-inbox.org/git/20190728235427.41425-1-carenas@gmail.com/

On Mon, Jul 29 2019, Carlo Arenas wrote:

I assume you mean "was passed -F...".

They're two flags because p->fixed is "--fixed-strings", and p->is_fixed is "there's no metachars here". So the former case needs escaping, as the code just below might do (the two aren't mutually exclusive).

I don't get how you think we can always fold them into one flag, but maybe I'm missing something...

As I noted in a reply there this seems like a way to fix a bug in "next" with a config knob. Yes we should fix the bug, but we've had the kwset code in git for years without needing this distinction, so after we work out the bugs I don't see why we'd need this.

The reason we ignore the user's choice here is because you might e.g. set grep.patternType=extended in your config, and you'd still want grepping for a fixed "foo" to be fast.

On Mon, Jul 29 2019, Ævar Arnfjörð Bjarmason wrote:

...and more generally, for any future sanity of implementation and maintenance I think we should only make the promise that we support certain syntax & semantics, not that the -F, -G, -E, -P options are guaranteed to dispatch to a given codepath.

Internally we should be free to switch between those, so e.g. if a pattern is fixed and you configure "basic" regexp, but we know your C library is faster for those matches with REG_EXTENDED we should just pass that regardless of -G or -E.

Of course that means we *must* expose the same semantics (to some reasonable extent), which means I have a lot of bugs in "next" to address.

I'm just saying that the presence of those bugs means we should be inclined to fix them / back out certain changes, not work around them with user-servicable knobs.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

That certainly is a very sensible goal.

Amen.

This change paves the way for later using this value the regex compile functions themselves.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 7 +++----
 grep.h | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

As discussed in the last commit partially fix a bug introduced in b65abcafc7 ("grep: use PCRE v2 for optimized fixed-string search", 2019-07-01). Because PCRE v2, unlike kwset, validates its UTF-8 input we'd die on e.g.:

    fatal: pcre2_match failed with error code -22: UTF-8 error:
    isolated byte with 0x80 bit set

When grepping a non-ASCII fixed string. This is a more general problem that's hard to fix, but we can at least fix the most common case of grepping for a fixed string without "-i". I can't think of a reason for why we'd turn on PCRE2_UTF when matching byte-for-byte like that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c                          | 3 ++-
 t/t7812-grep-icase-non-ascii.sh | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Yes, exactly. That's quite a sane and minimum fix/workaround, I would think.

Since my b65abcafc7 ("grep: use PCRE v2 for optimized fixed-string search", 2019-07-01) we've been dying on invalid UTF-8 data when grepping for fixed strings if the following are all true:

    * The subject string is non-ASCII (e.g. "ævar")
    * We're under a is_utf8_locale(), e.g. "en_US.UTF-8", not "C"
    * We compiled with PCRE v2
    * That PCRE v2 did not have JIT support

The last of those is why this wasn't caught earlier, per pcre2jit(3):

    "unless PCRE2_NO_UTF_CHECK is set, a UTF subject string is tested
    for validity. In the interests of speed, these checks do not
    happen on the JIT fast path, and if invalid data is passed, the
    result is undefined."

I.e. the subject being matched against our pattern was invalid, but we were lucky and getting away with it on the JIT path, but the non-JIT one is stricter.

This patch does nothing to fix that, instead we sneak in support for fixed patterns starting with "(*NO_JIT)", this disables the PCRE v2 jit with implicit fixed-string matching for testing, see pcre2syntax(3) the syntax.

This is technically a change in behavior, but it's so obscure that I figured it was OK. We'd previously consider this an invalid regular expression as regcomp() would die on it, now we feed it to the PCRE v2 fixed-string path. I thought this was better than introducing yet another GIT_TEST_* environment variable.

We're also relying on a behavior of PCRE v2 that technically could change, but I think the test coverage is worth dipping our toe into some somewhat undefined behavior.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c                          | 10 ++++++++++
 t/t7812-grep-icase-non-ascii.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

It is unfortunate that is_fixed() takes a counted string. Otherwise, using skip_prefix() to avoid "+no_jit_len" would have made it much easier to read. i.e.

	/* an illustration that does not quite work */
	char *pattern_body;
	if (skip_prefix(p->pattern, "(*NO_JIT)", &pattern_body) &&
            is_fixed(pattern_body))
		p->is_fixed = 1;
	

On Fri, Jul 26 2019, Junio C Hamano wrote:

Indeed, but then we couldn't use this for patterns that have NUL in them, which we otherwise support (and support here). So I think it's worth keeping it so it takes ptr+len.

On Fri, Jul 26, 2019 at 8:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Alternativelly; using `git -c pcre.jit=false grep ...` on top of [1], might be cleaner

Carlo

[1] https://public-inbox.org/git/20190728235427.41425-1-carenas@gmail.com/

Two tests in t7812 were missing redirects, failing to actually test the produced output.

Fixes: 8a5999838e ("grep: stess test PCRE v2 on invalid UTF-8 data")
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
---
 t/t7812-grep-icase-non-ascii.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Hi,

On Tue, 26 Nov 2019, Andreas Schwab wrote:

Apart from this line which is cuddled with a real footer (but is no footer, and the commit reference is not in the recommended format either because it lacks the date), this patch looks fine to me.

Ciao, Johannes

On Nov 26 2019, Johannes Schindelin wrote:

Where is that documented?

Andreas.

On Wed, Nov 27, 2019 at 12:11:49AM +0100, Andreas Schwab wrote:

It's mentioned as the preferred way to reference commits in SubmittingPatches (search for %ad).

But I don't see why it is "not a footer". The "Fixes:" key conforms to the trailer syntax, and the value of a trailer is free-form. Running:

  git log --format='%(trailers:key=Fixes)'

shows that Git is happy with it. And indeed, a few other people have used it before you. None of them with a date. ;)

-Peff

When the 'grep with invalid UTF-8 data' tests were added/adjusted in 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26) and 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching, 2019-07-26) they lacked a redirect which caused them to falsely succeed on most architectures. They failed on big-endian arches where the test never reached the portion which was missing the redirect.

A recent patch add the missing redirect and exposed the fact that the 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails on all architectures.

Based on the final paragraph in in 870eea8166:

    When grepping a non-ASCII fixed string. This is a more general problem
    that's hard to fix, but we can at least fix the most common case of
    grepping for a fixed string without "-i". I can't think of a reason
    for why we'd turn on PCRE2_UTF when matching byte-for-byte like that.

it seems that we don't expect that the case-insensitive grep will succeed. Adjust the test to reflect that expectation.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

Hi,

Andreas Schwab wrote:

Nice catch on these missing redirects. While testing the 2.24.0 rc's this test failed only on big-endian arches (like s390x in Fedora). It was briefly mentioned at the time in <20191020002648.GZ10893@pobox.com>.

After applying the fix to add the missing redirects, the test now fails on all architectures. It seems like that is the intended state and we simply need to adjust the final test with `grep -i` accordingly.

Of course, it's possible that I've misunderstood Ævar's intentions in 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching, 2019-07-26) or otherwise have poorly explained the reasoning in my commit message.

We don't appear to test PCRE (neither v1 nor v2) in our CI builds. I added libpcre2-dev and set USE_LIBPCRE to test this in travis. (Whether we want to enable that by default is worth discussing.)

Doing so confirms that the tests (incorrectly) pass without the missing redirects and fail when the redirects are added. Inverting the expectation of test_cmp in the final `grep -i` test results in a successful test run.

with PCRE2: https://travis-ci.org/tmzullinger/git/jobs/618733182

with PCRE2 and 'add missing redirects': https://travis-ci.org/tmzullinger/git/jobs/618723277

with PCRE2, 'add missing redirects' and this patch: https://travis-ci.org/tmzullinger/git/jobs/618796963

This still doesn't fix the failure on s390x.  There

    test_might_fail git grep -hi "Æ" invalid-0x80 >actual

fails and leaves nothing in 'actual' which causes the test to fail at the following

    test_cmp expected actual

line. If we suspect the test might fail, it seems like we need to account for that and not expect that 'expect' will match 'actual' as we currently do.

 t/t7812-grep-icase-non-ascii.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

On Nov 29 2019, Todd Zullinger wrote:

It's not about big vs little endian, it's only about JIT vs non-JIT.

Andreas.

Andreas Schwab <schwab@linux-m68k.org> writes:

So, which one of JIT / non-JIT sides did the test fail unexpectedly?

Should I do s/on big-endian arches/with PCRE with JIT disabled/ while queuing the patch?

Thanks.

On Dez 01 2019, Junio C Hamano wrote:

The non-JIT was the one that wasn't actually tested.

Right.

Andreas.

Hi,

Junio C Hamano wrote:

On s390x, the initial:

    test_might_fail git grep -hi "Æ" invalid-0x80 >actual

fails to produce any output in actual, but since we use test_might_fail, the test happily continues to:

    test_cmp expected actual

which fails.

The test output from and s390x build:

    expecting success of 7812.11 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i':
        test_might_fail git grep -hi "Æ" invalid-0x80 >actual &&
        test_cmp expected actual &&
        test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 &&
        test_cmp expected actual
    ++ test_might_fail git grep -hi Æ invalid-0x80
    ++ test_must_fail ok=success git grep -hi Æ invalid-0x80
    ++ case "$1" in
    ++ _test_ok=success
    ++ shift
    ++ git grep -hi Æ invalid-0x80
    fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set
    ++ exit_code=128
    ++ test 128 -eq 0
    ++ test_match_signal 13 128
    ++ test 128 = 141
    ++ test 128 = 269
    ++ return 1
    ++ test 128 -gt 129
    ++ test 128 -eq 127
    ++ test 128 -eq 126
    ++ return 0
    ++ test_cmp expected actual
    ++ diff -u expected actual
    --- expected    2019-10-19 21:56:08.634252012 +0000
    +++ actual      2019-10-19 21:56:08.714252012 +0000
    @@ -1 +0,0 @@
    -ævar
    error: last command exited with $?=1
    not ok 11 - PCRE v2: grep non-ASCII from invalid UTF-8 data with -i
    #
    #               test_might_fail git grep -hi "Æ" invalid-0x80 >actual &&
    #               test_cmp expected actual &&
    #               test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 &&
    #               test_cmp expected actual
    #
    # failed 1 among 11 test(s)

After Andreas' missing redirect fix, that still fails on s390x (not surprisingly). But now systems with JIT enabled fail at:

    test_must_fail git grep -hi "(*NO_JIT)Æ" invalid-0x80 >actual &&
    test_cmp expected actual

Though we say that the command must fail, so we shouldn't be surprised that 'expect' and 'actual' don't match. It would be more surprising if they did. :)

Here's how I changed the commit message locally. I was going to wait a day or so for any other feedback on the actual test changes, being a holiday weekend in the US (and more generally a weekend).

1:  d9aeaf0c98 ! 1:  d0c083db78 t7812: expect failure for grep -i with invalid UTF-8 data
    @@ Commit message
         8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26)
         and 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,
         2019-07-26) they lacked a redirect which caused them to falsely succeed
    -    on most architectures.  They failed on big-endian arches where the test
    -    never reached the portion which was missing the redirect.
    +    on most systems.  The 'grep -i' test failed on systems where JIT was
    +    disabled as it never reached the portion which was missing the redirect.
     
    -    A recent patch add the missing redirect and exposed the fact that the
    -    'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails on
    -    all architectures.
    +    A recent patch added the missing redirect and exposed the fact that the
    +    'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' test fails
    +    regardless of whether JIT is enabled.
     
         Based on the final paragraph in in 870eea8166:

Thanks for pointing out the proper reasoning to use in the commit message Andreas. I hadn't looked at the Fedora pcre2 package to see that it explicitly disables JIT on s390x.

I'm not sure if s390x is supported upstream or not -- it doesn't appear to have a specific entry in the sljit config header¹, so it seems likely it's not well-tested at the least. (Not that any of that is our concern here.)

¹ https://github.com/zherczeg/sljit/blob/master/sljit_src/sljitConfigInternal.h

Thanks for the follow-up Junio.

Todd Zullinger <tmz@pobox.com> writes:

OK. I locally edited the log message to match the above. I guess this forms an integral part of a topic that Andreas started with the "missing redirects" fix, so let me queue your patch directly on the same topic branch, without creating a separate one.

Thanks, both.

1-3 here are a re-roll on "next". I figured that was easier for everyone with the state of the in-flight patches, it certainly was for me. Sorry Junio if this creates a mess for you.

4-8 are a "fix" for the UTF-8 matching error noted in Carlo's "grep: skip UTF8 checks explicitally" in https://public-inbox.org/git/20190721183115.14985-1-carenas@gmail.com/

As noted the bug isn't fully fixed until 8/8, and that patch relies on unreleased PCRE v2 code. I'm hoping that with 7/8 we're in a good enough state to limp forward as noted in the rationale of those commits.

Ævar Arnfjörð Bjarmason (8):
  grep: remove overly paranoid BUG(...) code
  grep: stop "using" a custom JIT stack with PCRE v2
  grep: stop using a custom JIT stack with PCRE v1
  grep: consistently use "p->fixed" in compile_regexp()
  grep: create a "is_fixed" member in "grep_pat"
  grep: stess test PCRE v2 on invalid UTF-8 data
  grep: do not enter PCRE2_UTF mode on fixed matching
  grep: optimistically use PCRE2_MATCH_INVALID_UTF

 Makefile                        |  1 +
 grep.c                          | 68 +++++++++++----------------------
 grep.h                          | 13 ++-----
 t/helper/test-pcre2-config.c    | 12 ++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 39 +++++++++++++++++++
 7 files changed, 80 insertions(+), 55 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

As long as I can just apply all of them on top of no-kwset and keep it a single topic, it wouldn't be too much of a hassle.

Yikes. Perhaps we should kick the no-kwset thing out of 'next' and start from scratch? It does not sound that the world is ready yet.

But that is just a knee-jerk reaction before reading the actual patches. Let's see how they look ;-)

Thanks.

On Fri, Jul 26 2019, Junio C Hamano wrote:

I have some fix-for-the-fix and was going to submit a v3 of this series, but I think the more responsible thing to do at this point, especially with various patches from Carlo that need to be integrated in one way or another, is to back it out until the outstanding issues are addressed.

If it's not too much trouble, would you mind reverting just the two patches at the tip of ab/no-kwset in "next"? I.e.

    b65abcafc7 ("grep: use PCRE v2 for optimized fixed-string search", 2019-07-01)
    48de2a768c ("grep: remove the kwset optimization", 2019-07-01)

I believe the rest are all settled & haven't had any issues raised with them, and those tests & preparatory fixes would be very useful to have in "master" for any re-roll without needing to be distracted by those changes.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

As we'd be in pre-release freeze soonish, let me revert the merge to 'next' and drop these two tip commits from the topic branch (keeping the earlier parts), and ask you two to work together to get the pcre-jit stuff into a good shape.

Thanks.

As discussed in the "grep: stess test PCRE v2 on invalid UTF-8 data" commit leading up to this one there's a regression in b65abcafc7 ("grep: use PCRE v2 for optimized fixed-string search", 2019-07-01) when matching UTF-8 data.

This ultimately isn't straightforward to just "fix", because the kwset backend was so dumb about icase matching that we'd skip it entirely on non-ASCII. See the code removed in 48de2a768c ("grep: remove the kwset optimization", 2019-07-01).

Just going back to the C library for those isn't ideal, since it's likely to be even dumber about these mixed-encoding cases.

So let's support this "properly" using the PCRE2_MATCH_INVALID_UTF flag. This is new code that's not in any released PCRE v2 version, so we might need a fix that emulates it somehow. I figure that the case that with the non-icase bug out of the way this is obscure enough to tell people "upgrade your PCRE v2 too!'. It'll likely be released by the time we release the git version this commit is part of.

We can't just use PCRE2_NO_UTF_CHECK instead for the reasons discussed in [1].

1. https://public-inbox.org/git/87lfwn70nb.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                        |  1 +
 grep.c                          |  2 +-
 grep.h                          |  3 +++
 t/helper/test-pcre2-config.c    | 12 ++++++++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 13 ++++++++++++-
 7 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

This won't even build with any released pcre version; shouldn't we make it at least conditionally compiled code? Specifically...

... unlike this piece of code ...

... this part does not have any fallback definition.

On Fri, Jul 26 2019, Junio C Hamano wrote:

It works because we include grep.h, which'll define PCRE2_MATCH_INVALID_UTF=0 if pcre2.h doesn't give it to us. I've tested this on PCRE versions with/without PCRE2_MATCH_INVALID_UTF and it works & runs/skips the appropriate tests.

On Fri, Jul 26 2019, Ævar Arnfjörð Bjarmason wrote:

Ah, I spoke too soon, of course that's all guarded by "are we using PCRE v2 in general?". I'll fix it...

This is v3 of a patch I originally sent in mid-2019: https://lore.kernel.org/git/20190726150818.6373-9-avarab@gmail.com/

Back then we were near a release and the PCREv2 feature I'm using in 4/4 wasn't in any released version. Now it's in widely used releases, so we can use it and fix some long-standing TODOs in invalid UTF-8 grep matching edge cases.

Ævar Arnfjörð Bjarmason (4):
  grep/pcre2 tests: don't rely on invalid UTF-8 data test
  grep/pcre2: simplify boolean spaghetti
  grep/pcre2: further simplify boolean spaghetti
  grep/pcre2: better support invalid UTF-8 haystacks

 Makefile                        |  1 +
 grep.c                          | 11 +++++--
 grep.h                          |  4 +++
 t/helper/test-pcre2-config.c    | 12 ++++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 53 ++++++++++++++++++++++++++++-----
 7 files changed, 74 insertions(+), 9 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

As noted in [1] when I originally added this test in [2] the test was completely broken as it lacked a redirect[3]. I now think this whole thing is overly fragile. Let's only test if we have a segfault here.

Before this the first test's "test_cmp" was pretty meaningless. We were only testing if PCREv2 was so broken that it would spew out something completely unrelated on stdout, which isn't very plausible.

In the second test we're relying on PCREv2 forever holding to the current behavior of the PCRE_UTF8 flag, as opposed to learning some optimistic graceful fallback to PCRE2_MATCH_INVALID_UTF in the future. If that happens having this test broken under bisecting would suck.

A follow-up commit will actually test this case in a meaningful way under the PCRE2_MATCH_INVALID_UTF flag. Let's run this one unconditionally, and just make sure we don't segfault.

1. e714b898c6 (t7812: expect failure for grep -i with invalid UTF-8
   data, 2019-11-29)
2. 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data,
   2019-07-26)
3. c74b3cbb83 (t7812: add missing redirects, 2019-11-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7812-grep-icase-non-ascii.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Improve the support for invalid UTF-8 haystacks given a non-ASCII needle when using the PCREv2 backend.

This is a more complete fix for a bug I started to fix in 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching, 2019-07-26), now that PCREv2 has the PCRE2_MATCH_INVALID_UTF mode we can make use of it.

This fixes the sort of case described in 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26), i.e.:

    - The subject string is non-ASCII (e.g. "ævar")
    - We're under a is_utf8_locale(), e.g. "en_US.UTF-8", not "C"
    - We are using --ignore-case, or we're a non-fixed pattern

If those conditions were satisfied and we matched found non-valid UTF-8 data PCREv2 might bark on it, in practice this only happened under the JIT backend (turned on by default on most platforms).

Ultimately this fixes a "regression" in b65abcafc7 ("grep: use PCRE v2 for optimized fixed-string search", 2019-07-01), I'm putting that in scare-quotes because before then we wouldn't properly support these complex case-folding, locale etc. cases either, it just broke in different ways.

There was a bug related to this the PCRE2_NO_START_OPTIMIZE flag fixed in PCREv2 10.36. It can be worked around by setting the PCRE2_NO_START_OPTIMIZE flag. Let's do that in those cases, and add tests for the bug.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                        |  1 +
 grep.c                          |  8 +++++-
 grep.h                          |  4 +++
 t/helper/test-pcre2-config.c    | 12 +++++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 46 ++++++++++++++++++++++++++++++++-
 7 files changed, 71 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

On 24/01/2021 11:48, Ævar Arnfjörð Bjarmason wrote:

                                   ^^^^^^^^^^^^^^^^^^
I assume that this should be s/_MAJOR/_MINOR/. ;-)

ATB, Ramsay Jones

On 24/01/2021 13:53, Ramsay Jones wrote: [snip]

Although, perhaps you want:

            !(((PCRE2_MAJOR * 100) + PCRE2_MINOR) >= 1036)

... or something similar.

ATB, Ramsay Jones

On Sun, Jan 24 2021, Ramsay Jones wrote:

Oops on the s/MAJOR/MINOR/g. Well spotted, I think I'll wait a bit more for other comments for a re-roll.

Perhaps Junio can be kind and do the s/_MAJOR/_MINOR/ fixup in the meantime to save be from spamming the list too much...

FWIW I have tested this on a verion without PCRE2_MATCH_INVALID_UTF, but I think I did that by manually editing the "PCRE2_UTF" line above, and then wrote this bug.

Probably better to use pcre2_config(PCRE2_CONFIG_VERSION) at that point and versioncmp() the string.

FWIW we used this pattern (I added) before with PCRE, see e87de7cab4 (grep: un-break building with PCRE < 8.32, 2017-05-25).

And if you want to be clever ((PCRE2_MAJOR) | ((PCRE2_MINOR) << 16)) is probably better, then you can get it out with bit operations :)

On 24/01/2021 14:49, Ævar Arnfjörð Bjarmason wrote:

Umm, sorry for not making myself clear, _just_ changing MAJOR to MINOR is insufficient.

Yep, I seem to have 10.34 on Linux Mint 20.1 (based on Ubuntu 20.04).

OK, but it needs to be 'something similar' (try putting, say, MAJOR 11 and MINOR 0->35 in your expression).

ATB, Ramsay Jones

On Sun, Jan 24 2021, Ramsay Jones wrote:

Ah yes, of course. I re-rolled a v5 with a fix for that. Thanks.

Changes since v2: Dropped mid-series unrelated refactoring. Will leave it to a future patch series to simplify/move/refactor this code more generally.

Ævar Arnfjörð Bjarmason (2):
  grep/pcre2 tests: don't rely on invalid UTF-8 data test
  grep/pcre2: better support invalid UTF-8 haystacks

 Makefile                        |  1 +
 grep.c                          |  8 ++++-
 grep.h                          |  4 +++
 t/helper/test-pcre2-config.c    | 12 ++++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 53 ++++++++++++++++++++++++++++-----
 7 files changed, 72 insertions(+), 8 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

Range-diff: -: ---------- > 1: 699bb6b324 grep/pcre2 tests: don't rely on invalid UTF-8 data test -: ---------- > 2: e4807d6879 grep/pcre2: better support invalid UTF-8 haystacks

Fixes a version comparison typo/thinko, pointed out by Ramsay Jones.

Ævar Arnfjörð Bjarmason (2):
  grep/pcre2 tests: don't rely on invalid UTF-8 data test
  grep/pcre2: better support invalid UTF-8 haystacks

 Makefile                        |  1 +
 grep.c                          | 18 ++++++++++-
 grep.h                          |  4 +++
 t/helper/test-pcre2-config.c    | 12 ++++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 53 ++++++++++++++++++++++++++++-----
 7 files changed, 82 insertions(+), 8 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

Range-diff:
1:  699bb6b324 = 1:  699bb6b324 grep/pcre2 tests: don't rely on invalid UTF-8 data test
2:  e4807d6879 ! 2:  04c87c04d7 grep/pcre2: better support invalid UTF-8 haystacks
    @@ grep.c: static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_
     -		options |= PCRE2_UTF;
     +		options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
     +
    -+	if (PCRE2_MATCH_INVALID_UTF &&
    -+	    options & (PCRE2_UTF | PCRE2_CASELESS) &&
    -+	    !(PCRE2_MAJOR >= 10 && PCRE2_MAJOR >= 36))
    -+		/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */
    -+		options |= PCRE2_NO_START_OPTIMIZE;
    ++	/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */
    ++	if (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | PCRE2_CASELESS)) {
    ++		struct strbuf buf;
    ++		int len;
    ++		int err;
    ++
    ++		if ((len = pcre2_config(PCRE2_CONFIG_VERSION, NULL)) < 0)
    ++			BUG("pcre2_config(..., NULL) failed: %d", len);
    ++		strbuf_init(&buf, len + 1);
    ++		if ((err = pcre2_config(PCRE2_CONFIG_VERSION, buf.buf)) < 0)
    ++			BUG("pcre2_config(..., buf.buf) failed: %d", err);
    ++		if (versioncmp(buf.buf, "10.36") < 0)
    ++			options |= PCRE2_NO_START_OPTIMIZE;
    ++		strbuf_release(&buf);
    ++	}
      
      	p->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,
      					 p->patternlen, options, &error, &erroffset,

As noted in [1] when I originally added this test in [2] the test was completely broken as it lacked a redirect[3]. I now think this whole thing is overly fragile. Let's only test if we have a segfault here.

Before this the first test's "test_cmp" was pretty meaningless. We were only testing if PCREv2 was so broken that it would spew out something completely unrelated on stdout, which isn't very plausible.

In the second test we're relying on PCREv2 forever holding to the current behavior of the PCRE_UTF8 flag, as opposed to learning some optimistic graceful fallback to PCRE2_MATCH_INVALID_UTF in the future. If that happens having this test broken under bisecting would suck.

A follow-up commit will actually test this case in a meaningful way under the PCRE2_MATCH_INVALID_UTF flag. Let's run this one unconditionally, and just make sure we don't segfault.

1. e714b898c6 (t7812: expect failure for grep -i with invalid UTF-8
   data, 2019-11-29)
2. 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data,
   2019-07-26)
3. c74b3cbb83 (t7812: add missing redirects, 2019-11-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7812-grep-icase-non-ascii.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Improve the support for invalid UTF-8 haystacks given a non-ASCII needle when using the PCREv2 backend.

This is a more complete fix for a bug I started to fix in 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching, 2019-07-26), now that PCREv2 has the PCRE2_MATCH_INVALID_UTF mode we can make use of it.

This fixes the sort of case described in 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26), i.e.:

    - The subject string is non-ASCII (e.g. "ævar")
    - We're under a is_utf8_locale(), e.g. "en_US.UTF-8", not "C"
    - We are using --ignore-case, or we're a non-fixed pattern

If those conditions were satisfied and we matched found non-valid UTF-8 data PCREv2 might bark on it, in practice this only happened under the JIT backend (turned on by default on most platforms).

Ultimately this fixes a "regression" in b65abcafc7 ("grep: use PCRE v2 for optimized fixed-string search", 2019-07-01), I'm putting that in scare-quotes because before then we wouldn't properly support these complex case-folding, locale etc. cases either, it just broke in different ways.

There was a bug related to this the PCRE2_NO_START_OPTIMIZE flag fixed in PCREv2 10.36. It can be worked around by setting the PCRE2_NO_START_OPTIMIZE flag. Let's do that in those cases, and add tests for the bug.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                        |  1 +
 grep.c                          | 18 ++++++++++++-
 grep.h                          |  4 +++
 t/helper/test-pcre2-config.c    | 12 +++++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 46 ++++++++++++++++++++++++++++++++-
 7 files changed, 81 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

As noted in [1] when I originally added this test in [2] the test was completely broken as it lacked a redirect[3]. I now think this whole thing is overly fragile. Let's only test if we have a segfault here.

Before this the first test's "test_cmp" was pretty meaningless. We were only testing if PCREv2 was so broken that it would spew out something completely unrelated on stdout, which isn't very plausible.

In the second test we're relying on PCREv2 forever holding to the current behavior of the PCRE_UTF8 flag, as opposed to learning some optimistic graceful fallback to PCRE2_MATCH_INVALID_UTF in the future. If that happens having this test broken under bisecting would suck.

A follow-up commit will actually test this case in a meaningful way under the PCRE2_MATCH_INVALID_UTF flag. Let's run this one unconditionally, and just make sure we don't segfault.

1. e714b898c6 (t7812: expect failure for grep -i with invalid UTF-8
   data, 2019-11-29)
2. 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data,
   2019-07-26)
3. c74b3cbb83 (t7812: add missing redirects, 2019-11-26)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7812-grep-icase-non-ascii.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Improve the support for invalid UTF-8 haystacks given a non-ASCII needle when using the PCREv2 backend.

This is a more complete fix for a bug I started to fix in 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching, 2019-07-26), now that PCREv2 has the PCRE2_MATCH_INVALID_UTF mode we can make use of it.

This fixes the sort of case described in 8a5999838e (grep: stess test PCRE v2 on invalid UTF-8 data, 2019-07-26), i.e.:

    - The subject string is non-ASCII (e.g. "ævar")
    - We're under a is_utf8_locale(), e.g. "en_US.UTF-8", not "C"
    - We are using --ignore-case, or we're a non-fixed pattern

If those conditions were satisfied and we matched found non-valid UTF-8 data PCREv2 might bark on it, in practice this only happened under the JIT backend (turned on by default on most platforms).

Ultimately this fixes a "regression" in b65abcafc7 ("grep: use PCRE v2 for optimized fixed-string search", 2019-07-01), I'm putting that in scare-quotes because before then we wouldn't properly support these complex case-folding, locale etc. cases either, it just broke in different ways.

There was a bug related to this the PCRE2_NO_START_OPTIMIZE flag fixed in PCREv2 10.36. It can be worked around by setting the PCRE2_NO_START_OPTIMIZE flag. Let's do that in those cases, and add tests for the bug.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                        |  1 +
 grep.c                          |  8 +++++-
 grep.h                          |  4 +++
 t/helper/test-pcre2-config.c    | 12 +++++++++
 t/helper/test-tool.c            |  1 +
 t/helper/test-tool.h            |  1 +
 t/t7812-grep-icase-non-ascii.sh | 46 ++++++++++++++++++++++++++++++++-
 7 files changed, 71 insertions(+), 2 deletions(-)
 create mode 100644 t/helper/test-pcre2-config.c

Follow-up the last commit by splitting the fixed check for the PCRE2_UTF flag into a variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Simplify an expression I added in 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching, 2019-07-26) by using a simple application of De Morgan's laws[1]. I.e.:

    NOT(A && B) is Equivalent to (NOT(A) OR NOT(B))

1. https://en.wikipedia.org/wiki/De_Morgan%27s_laws

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

At this level, however, the left one looks much simpler than the right one ;-)

In the context of this expression, well, I guess the rewritten one is probably simpler but can we explain the whole condition in fewer than three lines? With or without the rewrite, it still looks too complicated to me.

Am 24.01.21 um 06:33 schrieb Junio C Hamano:

Make the condition

 	if (!opt->ignore_locale &&
	    is_utf8_locale() &&
	    has_non_ascii(p->pattern) &&
	    (opt->ignore_case ||
		(!p->fixed &&
		 !p->is_fixed)))
	{
  		options |= PCRE2_UTF;
	}

With the knowledge of the equivalence

    (A => B)  <=>  (NOT(A) OR B)

(A => B means "if A then B"), the condition makes a lot of sense when read aloud:

    if
       NOT ignore locale
       AND
       is UTF8
       AND
       has non-ASCII
       AND
         if
            NOT ignore case
         then if also
            NOT fixed
            AND
            NOT is fixed
    then
        ...

The codition amounts to extending a series of conjunctions with more conjuctions IF a condition is satisfied. That's quite sensible.

You have to swap the polarity of the first condition of || in your head, though, to achieve that meaning. That works with every OR condition, BTW.

-- Hannes

As reported in [1] the code I added in 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) to use a custom JIT stack has never worked. It was incorrectly copy/pasted from code I added in fbaceaac47 ("grep: add support for the PCRE v1 JIT API", 2017-05-25), which did work.

Thus our intention of starting with 1 byte of stack at a maximum of 1 MB didn't happen, we'd always use the 32 KB stack provided by PCRE v2's jit_machine_stack_exec()[2]. The reason I allocated a custom stack at all was this advice in pcrejit(3) (same in pcre2jit(3)):

    "By default, it uses 32KiB on the machine stack. However, some
    large or complicated patterns need more than this"

Since we've haven't had any reports of users running into PCRE2_ERROR_JIT_STACKLIMIT in the wild I think we can safely assume that we can just use the library defaults instead and drop this code. This won't change with the wider use of PCRE v2 in ed0479ce3d ("Merge branch 'ab/no-kwset' into next", 2019-07-15), a fixed string search is not a "large or complicated pattern".

For good measure I ran the performance test noted in 94da9193a6, although the command is simpler now due to my 0f50c8e32c ("Makefile: remove the NO_R_TO_GCC_LINKER flag", 2019-05-17):

    GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE2=Y CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre2/inst' ./run HEAD~ HEAD p7820-grep-engines.sh

Just the /perl/ results are:

    Test                                            HEAD~             HEAD
    ---------------------------------------------------------------------------------------
    7820.3: perl grep 'how.to'                      0.17(0.27+0.65)   0.17(0.24+0.68) +0.0%
    7820.7: perl grep '^how to'                     0.16(0.23+0.66)   0.16(0.23+0.67) +0.0%
    7820.11: perl grep '[how] to'                   0.18(0.35+0.62)   0.18(0.33+0.65) +0.0%
    7820.15: perl grep '(e.t[^ ]*|v.ry) rare'       0.17(0.45+0.54)   0.17(0.49+0.50) +0.0%
    7820.19: perl grep 'm(ú|u)lt.b(æ|y)te'          0.16(0.33+0.58)   0.16(0.29+0.62) +0.0%

So, as expected there's no change, and running with valgrind reveals that we have fewer allocations now.

1. https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/
2. I didn't really intend to start with 1 byte, looking at the PCRE v2
   code again what happened is that I cargo-culted some of PCRE v2's
   own test code which was meant to test re-allocations. It's more
   sane to start with say 32 KB with a max of 1 MB, as pcre2grep.c
   does.

Reported-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 10 ----------
 grep.h |  4 ----
 2 files changed, 14 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Does everybody use pcre2 with JIT with Git these days, or only those who want to live near the bleeding edge?

In any case, if we were not "using" the custom stack anyway for v2, this change does not hurt anybody, possibly other than those who will learn about pcre2 support by reading this message and experiments with larger patterns. And it should be simple to wire it back if it becomes necessary later.

Thanks for cleaning up.

On Wed, Jul 24 2019, Junio C Hamano wrote:

My informal survey of various package recipies suggests that all the big *nix distros are using it by default now, so we have a lot of users in the wild, including in the just-released Debian stable.

So I'm confidend that if there were issues with e.g. it dying on patterns in practical use we'd have heard about them.

*nod*

On Wed, Jul 24, 2019 at 1:20 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

FWIW neither OpenBSD or NetBSD enable JIT, and the git that comes with Xcode (AKA Apple Git) doesn't either, while still using PCRE1

the following pattern fails unless 1MB stack is available:

  '^([/](?!/)|[^/])*~/.*'

the workaround implemented in GNU grep (that uses PCRE1) and the related discussion[1] are a very interesting read

Carlo

[1] https://www.mail-archive.com/bug-grep@gnu.org/msg05763.html

Remove code that would trigger if pcre_config() or pcre2_config() was so broken that "do we have JIT?" wouldn't return a boolean.

I added this code back in fbaceaac47 ("grep: add support for the PCRE v1 JIT API", 2017-05-25) and then as noted in [1] incorrectly copy/pasted some of it in 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01).

Let's just remove it instead of fixing that bug. Being this paranoid about what PCRE returns is crossing the line into unreasonable paranoia.

1. https://public-inbox.org/git/20190722181923.21572-1-dev+git@drbeat.li/

Reported-by:  Beat Bolli <dev+git@drbeat.li>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Simplify the PCRE v1 code for the same reasons as for the PCRE v2 code in the last commit. Unlike with v2 we actually used the custom stack in v1, but let's use PCRE's built-in 32 KB one instead, since experience with v2 shows that's enough. Most distros are already using v2 as a default, and the underlying sljit code is the same.

Unfortunately we can't just pass a NULL to pcre_jit_exec() as with pcre2_jit_match(). Unlike the v2 function it doesn't support that. Instead we need to use the fatter pcre_exec() if we'd like the same behavior.

This will make things slightly slower than on the fast-path function, but it's OK since we care less about v1 performance these days since we have and recommend v2. Running a similar performance test as what I ran in fbaceaac47 ("grep: add support for the PCRE v1 JIT API", 2017-05-25) via:

    GIT_PERF_REPEAT_COUNT=30 GIT_PERF_LARGE_REPO=~/g/linux GIT_PERF_MAKE_OPTS='-j8 USE_LIBPCRE1=Y CFLAGS=-O3 LIBPCREDIR=/home/avar/g/pcre/inst' ./run HEAD~ HEAD p7820-grep-engines.sh

Gives us this, just the /perl/ results:

    Test                                            HEAD~             HEAD
    ---------------------------------------------------------------------------------------
    7820.3: perl grep 'how.to'                      0.19(0.67+0.52)   0.19(0.65+0.52) +0.0%
    7820.7: perl grep '^how to'                     0.19(0.78+0.44)   0.19(0.72+0.49) +0.0%
    7820.11: perl grep '[how] to'                   0.39(2.13+0.43)   0.40(2.10+0.46) +2.6%
    7820.15: perl grep '(e.t[^ ]*|v.ry) rare'       0.44(2.55+0.37)   0.45(2.47+0.41) +2.3%
    7820.19: perl grep 'm(ú|u)lt.b(æ|y)te'          0.23(1.06+0.42)   0.22(1.03+0.43) -4.3%

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 grep.c | 28 +++++-----------------------
 grep.h |  5 -----
 2 files changed, 5 insertions(+), 28 deletions(-)

since this moves PCRE1 out of the JIT fast path, introduces the regression where git grep will abort if there is binary data or non UTF-8 text in the repository/log and should be IMHO hold out until a fix for that can be merged.

this also needs additional changes to better support NO_LIBPCRE1_JIT, patch to follow

Carlo

On Fri, Jul 26 2019, Carlo Arenas wrote:

I think you're mostly replying to the wrong thread. None of the patches I've sent disable PCRE v1 JIT, as the performance numbers show. The JIT stack is resized, and for v2 some dead code removed.

You're talking about the kwset series, not this cleanup series.

Looking forward to it, thanks!

On Fri, Jul 26, 2019 at 6:50 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I didn't mean JIT was disabled, but that we are calling now the regular PCRE1 function which does UTF-8 validation (unlike the one used before)

a combination of both (as seen in pu) and that will also happen in next if this series get merged there.

before this cleanup series, a git compiled against PCRE1 and not using NO_LIBPCRE1_JIT will use the jit fast path function and therefore would have no problems with binary or non UTF-8 content in the repository, but will regress after.

Carlo

On Fri, Jul 26 2019, Carlo Arenas wrote:

I see. Yes you're right, I misread pcrejit(3) about how the "fast path API" worked (or more accurately, misremembered). Yes, this is now a new caveat.

I have some patches on top of next I'm about to send that hopefully make this whole thing less of a mess.

Sent as an RFC since it was meant to be applied against ab/pcre-jit-fixes but that is likely to change with the reroll of that branch.

 [PATCH 1/2] grep: make sure NO_LIBPCRE1_JIT disable JIT in PCRE1
 [PATCH 2/2] grep: refactor and simplify PCRE1 support

The end result could be squashed together before merging but sent as independentt changes to make review easier, with the first change doing the minimum required to make PCRE1 great again.

 Makefile |  9 ++-------
 grep.c   | 15 +++++++++------
 grep.h   | 11 -----------
 3 files changed, 11 insertions(+), 24 deletions(-)

base-commit: 0f8c4ddfdddb72dc62d76864f5d3d31f136c7129

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(-)

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 | 15 +++++++++------
 grep.h |  9 ---------
 2 files changed, 9 insertions(+), 15 deletions(-)