[0/3] grep: PCRE JIT fixes

Message

Ævar Arnfjörð Bjarmason July 24, 2019, 3:14 p.m. UTC

There's a couple of patches fixing mistakes in the JIT code I added
for PCRE in <[e-mail]> and
<[e-mail]>

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

Comments

Junio C Hamano July 24, 2019, 4:18 p.m. UTC | #1

Ævar Arnfjörð Bjarmason  <[e-mail]> writes:

> There's a couple of patches fixing mistakes in the JIT code I added
> for PCRE in <[e-mail]> and
> <[e-mail]>
>
> 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.

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.

> Æ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 July 24, 2019, 8:03 p.m. UTC | #2

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

> Ævar Arnfjörð Bjarmason  <[e-mail]> writes:
>
>> There's a couple of patches fixing mistakes in the JIT code I added
>> for PCRE in <[e-mail]> and
>> <[e-mail]>
>>
>> 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.
>
> 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 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.

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