git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Mathias Krause <minipli@grsecurity.net>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Subject: Re: [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails
Date: Mon, 30 Jan 2023 12:08:31 -0800	[thread overview]
Message-ID: <xmqqlelj3hvk.fsf@gitster.g> (raw)
In-Reply-To: <xmqq357r4zvk.fsf@gitster.g> (Junio C. Hamano's message of "Mon, 30 Jan 2023 10:54:23 -0800")

Junio C Hamano <gitster@pobox.com> writes:

> If we were to keep that "die", it is absolutely required, I would
> think.  Users who got their Git with JIT-enabled pcre2 may be
> viewing JIT merely as "a clever optimization the implementation is
> allowed to use when able", without knowing and more importantly
> without wanting to know how to disable it from within their
> patterns.
>
> But can't we drop that die() if we took the v1 route?

Having said all that, I do not mind queuing v2 if the "use *NO_JIT
to disable" is added to the message to help users who are forced to
redo the query.

And in practice, it shouldn't make that much difference, because the
only scenario (other than the SELinux-like situation where JIT is
compiled in but does not work at all) that the difference may matter
would happen when a non-trivial portion of the patterns users use
are not workable with JIT, but if that were the case, we would have
written JIT off as not mature enough and not yet usable long time
ago.  So, in practice, patterns refused by JIT would be a very tiny
minority to matter in real life, and "failing fast to inconvenience
users" would not be too bad.

So while I still think v1's simplicity is the right thing to have
here, I think it is waste of our braincell to compare v1 vs v2.  As
v2 gives smaller incremental behaviour change perceived by end
users, if somebody really wanted to, I'd expect that a low-hanging
fruit #leftoverbit on top of such a patch, after the dust settles,
would be to

 (1) rename pcre2_jit_functional() to fall_back_to_interpreter() or
     something,

 (2) add a configuration variable to tell fall_back_to_interpreter()
     that any form of JIT error is allowed to fall back to
     interpreter().

and such a patch will essentially give back the simplicity of v1 to
folks who opt into the configuration.

Thanks.

  reply	other threads:[~2023-01-30 20:08 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-16 12:15 [PATCH] grep: fall back to interpreter mode if JIT fails Mathias Krause
2022-12-16 16:12 ` Ævar Arnfjörð Bjarmason
2022-12-16 19:26   ` Mathias Krause
2022-12-16 23:09     ` Junio C Hamano
2022-12-17  2:50       ` Carlo Arenas
2022-12-19  9:00         ` Ævar Arnfjörð Bjarmason
2022-12-20 19:29           ` Mathias Krause
2022-12-20 21:11             ` Ævar Arnfjörð Bjarmason
2023-01-18 14:22               ` Mathias Krause
2023-01-18 15:44                 ` Ævar Arnfjörð Bjarmason
2023-01-19  9:19                   ` Mathias Krause
2022-12-16 22:52 ` Junio C Hamano
2022-12-20 20:40   ` Mathias Krause
2023-01-27 15:49 ` [PATCH v2] grep: fall back to interpreter if JIT memory allocation fails Mathias Krause
2023-01-27 16:34   ` Junio C Hamano
2023-01-27 17:39     ` Junio C Hamano
2023-01-27 18:46       ` Junio C Hamano
2023-01-29 13:37         ` Mathias Krause
2023-01-29 13:36       ` Mathias Krause
2023-01-29 17:15         ` Junio C Hamano
2023-01-30 10:56           ` Ævar Arnfjörð Bjarmason
2023-01-30 18:49             ` Junio C Hamano
2023-01-31  8:34               ` Ævar Arnfjörð Bjarmason
2023-01-30 11:08           ` Mathias Krause
2023-01-30 18:54             ` Junio C Hamano
2023-01-30 20:08               ` Junio C Hamano [this message]
2023-01-30 21:21                 ` Junio C Hamano
2023-01-30 22:30                   ` Ramsay Jones
2023-01-30 23:27                     ` Junio C Hamano
2023-01-31  7:48                   ` Mathias Krause
2023-01-31 16:41                     ` Junio C Hamano
2023-01-31 18:34                       ` Mathias Krause
2023-01-31  7:30                 ` Mathias Krause
2023-01-29 12:28     ` Mathias Krause
2023-01-31 18:56   ` [PATCH v3] " Mathias Krause
2023-01-31 21:05     ` Junio C Hamano

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=xmqqlelj3hvk.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=minipli@grsecurity.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public 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).