Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator

[Carlo Arenas <carenas@gmail.com>]

On Tue, Aug 27, 2019 at 2:07 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Unfortunately, this is _still_ incorrect.

I know, and that is why we worked out a v6 RC with Rene than then was
pushed to github[0] and validated to work thanks to your integration
as detailed in [1], I never got to send an update to the list though
because Rene wanted my squashing into his patch to be independent as
detailed there and because I assumed that when Junio didn't pull my
reroll, it was probably because your tree had a fix already anyway.

the recent discovery that xmalloc wasn't thread safe though,
complicates things further and that is why I was expecting to reroll
this on top of both ab/pcre-jit-fixes and jk/drop-release-pack-memory
(later one already in next) as detailed in [2]

> I pointed out multiple times that custom allocators can be activated at
> run-time rather than compile-time, therefore making the choice at
> compile-time is wrong. Besides, there is nothing specific to nedmalloc
> about this. So the patch is double-wrong on that account.

Just to clarify, I think my patch accounts for that (haven't tested
that assumption, but will do now that I have a windows box, probably
even with mi-alloc) but yes, the only reason why there were references
to NEDMALLOC was to isolate the code and make sure the fix was
tackling the problem, it was not my intention to do so at the end,
specially once we agreed that xmalloc should be used anyway.

> The patch has a yet even more immediate problem: t7816.48 is failing in
> the CI build for _weeks_ now: it requires that global context to be
> initialized, but no code path hits the initialization, resulting in a
> very, very ugly:
>
>         BUG: grep.c:516: pcre2_global_context uninitialized
>
> See
> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=15151&view=ms.vss-test-web.build-test-results-tab&runId=41282&resultId=101710&paneView=debug
> for details.

IMHO this is probably testing V3 (from pu) and which was hopefully
fixed in the last github force push for my branch

> All of this could be easily avoided. As I had pointed out already, the
> obvious fragility is not worth the optimization, and we should just
> initialize the global context always, as does this patch
> (https://github.com/git-for-windows/git/commit/5e5b959169e6efee73e0b50e464166822b7d2d07).

ironically only found out about that patch after I got a windows box
(running Windows Home though) and had finished testing my own squashed
fix[3] that has succeeded being validated in GitHub

apologize for the delays, and will be fine using your squash, mine,
the V6 RC (my preference) or dropping this series from pu if that
would help clear the ugliness of pu for windows

hopefully this won't be repeated now that I am aware of github's
integration and have my own (albeit very slow) windows environment as
well.

Carlo

[0] https://github.com/git/git/commit/0ca5d0550c17a68d83b8922b71aeff891958ed0e
[1] https://public-inbox.org/git/CAPUEspiFuvgMQ3W1se1B=8aTTrQsJSZTyQTzG1TpiyN8HTOpkA@mail.gmail.com/
[2] https://public-inbox.org/git/CAPUEspg9F7RutCUCoRAAXmRePjiunq3-zG7cN3uz_t5DVMxP=g@mail.gmail.com/
[3] https://github.com/git/git/pull/627