git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Carlo Arenas <carenas@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, sandals@crustytoothpaste.net
Subject: Re: [RFC PATCH] grep: allow for run time disabling of JIT in PCRE
Date: Tue, 30 Jul 2019 06:01:53 -0700	[thread overview]
Message-ID: <CAPUEspiqKim_Ypc+PPZPKvqQu9DvW7D+mHxB9cyheo8FDQs3VQ@mail.gmail.com> (raw)
In-Reply-To: <87wog15834.fsf@evledraar.gmail.com>

On Mon, Jul 29, 2019 at 5:38 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Jul 29 2019, Carlo Arenas wrote:
> > On Mon, Jul 29, 2019 at 1:55 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> On Mon, Jul 29 2019, Carlo Marcelo Arenas Belón wrote:
> >>
> >> > PCRE1 allowed for a compile time flag to disable JIT, but PCRE2 never
> >> > had one, forcing the use of JIT if -P was requested.
> >>
> >> What's that PCRE1 compile-time flag?
> >
> > NO_LIBPCRE1_JIT at GIT compile time (regardless of JIT support in the
> > PCRE1 library you are using)
>
> Ah of course, I was reading this as "regexp
> compile-time". I.e. something like (*NO_JIT). No *such* thing exists for
> PCRE v1 JIT AFAIK as exposed by git-grep.

correct, but there are still other knobs like (*UTF), (*UCP), (?m) or
(?i) that also
affect some of our assumptions.

> >> > After ed0479ce3d (Merge branch 'ab/no-kwset' into next, 2019-07-15)
> >> > the PCRE2 engine will be used more broadly and therefore adding this
> >> > knob will give users a fallback for situations like the one observed
> >> > in OpenBSD with a JIT enabled PCRE2, because of W^X restrictions:
> >> >
> >> >   $ git grep 'foo bar'
> >> >   fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'
> >> >   $ git grep -G 'foo bar'
> >> >   fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'
> >> >   $ git grep -E 'foo bar'
> >> >   fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'
> >> >   $ git grep -F 'foo bar'
> >> >   fatal: Couldn't JIT the PCRE2 pattern 'foo bar', got '-48'
> >>
> >> Yeah that obviously sucks more with ab/no-kwset, but that seems like a
> >> case where -P would have been completely broken before, and therefore I
> >> can't imagine the package ever passed "make test". Or is W^X also
> >> exposed as some run-time option on OpenBSD?
> >
> > ironically, you could use PCRE1 since that is not using the JIT fast
> > path and therefore will fallback automatically to the interpreter
>
> ...because OpenBSD PCRE v1 was compiled with --disable-jit before, but
> their v2 package has --enable-jit, it just doesn't work at all? Is this
> your custom built git + OpenBSD packages of PCRE coming with the OS?

sorry for the confusion, custom builds of both PCRE and git.
and I was referring to the fact that after 685668faaa (grep: stop
using a custom JIT stack with PCRE v1, 2019-07-26)
and with my patches to avoid UTF-8 issues, git + PCRE1 was a much pleasant
experience than git + PCRE2 in OpenBSD (which is also why I didn't
even care about fixing it for pcre.jit=!1 yet)

as shown by :
$ git grep 'foo bar' Documentation | cat
Documentation/RelNotes/1.8.0.txt:   when the user says "git checkout
-b -t foo bar" (e.g. "-t" is not a
Documentation/git-rev-list.txt: $ git rev-list foo bar ^baz
Documentation/gitattributes.txt:abc foo bar baz
$ git grep -P 'foo bar' Documentation | cat
Documentation/RelNotes/1.8.0.txt:   when the user says "git checkout
-b -t foo bar" (e.g. "-t" is not a
Documentation/git-rev-list.txt: $ git rev-list foo bar ^baz
Documentation/gitattributes.txt:abc foo bar baz
$ git grep -P 'foo[ ]bar' Documentation | cat
Documentation/RelNotes/1.8.0.txt:   when the user says "git checkout
-b -t foo bar" (e.g. "-t" is not a
Documentation/git-rev-list.txt: $ git rev-list foo bar ^baz
Documentation/gitattributes.txt:abc foo bar baz
$ dmesg | grep git
git-grep(87484): mmap W^X violation

the last of which might be suppressed with `-c pcre.jit=0` once I get
to fix that

> I don't use OpenBSD, but isn't this their recipe? Seems they use "make
> test", and don't compile with PCRE at all if I'm reading it right:
> https://github.com/openbsd/ports/blob/master/devel/git/Makefile

yes, I was using OpenBSD as a testbase where issues with JIT would
be easily reproducible; their packagers are smart enough not to enable
JIT by default in PCRE or even link it with git as you pointed out.

NetBSD/HardenedBSD might be a better example of a native default package that
would be affected in its standard configuration, if that is what you
were looking for.

