On 27.01.23 17:34, Junio C Hamano wrote: > Mathias Krause <minipli@xxxxxxxxxxxxxx> writes: > >> As having a functional PCRE2 JIT compiler is a legitimate use case for >> performance reasons, we'll only do the fallback if the supposedly >> available JIT is found to be non-functional by attempting to JIT compile >> a very simple pattern. If this fails, JIT is deemed to be non-functional >> and we do the interpreter fallback. For all other cases, i.e. the simple >> pattern can be compiled but the user provided cannot, we fail hard as we >> do now as the reason for the failure must be the pattern itself. > > I do not know if it is a good idea to rely on the "very simple > pattern". The implementation of JIT could devise various ways to > succeed for such simple patterns without having writable-executable > piece of memory. Well, if PCRE2 JIT ever changes to optimize this case, we would be back to the error I'm seeing right now. But I doubt that PCRE2 will be doing optimizations like that. The current implementation does the JIT memory allocation test very early, even before looking at the pattern: https://github.com/PCRE2Project/pcre2/blob/pcre2-10.42/src/pcre2_jit_compile.c#L14450-L14465 But I can add a call to pcre2_pattern_info(PCRE2_INFO_JITSIZE) if you really like me to, but IMHO it's not needed. > What happened to the earlier idea of falling back > to the interpreted codepath, which will catch any bad pattern that > has "the reason for the failure" by failing anyway? Ævar's concerns about always falling back to the interpreter mode made me change the patch like this. Basically what he's concerned about are two things: 1/ "Crazy patterns" that fail the JIT but will work for the interpreter can be a serve performance regression. 2/ Always falling back to interpreter mode might mask JIT API usage errors, we'd like to see. While 1/ could also be seen as a limitation of current 'git grep', I share Ævar's extended runtime regression concerns. If, for example, some web interface offers users to supply arbitrary grep patterns, abusing the interpreter mode fallback will consume significant more CPU resources than it does right now (which simply fails with an error). > >> +static int pcre2_jit_functional(void) >> +{ >> + static int jit_working = -1; >> + pcre2_code *code; >> + size_t off; >> + int err; >> + >> + if (jit_working != -1) >> + return jit_working; >> + >> + /* >> + * Try to JIT compile a simple pattern to probe if the JIT is >> + * working in general. It might fail for systems where creating >> + * memory mappings for runtime code generation is restricted. >> + */ >> + code = pcre2_compile((PCRE2_SPTR)".", 1, 0, &err, &off, NULL); >> + if (!code) >> + return 0; >> + >> + jit_working = pcre2_jit_compile(code, PCRE2_JIT_COMPLETE) == 0; >> + pcre2_code_free(code); > > I'd prefer not having to worry about: Or it might not fail for such > systems, as the pattern is too simple and future versions of > pcre2_compile() could have special case code. See above, it's unlikely to happen. > >> @@ -317,8 +342,23 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt >> pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on); >> if (p->pcre2_jit_on) { >> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE); >> - if (jitret) >> + if (jitret == PCRE2_ERROR_NOMEMORY && !pcre2_jit_functional()) { >> + /* >> + * Even though pcre2_config(PCRE2_CONFIG_JIT, ...) >> + * indicated JIT support, the library might still >> + * fail to generate JIT code for various reasons, >> + * e.g. when SELinux's 'deny_execmem' or PaX's >> + * MPROTECT prevent creating W|X memory mappings. >> + * >> + * Instead of faling hard, fall back to interpreter >> + * mode, just as if the pattern was prefixed with >> + * '(*NO_JIT)'. >> + */ >> + p->pcre2_jit_on = 0; >> + return; > > Yes, the "instead of failing hard, fall back" makes sense. Just > that I do not see why the runtime test is a good thing to have. It prevents the fallback from being abused and introducing new regressions. So it's good to have. > In > short, we are not in the business of catching bugs in pcre2_jit > implementations, so if they say they cannot compile the pattern (I > would even say I doubt the point of checking the return code to > ensure it is NOMEMORY), it would be fine to let the interpreter > codepath to inspect the pattern and diagnose problems with it, or > take the slow match without JIT. Yeah, unfortunately they're not gonna fix what's a bug, IMHO. They think it's a feature: https://github.com/PCRE2Project/pcre2/pull/157 Anyhow, the error code is very well documented, see pcre2_jit_compile(3) """ [...] The function can also return PCRE2_ERROR_NOMEMORY if JIT is unable to allocate executable memory for the compiler, even if it was because of a system security restriction. """ And that's very much in line with what the test in pcre2_jit_compile()'s current implementation does. > What am I missing? Please have a look at Ævar's reasoning here: https://lore.kernel.org/git/221220.86bknxwy9t.gmgdl@xxxxxxxxxxxxxxxxxxx/ Thanks, Mathias
