git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Carlo Arenas <carenas@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org, avarab@gmail.com, gitster@pobox.com,
	l.s.r@web.de, michal.kiedrowicz@gmail.com
Subject: Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator
Date: Tue, 27 Aug 2019 04:51:14 -0700
Message-ID: <CAPUEspjJNSrJQT7xV2fsdp2t5odW5fzzPdDxuar_5x_JPUtf6Q@mail.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.1908271057280.46@tvgsbejvaqbjf.bet>

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

  reply	other threads:[~2019-08-27 11:51 UTC|newest]

Thread overview: 67+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-05 11:51 [PATCH 0/1] Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines Johannes Schindelin via GitGitGadget
2019-08-05 11:51 ` [PATCH 1/1] pcre2: allow overriding the system allocator Johannes Schindelin via GitGitGadget
2019-08-05 16:19   ` Carlo Arenas
2019-08-05 16:27     ` Carlo Arenas
2019-08-05 19:32       ` Johannes Schindelin
2019-08-05 19:26     ` Johannes Schindelin
2019-08-05 21:53     ` Junio C Hamano
2019-08-06  6:24       ` Carlo Arenas
2019-08-06  8:50       ` [PATCH 0/3] grep: no leaks (WIP) Carlo Marcelo Arenas Belón
2019-08-06  8:50         ` [PATCH 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-08 13:54           ` Johannes Schindelin
2019-08-08 15:19             ` Carlo Arenas
2019-08-06  8:50         ` [PATCH 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-08 13:56           ` Johannes Schindelin
2019-08-08 14:32             ` Carlo Arenas
2019-08-06  8:50         ` [PATCH 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-06 16:36         ` [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed) Carlo Marcelo Arenas Belón
2019-08-06 16:36           ` [RFC PATCH v3 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-06 16:36           ` [RFC PATCH v3 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-07  5:38             ` René Scharfe
2019-08-07  9:49               ` Carlo Arenas
2019-08-07 13:02                 ` René Scharfe
2019-08-07 13:08                   ` [PATCH 1/2] nedmalloc: do assignments only after the declaration section René Scharfe
2019-08-07 13:09                   ` [PATCH 2/2] nedmalloc: avoid compiler warning about unused value René Scharfe
2019-08-08  2:35                   ` [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator Carlo Arenas
2019-08-08  7:07                     ` René Scharfe
2019-08-08 12:38                       ` Carlo Arenas
2019-08-08 14:29                         ` René Scharfe
2019-08-08 20:18                           ` Johannes Schindelin
2019-08-07 18:15                 ` Junio C Hamano
2019-08-06 16:36           ` [RFC PATCH v3 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-06 16:48           ` [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed) Junio C Hamano
2019-08-07 21:39           ` [RFC PATCH v4 " Carlo Marcelo Arenas Belón
2019-08-07 21:39             ` [RFC PATCH v4 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-07 21:39             ` [RFC PATCH v4 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-07 22:28               ` Junio C Hamano
2019-08-07 21:39             ` [RFC PATCH v4 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-09  3:02             ` [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes Carlo Marcelo Arenas Belón
2019-08-09  3:02               ` [RFC PATCH v5 1/3] grep: make PCRE1 aware of custom allocator Carlo Marcelo Arenas Belón
2019-08-09  3:02               ` [RFC PATCH v5 2/3] grep: make PCRE2 " Carlo Marcelo Arenas Belón
2019-08-27  9:07                 ` Johannes Schindelin
2019-08-27 11:51                   ` Carlo Arenas [this message]
2019-10-03  5:02                     ` Junio C Hamano
2019-10-03  8:08                       ` Johannes Schindelin
2019-10-03 11:17                         ` Carlo Arenas
2019-10-03 18:23                           ` Johannes Schindelin
2019-10-03 22:57                           ` Junio C Hamano
2019-08-09  3:02               ` [RFC PATCH v5 3/3] grep: avoid leak of chartables in PCRE2 Carlo Marcelo Arenas Belón
2019-08-09 11:24               ` [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes Carlo Arenas
2019-08-09 17:01                 ` René Scharfe
2019-08-09 17:46                   ` Junio C Hamano
2019-08-09 21:26                   ` Johannes Schindelin
2019-08-10  3:03                     ` [PATCH] SQUASH Carlo Marcelo Arenas Belón
2019-08-10  7:57                       ` René Scharfe
2019-08-10  8:42                         ` Carlo Arenas
2019-08-10 19:47                           ` René Scharfe
2019-08-12  7:35                             ` Carlo Arenas
2019-08-12 12:14                               ` René Scharfe
2019-08-12 12:28                                 ` Carlo Arenas
2019-08-10 13:57                       ` Johannes Schindelin
2019-08-10  3:05                     ` [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes Carlo Arenas
2019-08-10  7:56                       ` René Scharfe
2019-08-10 12:40                         ` Carlo Arenas
2019-08-10 21:16                           ` René Scharfe
2019-08-08 20:21           ` [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed) Johannes Schindelin
2019-08-09  6:52             ` Carlo Arenas
2019-08-09 21:13               ` Johannes Schindelin

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=CAPUEspjJNSrJQT7xV2fsdp2t5odW5fzzPdDxuar_5x_JPUtf6Q@mail.gmail.com \
    --to=carenas@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=michal.kiedrowicz@gmail.com \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git