> >> I.e. aside from the merits of such a setting in general these examples
> >> seem like just working around something that should be fixed at make
> >> all/test time, or maybe I'm missing something.
> >
> > 1) before you could just avoid using -P and still be able to grep
> > 2) there is no way to tell PCRE2 to get out of the way even if you are
> >     not using -P
>
> Right, no arguments at all about ab/no-kwset making this worse (re: your
> #1). I just really prefer not to expose/document config for what
> *should* be something purely internal if the X-Y problem is a bug being
> exposed that we should just fix.
>
> Particularly because I think it's a losing battle to provide run-time
> options for what are surely a *lot* of "make test" failures.

not sure I understand.  The knob was there to give flexibility to the user
to decide for himself how he wants to use the application and how that
use fits in their environment.

we can't predict either of those with 100% certainty and while it makes
sense we will impose some constrains we should understand that the
more inflexible those are, the more users will be alienated.

this specific constrain is particularly silly, even PCRE recommends using
JIT only as an optimization and fallback to the interpreter but we don't
follow that advice and the least we could do is give a escape hatch to
the users.

> If it really is unavoidable to detect this until runtime in some common
> configurations I have no problem with it, I just haven't encountered
> that so far.

guess it is sort of a chicken/egg problem and we would rather have
OpenBSD never linking their git with PCRE.

FWIW we can't know ahead of time if someone setup will include
running a PAX/SELinux enabled kernel on their otherwise regular
userspace encountering this problem.

there is also the case of Linux distributions without official packages
(like Gentoo and Arch) where each user decides which options they
want to use on their packages at their own time.

> patches = your recent
> https://public-inbox.org/git/20181209230024.43444-2-carenas@gmail.com/
> or something earlier?
>
> That patch seems sane without having tested it. Seems like the
> equivalent of what we do with v1 with PCRE2_JIT_COMPLETE.

I am missing context here; that patch was obsoleted by your ab/pcre-jit-fixes
branch and has nothing to do with PCRE2_JIT_COMPLETE.

if I recall correctly, was waiting on feedback on the series on top of your
original pcre-jit-fixes branch (with only 3 patches) and that was posted in:
https://public-inbox.org/git/20190726202642.7986-1-carenas@gmail.com/T/#u

> I *am* curious if there's setups where fixing the code for PCRE v1 isn't
> purely an academic exercise. Is there a reason for why these platforms
> can't just move to PCRE v2 in principle (dumpster fires in "next"
> non-withstanding)?

how can you expect me (or anyone else) to answer that question? obviously
we don't know, I personally for sure have no problem.

brian mentioned[1] some CentOS 6 users that don't have PCRE2 in their systems,
I mentioned Xcode's git in macOS as likely not updating since it uses
a system library
and that is also used by several other applications (like Safari)

but regardless of that, I see no reason for making their life more
difficult; PCRE1 is
widely used, it is still being supported, and they can use PCRE2 as an
alternative
most of the time (at least with git)

Carlo

[1] https://public-inbox.org/git/20190615191514.GD8616@genre.crustytoothpaste.net/

  reply	other threads:[~2019-07-30 13:02 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-28 23:54 Carlo Marcelo Arenas Belón
2019-07-29  0:09 ` Carlo Arenas
2019-07-29  4:57 ` Junio C Hamano
2019-07-29  5:29   ` Carlo Arenas
2019-07-29  8:55 ` Ævar Arnfjörð Bjarmason
2019-07-29 10:26   ` Carlo Arenas
2019-07-29 12:38     ` Ævar Arnfjörð Bjarmason
2019-07-30 13:01       ` Carlo Arenas [this message]
2019-07-29 10:59 ` [RFC PATCH v2] " Carlo Marcelo Arenas Belón
2019-07-29 11:33   ` Carlo Arenas
2019-07-29 15:11   ` René Scharfe
2019-07-29 17:47     ` Junio C Hamano
2019-07-30  0:49       ` Carlo Arenas
2019-07-30 17:55         ` René Scharfe
2019-07-31 12:36         ` Johannes Schindelin
2019-07-31 16:18           ` Junio C Hamano
2019-07-31 12:32   ` Johannes Schindelin
2019-07-31 14:57     ` Ævar Arnfjörð Bjarmason
2019-08-04  0:25       ` Carlo Arenas
2019-08-04  3:14   ` [RFC PATCH v3] grep: treat PCRE2 jit compilation memory error as non fatal Carlo Marcelo Arenas Belón
2019-08-04  7:43     ` Carlo Arenas
2019-08-05 20:16       ` Junio C Hamano
2019-07-31 12:24 ` [RFC PATCH] grep: allow for run time disabling of JIT in PCRE Johannes Schindelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPUEspiqKim_Ypc+PPZPKvqQu9DvW7D+mHxB9cyheo8FDQs3VQ@mail.gmail.com \
    --to=carenas@gmail.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sandals@crustytoothpaste.net \
    --subject='Re: [RFC PATCH] grep: allow for run time disabling of JIT in PCRE' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this 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).