Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1

From: Carlo Arenas <carenas@gmail.com>
Date: Dec 11, 2018, 20:11 UTC
Message-ID: <CAPUEspjHs7+G+FHXjxb4rCcNLqaybbhZESik=14_9Q5h2HMzMA@mail.gmail.com>
In-Reply-To: <87pnu9yekk.fsf@evledraar.gmail.com>

On Mon, Dec 10, 2018 at 12:24 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 9 quoted lines

> On Mon, Dec 10 2018, brian m. carlson wrote:
> > Considering that some Linux users use PaX kernels with standard
> > distributions and that most BSD kernels can be custom-compiled with a
> > variety of options enabled or disabled, I think this is something we
> > should detect dynamically.
>
> Right. I'm asking whether we're mixing up cases where it can always be
> detected at compile-time on some systems v.s. cases where it'll
> potentially change at runtime.

the closer we come to a system specific issues is with macOS where the compiler (in some newer versions) is allocating the memory using the MAP_JIT flag, which seems was originally meant to be only used in iOS and has the strange characteristic of failing the mmap for versions older than 10.14 if it was called more than once.

IMHO as brian pointed out, this is better done at runtime.

Show 17 quoted lines

> >> I'm inclined to suggest that we should have another ifdef here for "if
> >> JIT fails I'd like it to die", so that e.g. packages I build (for
> >> internal use) don't silently slow down in the future, only for me to
> >> find some months later that someone enabled an overzealous SELinux
> >> policy and we swept this under the rug.
> >
> > My view is that JIT is a nice performance optimization, but it's
> > optional. I honestly don't think it should even be exposed through the
> > API: if it works, then things are faster, and if it doesn't, then
> > they're not. I don't see the value in an option for causing things to be
> > broken if someone improves the security of the system.
>
> For many users that's definitely the case, but for others that's like
> saying a RDBMS is still going to be functional if the "ORDER BY"
> function degrades to bubblesort. The JIT improves performance my
> multi-hundred percents sometimes, so some users (e.g. me) rely on that
> not being silently degraded.

the opposite is also true, specially considering that some old versions of pcre result in a segfault instead of an error message and therefore since there is no way to disable JIT, the only option left is not to use `git grep -P` (or the equivalent git log call)

Show 7 quoted lines

> So I'm wondering if we can have something like:
>
>     if (!jit)
>         if (must_have_jit)
>             BUG(...); // Like currently
>         else
>             fallback(); // new behavior

I am wondering if something like a `git doctor` command might be an interesting alternative to this.

This way we could (for ex: in NetBSD) give the user a hint of what to do to make their git grep -P faster when we detect we are running the fallback, and might be useful as well to provide hints for optimizations that could be used in other cases (probably even depending on the size of the git repository)

For your use case, you just need to add a crontab that will trigger an alarm if this command ever mentions PCRE

Carlo