[PATCH 0/1] Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines

[PATCH 0/1] Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines

[Johannes Schindelin via GitGitGadget]

For a couple of days, maybe even a week, pu fails consistently, in the
Windows job where it tests t7816. The symptom is a segmentation fault.

I finally got to diagnose this, and it looked at first as if there was yet
another buffer overrun that was so small that valgrind failed to detect it
(see https://github.com/gitgitgadget/git/pull/178). The problem is another
one, though: we ask PCRE2 to allocate a table (and it uses the system
allocator for that), and then try to release it using nedmalloc's free() 
replacement.

This is squarely a problem with cb/pcre2-chartables-leakfix in conjunction
with an overridden allocator.

Junio, for your convenience, I rebased this patch directly on top of 
ab/pcre-v2 and pushed out the let-pcre2-respect-nedmalloc branch at 
https://github.com/dscho/git ready to be pulled. The rebased version is not
technically a bug fix, as I do not see any way that ab/pcre-v2 uses
mismatched allocators for malloc()/free(), but just in case that you wanted
to have it in v2.23.0 and not on top cb/pcre2-chartables-leakfix...

Johannes Schindelin (1):
  pcre2: allow overriding the system allocator

 grep.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)


base-commit: 7d3bf76999452ff64c84cec25fd95a7a95744b78
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-306%2Fdscho%2Ffix-for-pcre-chartables-leakfix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-306/dscho/fix-for-pcre-chartables-leakfix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/306
-- 
gitgitgadget

[PATCH 1/1] pcre2: allow overriding the system allocator

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),
we try to release the UTF-8 tables obtained via `pcre2_maketables()`. To
do that, we use the function `free()`. That is all fine and dandy as
long as that refers to the system allocator.

However, when we compile Git with `USE_NED_ALLOCATOR` (notably on
Windows), then `free()` actually calls `nedfree()`. But
`pcre2_maketables()` allocated the tables using the system allocator
because we did not tell it to use nedmalloc instead.

This leads to segmentation faults when the UTF-8 tables are released,
most notably in the `t7816-grep-binary-pattern.sh` test script.

PCRE2 does have an option to override the allocator it should use, and
this patch calls upon it.

As there are other ways to override the system allocator than
`USE_NED_ALLOCATOR`, we choose to specify the allocator we want to use
specifically, even if we still use the system allocator.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 grep.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index d4c5d464ad..d6d29fc724 100644
--- a/grep.c
+++ b/grep.c
@@ -482,6 +482,27 @@ static void free_pcre1_regexp(struct grep_pat *p)
 #endif /* !USE_LIBPCRE1 */
 
 #ifdef USE_LIBPCRE2
+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
+{
+	return malloc(size);
+}
+
+static void pcre2_free(void *pointer, void *memory_data)
+{
+	return free(pointer);
+}
+
+static pcre2_general_context *get_pcre2_context(void)
+{
+	static pcre2_general_context *context;
+
+	if (!context)
+		context = pcre2_general_context_create(pcre2_malloc,
+						       pcre2_free, NULL);
+
+	return context;
+}
+
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
 {
 	int error;
@@ -498,8 +519,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			p->pcre2_tables = pcre2_maketables(NULL);
-			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
+			p->pcre2_tables = pcre2_maketables(get_pcre2_context());
+			p->pcre2_compile_context =
+				pcre2_compile_context_create(get_pcre2_context());
 			pcre2_set_character_tables(p->pcre2_compile_context,
 							p->pcre2_tables);
 		}
@@ -513,7 +535,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 					 p->pcre2_compile_context);
 
 	if (p->pcre2_pattern) {
-		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);
+		p->pcre2_match_data =
+			pcre2_match_data_create_from_pattern(p->pcre2_pattern, get_pcre2_context());
 		if (!p->pcre2_match_data)
 			die("Couldn't allocate PCRE2 match data");
 	} else {
@@ -550,7 +573,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}
 
-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, get_pcre2_context());
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
 		p->pcre2_match_context = pcre2_match_context_create(NULL);
-- 
gitgitgadget

Re: [PATCH 1/1] pcre2: allow overriding the system allocator

[Carlo Arenas]

On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),
> we try to release the UTF-8 tables obtained via `pcre2_maketables()`. To
> do that, we use the function `free()`. That is all fine and dandy as
> long as that refers to the system allocator.

Sorry; I should have thought of this, but assumed was safe since it
would be broken
the same way with PCRE1.

Presume git in windows only builds against PCRE2?

LGTM except from the suggestion below that might make the code more "standard"
and probably be a good base for a similar PCRE1 fix
>
> +static pcre2_general_context *get_pcre2_context(void)
> +{
> +       static pcre2_general_context *context;
> +
> +       if (!context)
> +               context = pcre2_general_context_create(pcre2_malloc,
> +                                                      pcre2_free, NULL);
> +
> +       return context;
> +}

instead of using a static variable inside this helper function it
might be better to use
one extra field inside the (struct grep_pat *p), where all other
variables are kept

Additionally to being more consistent will avoid creating the global
context for the
most common case (when the locale is either C/POSIX or UTF-8) and therefore
have a smaller impact on performance.

Carlo

Re: [PATCH 1/1] pcre2: allow overriding the system allocator

[Carlo Arenas]

And forgot to mention that technically these are not UTF-8 tables but
single byte tables like for example the ones used with en_US.ISO8859-1

Carlo

Re: [PATCH 1/1] pcre2: allow overriding the system allocator

[Johannes Schindelin]

Hi Carlo,

On Mon, 5 Aug 2019, Carlo Arenas wrote:

> And forgot to mention that technically these are not UTF-8 tables but
> single byte tables like for example the ones used with en_US.ISO8859-1

Thank you for pointing that out, I completely missed that.

Junio, do you want me to re-send, or can you touch up the commit message
while applying?

Ciao,
Dscho

Re: [PATCH 1/1] pcre2: allow overriding the system allocator

[Johannes Schindelin]

Hi Carlo,

On Mon, 5 Aug 2019, Carlo Arenas wrote:

> On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),
> > we try to release the UTF-8 tables obtained via `pcre2_maketables()`. To
> > do that, we use the function `free()`. That is all fine and dandy as
> > long as that refers to the system allocator.
>
> Sorry; I should have thought of this, but assumed was safe since it
> would be broken
> the same way with PCRE1.
>
> Presume git in windows only builds against PCRE2?

That's right, we only build against PCRE2.

> LGTM except from the suggestion below that might make the code more "standard"
> and probably be a good base for a similar PCRE1 fix
> >
> > +static pcre2_general_context *get_pcre2_context(void)
> > +{
> > +       static pcre2_general_context *context;
> > +
> > +       if (!context)
> > +               context = pcre2_general_context_create(pcre2_malloc,
> > +                                                      pcre2_free, NULL);
> > +
> > +       return context;
> > +}
>
> instead of using a static variable inside this helper function it
> might be better to use one extra field inside the (struct grep_pat
> *p), where all other variables are kept

My thinking about that was that this would add more code, and thus more
opportunities to introduce bugs. Also, it's not like the general context
really has any _state_ per se. It just registers the current allocator,
which we want to assume is constant over the life-time of the process.

So does it really make sense to create one general context per grep
pattern? (Or even per grep pattern and thread?)

> Additionally to being more consistent will avoid creating the global
> context for the most common case (when the locale is either C/POSIX or
> UTF-8) and therefore have a smaller impact on performance.

Given that my patch does _less_ than what you suggest (it really only
creates at most _one_ general context per process, not one per
internal-grep invocation, possibly per-thread, and it also never calls
`pcre2_general_context_free()`), I highly doubt that your proposed
version would have a _smaller_ impact on performance.

Ciao,
Dscho

Re: [PATCH 1/1] pcre2: allow overriding the system allocator

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> LGTM except from the suggestion below that might make the code more "standard"
> and probably be a good base for a similar PCRE1 fix
>>
>> +static pcre2_general_context *get_pcre2_context(void)
>> +{
>> +       static pcre2_general_context *context;
>> +
>> +       if (!context)
>> +               context = pcre2_general_context_create(pcre2_malloc,
>> +                                                      pcre2_free, NULL);
>> +
>> +       return context;
>> +}
>
> instead of using a static variable inside this helper function it
> might be better to use
> one extra field inside the (struct grep_pat *p), where all other
> variables are kept
>
> Additionally to being more consistent will avoid creating the global

"standard" and "more consistent" are good things, but I am not sure
I should agree with the argument without knowing what you are
comparing your suggested improvement with.  Whose standard practice
are we trying to be consistent with?  Keeping dynamic resources hooked
to "struct grep_pat" so that (1) different patterns could use different
settings when they desire and (2) the resources are not hidden behind
a function-scope static and can be discarded when we are done with
the pattern, which is the standard in our "grep" subsystem?

I think general context probably corresponds to a bit higher level
than individual grep_pat.  E.g. when running "grep -e foo -e bar",
do we expect resources needed by patterns "foo" and "bar" would want
to be allocated and freed by potentially separate <alloc,free>
function pairs?

> context for the
> most common case (when the locale is either C/POSIX or UTF-8) and therefore
> have a smaller impact on performance.

I am not sure about the impact on performance, but if it helps us
keep the subsystem reusable by avoiding function-scope static that
we cannot clear, that would be a good thing.  But "struct grep_pat"
may be a bit too fine-grained to control general context.

Re: [PATCH 1/1] pcre2: allow overriding the system allocator

[Carlo Arenas]

On Mon, Aug 5, 2019 at 2:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Arenas <carenas@gmail.com> writes:
>
> > LGTM except from the suggestion below that might make the code more "standard"
> > and probably be a good base for a similar PCRE1 fix
> >>
> >> +static pcre2_general_context *get_pcre2_context(void)
> >> +{
> >> +       static pcre2_general_context *context;
> >> +
> >> +       if (!context)
> >> +               context = pcre2_general_context_create(pcre2_malloc,
> >> +                                                      pcre2_free, NULL);
> >> +
> >> +       return context;
> >> +}
> >
> > instead of using a static variable inside this helper function it
> > might be better to use
> > one extra field inside the (struct grep_pat *p), where all other
> > variables are kept
> >
> > Additionally to being more consistent will avoid creating the global
>
> "standard" and "more consistent" are good things, but I am not sure
> I should agree with the argument without knowing what you are
> comparing your suggested improvement with.  Whose standard practice
> are we trying to be consistent with?  Keeping dynamic resources hooked
> to "struct grep_pat" so that (1) different patterns could use different
> settings when they desire and (2) the resources are not hidden behind
> a function-scope static and can be discarded when we are done with
> the pattern, which is the standard in our "grep" subsystem?

It was my impression that we were abusing the struct grep_pat to avoid
having to deal properly with threading and interlocks.

I agree my wording wasn't clear enough and my hinting a little obscure
but the original code is racy and it wouldn't be if the "global context" will be
initialized/maintained there; as an added benefit it will be straight forward to
clear (together with the rest)

I am not advocating that as a good design, but also think the code will be
shorter (which was another rationale for the proposed change, to avoid
introducing yet more bugs and since it was even suggested for inclusion
in the next release)

> I think general context probably corresponds to a bit higher level
> than individual grep_pat.  E.g. when running "grep -e foo -e bar",
> do we expect resources needed by patterns "foo" and "bar" would want
> to be allocated and freed by potentially separate <alloc,free>
> function pairs?

no with a different design; but currently even if almost all the time
we have the same pattern for all workers (ex: -e foo), why
are we doing the compilation (plus JIT translation) and creating this
table and all other context pointers (plus a jit stack) once per
thread?

just so we can move forward with a better design will send a proposed
patch that does things
a little be better as an RFC

> > context for the
> > most common case (when the locale is either C/POSIX or UTF-8) and therefore
> > have a smaller impact on performance.
>
> I am not sure about the impact on performance, but if it helps us
> keep the subsystem reusable by avoiding function-scope static that
> we cannot clear, that would be a good thing.  But "struct grep_pat"
> may be a bit too fine-grained to control general context.

the "performance" point I was making was that with the current code
the chartable is only created when it is strictly needed (meaning the
pattern/haystack will do matching in non UTF-8 mode but with characters
with code higher than 127 and therefore MUST agree in a codepage)

most of the time (like when using UTF-8) the chartable (and therefore the
global context) is not needed (even when using alternate allocators)

there is a chance that PCRE2 might perform better with NED, but not
in my system and since we haven't been using NED with PCRE2 until
this proposed change it might be better to do that independently anyway.

Carlo

[PATCH 0/3] grep: no leaks (WIP)

[Carlo Marcelo Arenas Belón]

This is an incomplete reroll for cb/pcre2-chartables-leakfix that attempts
to address the root cause of the problem reported[1] by Dscho with PCRE2
and that is that until now PCRE and NED never worked together.

The first patch is likely correct but since I'd been unable to replicate
the issue I can't be completely sure, if a kind soul with access to a
windows developer environment could give it a try we will know for sure
but FWIW it runs fine and doesn't introduce any failures in our tests

The second patch is obviously incomplete but should address the problem
reported; there are still more things to consider as explained there

The third patch is the original leak patch rebased on top.

Carlo Marcelo Arenas Belón (3):
  grep: make PCRE1 aware of custom allocator
  grep: make PCRE2 aware of custom allocator
  grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 grep.c         | 46 ++++++++++++++++++++++++++++++++++++++++++----
 grep.h         |  2 ++
 4 files changed, 46 insertions(+), 5 deletions(-)

[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/
-- 
2.23.0.rc1

[PATCH 1/3] grep: make PCRE1 aware of custom allocator

[Carlo Marcelo Arenas Belón]

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
to override the system alocator, and so it is incompatible with
USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported

[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bd246f2989..4b384f3759 100644
--- a/Makefile
+++ b/Makefile
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif
diff --git a/grep.c b/grep.c
index cd952ef5d3..0154998695 100644
--- a/grep.c
+++ b/grep.c
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;
-- 
2.23.0.rc1

Re: [PATCH 1/3] grep: make PCRE1 aware of custom allocator

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2317 bytes --]

Hi Carlo,

On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
> to override the system alocator, and so it is incompatible with
> USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)
>
> Make the minimum change possible to ensure this combination is supported
>
> [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  Makefile |  2 +-
>  grep.c   | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index bd246f2989..4b384f3759 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
>  endif
>
>  ifdef USE_NED_ALLOCATOR
> -	COMPAT_CFLAGS += -Icompat/nedmalloc
> +	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc

This pretends that all custom allocators are selected at build time,
something I tried to stress in my commit message as not true. You can
pre-load not only nedmalloc, but also jemalloc and unless I am mistaken
also tcmalloc. And mi-malloc.

So the premise of this patch, that you can tell at compile time that a
different allocator than the system one will be in use is simply
incorrect.

Ciao,
Dscho

>  	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
>  	OVERRIDE_STRDUP = YesPlease
>  endif
> diff --git a/grep.c b/grep.c
> index cd952ef5d3..0154998695 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
>   * Initialize one instance of grep_opt and copy the
>   * default values from the template we read the configuration
>   * information in an earlier call to git_config(grep_config).
> + *
> + * If using PCRE make sure that the library is configured
> + * to use the right allocator (ex: NED)
>   */
>  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
>  {
>  	struct grep_opt *def = &grep_defaults;
>  	int i;
>
> +#ifdef USE_NED_ALLOCATOR
> +#ifdef USE_LIBPCRE1
> +	pcre_malloc = malloc;
> +	pcre_free = free;
> +#endif
> +#endif
> +
>  	memset(opt, 0, sizeof(*opt));
>  	opt->repo = repo;
>  	opt->prefix = prefix;
> --
> 2.23.0.rc1
>
>

Re: [PATCH 1/3] grep: make PCRE1 aware of custom allocator

[Carlo Arenas]

On Thu, Aug 8, 2019 at 6:55 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:
>
> > 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
> > to override the system allocator, and so it is incompatible with
> > USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)
> >
> > Make the minimum change possible to ensure this combination is supported
> >
> > [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com
> >
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > ---
> >  Makefile |  2 +-
> >  grep.c   | 10 ++++++++++
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index bd246f2989..4b384f3759 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
> >  endif
> >
> >  ifdef USE_NED_ALLOCATOR
> > -     COMPAT_CFLAGS += -Icompat/nedmalloc
> > +     COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
>
> This pretends that all custom allocators are selected at build time,
> something I tried to stress in my commit message as not true. You can
> pre-load not only nedmalloc, but also jemalloc and unless I am mistaken
> also tcmalloc. And mi-malloc.

the patch only fixes the case when NED was chosen at build time, which
before this patch meant that the system allocator was used by PCRE1
and NED by git.

That combination happened to work unless any pointer crossed over but
neglected any benefit of using NED instead of the system allocator
with PCRE1.

the PCRE1 API was therefore safer to use, PCRE2 is not and that will
be addressed[1] in a future version by making sure this "layering
violation" is covered

runtime custom allocator changes are not affected and will keep
working as usual, which (at least in Linux when using LD_PRELOAD which
is probably what you are refer to by "preload") means that the dynamic
linker will replace all calls to malloc/free with the ones you
provided at exec time and before it starts the binary (including
whichever libraries it loaded) and therefore will make both git and
PCRE2 to use the same allocator.

nedmalloc seems (at least in Linux) to be smart enough to cope with
pointers that were not allocated by itself and doesn't crash so
presume there is something else in Windows (maybe the dynamic linker,
if there is one) that might be also part of the reason for this
segfault.

of course, the trigger of this is my patch, so I'll take the blame for
this bug regardless and I am looking forward to a fix

> So the premise of this patch, that you can tell at compile time that a
> different allocator than the system one will be in use is simply
> incorrect.

in the context that the only available (and integrated with our build
system) allocator is NED at compile time, I think it is correct.
you are correct though that if another allocator will be added as a
compile time option, we will need to do something similar as well.

Carlo

[1] https://bugs.exim.org/show_bug.cgi?id=2429

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

[Carlo Marcelo Arenas Belón]

Most of the code stolen from[1] to easy on comparison and including
the deficiency of setting the global context even for patterns that
won't need it.

Ideally, the call from grep_init could be moved to a place where it
could be set without needing a lock and at least with this approach
we have a place to clear it (which is obviously missing more callers,
but at least shows how it will look for the grep subcommand)

I had also dropped most other users of the global context in a failed
attempt to make the change smaller, but also to keep the current
behaviour so that we could see the effect of enabling NED for PCRE2
more clearly.

Sadly, that will likely require a Windows box, as NED (at least our
version) is horribly broken in macOS (maybe it wasn't 64 bit clean)
and in Linux builds, but I can't reproduce your crasher and it is
most likely slower than the system malloc.

[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/

Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/grep.c |  1 +
 grep.c         | 31 +++++++++++++++++++++++++++++--
 grep.h         |  1 +
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 560051784e..e49c20df60 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }
diff --git a/grep.c b/grep.c
index 0154998695..e748a6d68c 100644
--- a/grep.c
+++ b/grep.c
@@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,
 
 static struct grep_opt grep_defaults;
 
+#ifdef USE_LIBPCRE2
+static pcre2_general_context *pcre2_global_context;
+
+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
+{
+	return malloc(size);
+}
+
+static void pcre2_free(void *pointer, void *memory_data)
+{
+	return free(pointer);
+}
+#endif
+
 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
 	[GREP_COLOR_FILENAME]	    = "filename",
@@ -153,6 +167,7 @@ int grep_config(const char *var, const char *value, void *cb)
  *
  * If using PCRE make sure that the library is configured
  * to use the right allocator (ex: NED)
+ * if any object is created it should be cleaned up in grep_destroy()
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
@@ -164,6 +179,10 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	pcre_malloc = malloc;
 	pcre_free = free;
 #endif
+#ifdef USE_LIBPCRE2
+	pcre2_global_context = pcre2_general_context_create(pcre2_malloc,
+							pcre2_free, NULL);
+#endif
 #endif
 
 	memset(opt, 0, sizeof(*opt));
@@ -188,6 +207,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 		color_set(opt->colors[i], def->colors[i]);
 }
 
+void grep_destroy(void)
+{
+#ifdef USE_LIBPCRE2
+	pcre2_general_context_free(pcre2_global_context);
+#endif
+}
+
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	/*
@@ -509,7 +535,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
+			character_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}
@@ -560,7 +586,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}
 
-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024,
+					pcre2_global_context);
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
 		p->pcre2_match_context = pcre2_match_context_create(NULL);
diff --git a/grep.h b/grep.h
index 1875880f37..526c2db9ef 100644
--- a/grep.h
+++ b/grep.h
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
-- 
2.23.0.rc1

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5228 bytes --]

Hi Carlo,

On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> Most of the code stolen from[1] to easy on comparison and including
> the deficiency of setting the global context even for patterns that
> won't need it.
>
> Ideally, the call from grep_init could be moved to a place where it
> could be set without needing a lock and at least with this approach
> we have a place to clear it (which is obviously missing more callers,
> but at least shows how it will look for the grep subcommand)
>
> I had also dropped most other users of the global context in a failed
> attempt to make the change smaller, but also to keep the current
> behaviour so that we could see the effect of enabling NED for PCRE2
> more clearly.
>
> Sadly, that will likely require a Windows box, as NED (at least our
> version) is horribly broken in macOS (maybe it wasn't 64 bit clean)
> and in Linux builds, but I can't reproduce your crasher and it is
> most likely slower than the system malloc.
>
> [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/
>
> Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Actually not so much suggested by me, as your patch still causes
crashes (mine didn't):

https://dev.azure.com/gitgitgadget/git/_build/results?buildId=13935&view=ms.vss-test-web.build-test-results-tab

Ciao,
Dscho

> ---
>  builtin/grep.c |  1 +
>  grep.c         | 31 +++++++++++++++++++++++++++++--
>  grep.h         |  1 +
>  3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 560051784e..e49c20df60 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		run_pager(&opt, prefix);
>  	clear_pathspec(&pathspec);
>  	free_grep_patterns(&opt);
> +	grep_destroy();
>  	return !hit;
>  }
> diff --git a/grep.c b/grep.c
> index 0154998695..e748a6d68c 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,
>
>  static struct grep_opt grep_defaults;
>
> +#ifdef USE_LIBPCRE2
> +static pcre2_general_context *pcre2_global_context;
> +
> +static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)
> +{
> +	return malloc(size);
> +}
> +
> +static void pcre2_free(void *pointer, void *memory_data)
> +{
> +	return free(pointer);
> +}
> +#endif
> +
>  static const char *color_grep_slots[] = {
>  	[GREP_COLOR_CONTEXT]	    = "context",
>  	[GREP_COLOR_FILENAME]	    = "filename",
> @@ -153,6 +167,7 @@ int grep_config(const char *var, const char *value, void *cb)
>   *
>   * If using PCRE make sure that the library is configured
>   * to use the right allocator (ex: NED)
> + * if any object is created it should be cleaned up in grep_destroy()
>   */
>  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
>  {
> @@ -164,6 +179,10 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
>  	pcre_malloc = malloc;
>  	pcre_free = free;
>  #endif
> +#ifdef USE_LIBPCRE2
> +	pcre2_global_context = pcre2_general_context_create(pcre2_malloc,
> +							pcre2_free, NULL);
> +#endif
>  #endif
>
>  	memset(opt, 0, sizeof(*opt));
> @@ -188,6 +207,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
>  		color_set(opt->colors[i], def->colors[i]);
>  }
>
> +void grep_destroy(void)
> +{
> +#ifdef USE_LIBPCRE2
> +	pcre2_general_context_free(pcre2_global_context);
> +#endif
> +}
> +
>  static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
>  {
>  	/*
> @@ -509,7 +535,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>
>  	if (opt->ignore_case) {
>  		if (has_non_ascii(p->pattern)) {
> -			character_tables = pcre2_maketables(NULL);
> +			character_tables = pcre2_maketables(pcre2_global_context);
>  			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
>  			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
>  		}
> @@ -560,7 +586,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>  			return;
>  		}
>
> -		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
> +		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024,
> +					pcre2_global_context);
>  		if (!p->pcre2_jit_stack)
>  			die("Couldn't allocate PCRE2 JIT stack");
>  		p->pcre2_match_context = pcre2_match_context_create(NULL);
> diff --git a/grep.h b/grep.h
> index 1875880f37..526c2db9ef 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -189,6 +189,7 @@ struct grep_opt {
>  void init_grep_defaults(struct repository *);
>  int grep_config(const char *var, const char *value, void *);
>  void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
> +void grep_destroy(void);
>  void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
>
>  void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
> --
> 2.23.0.rc1
>
>

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

[Carlo Arenas]

On Thu, Aug 8, 2019 at 6:57 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:
> > Suggested-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Actually not so much suggested by me, as your patch still causes
> crashes (mine didn't):

the "equivalent" version in v4[1], that is still not on pu shouldn't,
if my hunch is correct

it also shouldn't crash if the test wouldn't be doing LC_ALL="C" but
we have yet a couple of extra bugs related to that, including one[2] I
CC you on which would be nice to get validated as well.

Carlo

[1] https://public-inbox.org/git/20190807213945.10464-3-carenas@gmail.com/
[2] https://public-inbox.org/git/20190807095322.8988-1-carenas@gmail.com/

[PATCH 3/3] grep: avoid leak of chartables in PCRE2

[Carlo Marcelo Arenas Belón]

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced
a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from
the original change and free the generated table just like it is done
for PCRE1.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index e748a6d68c..3e14ec91a6 100644
--- a/grep.c
+++ b/grep.c
@@ -524,7 +524,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;
@@ -535,9 +534,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}
@@ -642,6 +642,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
diff --git a/grep.h b/grep.h
index 526c2db9ef..533165c21a 100644
--- a/grep.h
+++ b/grep.h
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;
-- 
2.23.0.rc1

[RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)

[Carlo Marcelo Arenas Belón]

This series is a candidate reroll for cb/pcre2-chartables-leakfix, that
hopefully addresses the root cause of the problem reported by Dscho in
Windows, where the PCRE2 library wasn't aware of the custom allocator and
was returning a pointer created with the system malloc but passing it to
NED's free, resulting in a segfault.

The reason why it was triggered by the original leak fix is the layering
violation reported by René and that is exclusive to PCRE2 (hence why it
hasn't been reported with PCRE1).  The first patch could be droped and
then used in a different series that will fully integrate the custom
allocator with the PCRE library and that is currently missing with PCRE2.

Eitherway, since I am unable to replicate the original bug or take
performance numbers in a representative environment without Windows
this is only published as an RFC, eventhough it has been tested and
considered mostly complete.

Junio, could you comment in my assumption that the use of grep in
revision.c doesn't require initializing a PCRE2 global context and
therefore not doing the cleanup?

Carlo Marcelo Arenas Belón (3):
  grep: make PCRE1 aware of custom allocator
  grep: make PCRE2 aware of custom allocator
  grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 grep.c         | 51 +++++++++++++++++++++++++++++++++++++++++++++++---
 grep.h         |  2 ++
 4 files changed, 52 insertions(+), 4 deletions(-)

-- 
2.23.0.rc1

[RFC PATCH v3 1/3] grep: make PCRE1 aware of custom allocator

[Carlo Marcelo Arenas Belón]

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
to override the system alocator, and so it is incompatible with
USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported
by extending grep_init to set the PCRE1 specific functions to the NED
versions (using the aliased names though) and therefore making sure all
alocations are done inside PCRE1 with the same allocator than the rest
of git.

This change might have performance impact (hopefully for the best) and
so will be good idea to test it in a platform where NED might have a
positive impact (ex: Windows 7)

[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bd246f2989..4b384f3759 100644
--- a/Makefile
+++ b/Makefile
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif
diff --git a/grep.c b/grep.c
index cd952ef5d3..0154998695 100644
--- a/grep.c
+++ b/grep.c
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;
-- 
2.23.0.rc1

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

[Carlo Marcelo Arenas Belón]

94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include
a way to override the system allocator, and so it is incompatible with
USE_NED_ALLOCATOR.  The problem was made visible when an attempt to
avoid a leak in a data structure that is created by the library was
passed to NED's free for disposal triggering a segfault in Windows.

PCRE2 requires the use of a general context to override the allocator
and therefore, there is a lot more code needed than in PCRE1, including
a couple of wrapper functions.

Extend the grep API with a "destructor" that could be called to cleanup
any objects that were created and used globally.

Update builtin/grep to use that new API, but any other future
users should make sure to have matching grep_init/grep_destroy calls
if they are using the pattern matching functionality (currently only
relevant when using both NED and PCRE2)

Move some of the logic that was before done per thread (in the workers)
into an earlier phase to avoid degrading performance, but as the use
of PCRE2 with NED is better understood it is expected more of its
functions will be instructed to use the custom allocator as well as
was done in the original code[1] this work was based on.

[1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/

Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/grep.c |  1 +
 grep.c         | 36 +++++++++++++++++++++++++++++++++++-
 grep.h         |  1 +
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 560051784e..e49c20df60 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }
diff --git a/grep.c b/grep.c
index 0154998695..3e5bdf94a6 100644
--- a/grep.c
+++ b/grep.c
@@ -16,6 +16,22 @@ static int grep_source_is_binary(struct grep_source *gs,
 
 static struct grep_opt grep_defaults;
 
+#ifdef USE_LIBPCRE2
+static pcre2_general_context *pcre2_global_context;
+
+#ifdef USE_NED_ALLOCATOR
+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
+{
+	return malloc(size);
+}
+
+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
+{
+	return free(pointer);
+}
+#endif
+#endif
+
 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
 	[GREP_COLOR_FILENAME]	    = "filename",
@@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)
  *
  * If using PCRE make sure that the library is configured
  * to use the right allocator (ex: NED)
+ * if any object is created it should be cleaned up in grep_destroy()
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
@@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 		color_set(opt->colors[i], def->colors[i]);
 }
 
+void grep_destroy(void)
+{
+#ifdef USE_LIBPCRE2
+	pcre2_general_context_free(pcre2_global_context);
+#endif
+}
+
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	/*
@@ -319,6 +343,11 @@ void append_header_grep_pattern(struct grep_opt *opt,
 void append_grep_pattern(struct grep_opt *opt, const char *pat,
 			 const char *origin, int no, enum grep_pat_token t)
 {
+#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
+	if (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))
+		pcre2_global_context = pcre2_general_context_create(
+					pcre2_malloc, pcre2_free, NULL);
+#endif
 	append_grep_pat(opt, pat, strlen(pat), origin, no, t);
 }
 
@@ -507,9 +536,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	p->pcre2_compile_context = NULL;
 
+	/* pcre2_global_context is initialized in append_grep_pattern */
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
+#ifdef USE_NED_ALLOCATOR
+			if (!pcre2_global_context)
+				BUG("pcre2_global_context uninitialized");
+#endif
+			character_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}
diff --git a/grep.h b/grep.h
index 1875880f37..526c2db9ef 100644
--- a/grep.h
+++ b/grep.h
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
-- 
2.23.0.rc1

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

[René Scharfe]

Am 06.08.19 um 18:36 schrieb Carlo Marcelo Arenas Belón:
> 94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include
> a way to override the system allocator, and so it is incompatible with
> USE_NED_ALLOCATOR.  The problem was made visible when an attempt to
> avoid a leak in a data structure that is created by the library was
> passed to NED's free for disposal triggering a segfault in Windows.
>
> PCRE2 requires the use of a general context to override the allocator
> and therefore, there is a lot more code needed than in PCRE1, including
> a couple of wrapper functions.
>
> Extend the grep API with a "destructor" that could be called to cleanup
> any objects that were created and used globally.
>
> Update builtin/grep to use that new API, but any other future
> users should make sure to have matching grep_init/grep_destroy calls
> if they are using the pattern matching functionality (currently only
> relevant when using both NED and PCRE2)
>
> Move some of the logic that was before done per thread (in the workers)
> into an earlier phase to avoid degrading performance

Which logic is moved?  In the patch I basically only see additions.

>, but as the use
> of PCRE2 with NED is better understood it is expected more of its
> functions will be instructed to use the custom allocator as well as
> was done in the original code[1] this work was based on.
>
> [1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/

I'm not sure I understand that part.  Do you mean there are gaps of
knowledge about nedmalloc and/or PCRE2 and/or their interaction?  And
someone is working on closing those gaps, and is going to submit more
patches in the process?

Your patch uses a custom global context only if USE_NED_ALLOCATOR is
defined, while [1] does it unconditionally.  The latter is easier to
debug and requires less preprocessor directives.  What's the upside
of your approach?

>
> Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/grep.c |  1 +
>  grep.c         | 36 +++++++++++++++++++++++++++++++++++-
>  grep.h         |  1 +
>  3 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 560051784e..e49c20df60 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		run_pager(&opt, prefix);
>  	clear_pathspec(&pathspec);
>  	free_grep_patterns(&opt);
> +	grep_destroy();
>  	return !hit;
>  }
> diff --git a/grep.c b/grep.c
> index 0154998695..3e5bdf94a6 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -16,6 +16,22 @@ static int grep_source_is_binary(struct grep_source *gs,
>
>  static struct grep_opt grep_defaults;
>
> +#ifdef USE_LIBPCRE2
> +static pcre2_general_context *pcre2_global_context;
> +
> +#ifdef USE_NED_ALLOCATOR
> +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
> +{
> +	return malloc(size);

Should this be xmalloc() to get consistent out-of-memory handling?

> +}
> +
> +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
> +{
> +	return free(pointer);
> +}
> +#endif
> +#endif
> +
>  static const char *color_grep_slots[] = {
>  	[GREP_COLOR_CONTEXT]	    = "context",
>  	[GREP_COLOR_FILENAME]	    = "filename",
> @@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)
>   *
>   * If using PCRE make sure that the library is configured
>   * to use the right allocator (ex: NED)
> + * if any object is created it should be cleaned up in grep_destroy()
>   */
>  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
>  {
> @@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
>  		color_set(opt->colors[i], def->colors[i]);
>  }
>
> +void grep_destroy(void)
> +{
> +#ifdef USE_LIBPCRE2
> +	pcre2_general_context_free(pcre2_global_context);
> +#endif
> +}
> +
>  static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
>  {
>  	/*
> @@ -319,6 +343,11 @@ void append_header_grep_pattern(struct grep_opt *opt,
>  void append_grep_pattern(struct grep_opt *opt, const char *pat,
>  			 const char *origin, int no, enum grep_pat_token t)
>  {
> +#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
> +	if (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))
> +		pcre2_global_context = pcre2_general_context_create(
> +					pcre2_malloc, pcre2_free, NULL);
> +#endif
>  	append_grep_pat(opt, pat, strlen(pat), origin, no, t);
>  }
>
> @@ -507,9 +536,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>
>  	p->pcre2_compile_context = NULL;
>
> +	/* pcre2_global_context is initialized in append_grep_pattern */
>  	if (opt->ignore_case) {
>  		if (has_non_ascii(p->pattern)) {
> -			character_tables = pcre2_maketables(NULL);
> +#ifdef USE_NED_ALLOCATOR
> +			if (!pcre2_global_context)
> +				BUG("pcre2_global_context uninitialized");

[1] initializes on demand.  Why not do that?  To avoid race conditions
that would lead to occasional double-allocation of the global context?

> +#endif
> +			character_tables = pcre2_maketables(pcre2_global_context);
>  			p->pcre2_compile_context = pcre2_compile_context_create(NULL);

Don't you want to pass pcre2_global_context here as well?  And [1] even
uses it in some more places.

Oh, that's the "expected more" when "better understood" part from the
commit message, right?

Basically I'd expect the custom global context to be used for all PCRE2
allocations; I can't think of a reason for mixing allocators (e.g.
system malloc for PCRE2 regexes and nedmalloc for everything else).

>  			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
>  		}
> diff --git a/grep.h b/grep.h
> index 1875880f37..526c2db9ef 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -189,6 +189,7 @@ struct grep_opt {
>  void init_grep_defaults(struct repository *);
>  int grep_config(const char *var, const char *value, void *);
>  void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
> +void grep_destroy(void);
>  void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
>
>  void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
>

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

[Carlo Arenas]

On Tue, Aug 6, 2019 at 10:38 PM René Scharfe <l.s.r@web.de> wrote:
>
> Am 06.08.19 um 18:36 schrieb Carlo Marcelo Arenas Belón:
> > Move some of the logic that was before done per thread (in the workers)
> > into an earlier phase to avoid degrading performance
>
> Which logic is moved?  In the patch I basically only see additions.

the decision of if we want to create a global context or not, which is now being
done only once per pattern.  "moving" there was to imply the logic now
was no longer going to always use state in the struct_pat that then
will be evaluated at least once per thread, but could use state
globally and reuse some of the work (as I did for the generation of
the chartables in a later patch, that I had yet to send)

> >, but as the use
> > of PCRE2 with NED is better understood it is expected more of its
> > functions will be instructed to use the custom allocator as well as
> > was done in the original code[1] this work was based on.
> >
> > [1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/
>
> I'm not sure I understand that part.  Do you mean there are gaps of
> knowledge about nedmalloc and/or PCRE2 and/or their interaction?  And
> someone is working on closing those gaps, and is going to submit more
> patches in the process?
>
> Your patch uses a custom global context only if USE_NED_ALLOCATOR is
> defined, while [1] does it unconditionally.  The latter is easier to
> debug and requires less preprocessor directives.  What's the upside
> of your approach?

was hoping will perform better but it seems that testing can be done
only in windows

> > +#ifdef USE_NED_ALLOCATOR
> > +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
> > +{
> > +     return malloc(size);
>
> Should this be xmalloc() to get consistent out-of-memory handling?

good point, note though that since it is inside a USE_NED_ALLOCATOR
ifdef it is really
nedalloc in disguise
>
> > +}
> > +
> > +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
> > +{
> > +     return free(pointer);
> > +}
> > +#endif
> > +#endif
> > +
> >  static const char *color_grep_slots[] = {
> >       [GREP_COLOR_CONTEXT]        = "context",
> >       [GREP_COLOR_FILENAME]       = "filename",
> > @@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)
> >   *
> >   * If using PCRE make sure that the library is configured
> >   * to use the right allocator (ex: NED)
> > + * if any object is created it should be cleaned up in grep_destroy()
> >   */
> >  void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
> >  {
> > @@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
> >               color_set(opt->colors[i], def->colors[i]);
> >  }
> >
> > +void grep_destroy(void)
> > +{
> > +#ifdef USE_LIBPCRE2
> > +     pcre2_general_context_free(pcre2_global_context);
> > +#endif
> > +}
> > +
> >  static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
> >  {
> >       /*
> > @@ -319,6 +343,11 @@ void append_header_grep_pattern(struct grep_opt *opt,
> >  void append_grep_pattern(struct grep_opt *opt, const char *pat,
> >                        const char *origin, int no, enum grep_pat_token t)
> >  {
> > +#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
> > +     if (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))
> > +             pcre2_global_context = pcre2_general_context_create(
> > +                                     pcre2_malloc, pcre2_free, NULL);
> > +#endif
> >       append_grep_pat(opt, pat, strlen(pat), origin, no, t);
> >  }
> >
> > @@ -507,9 +536,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
> >
> >       p->pcre2_compile_context = NULL;
> >
> > +     /* pcre2_global_context is initialized in append_grep_pattern */
> >       if (opt->ignore_case) {
> >               if (has_non_ascii(p->pattern)) {
> > -                     character_tables = pcre2_maketables(NULL);
> > +#ifdef USE_NED_ALLOCATOR
> > +                     if (!pcre2_global_context)
> > +                             BUG("pcre2_global_context uninitialized");
>
> [1] initializes on demand.  Why not do that?  To avoid race conditions

this was just for help migrating the code, could go away even now, but
though it was nice to keep for safety (as someone mentioned recently)

> that would lead to occasional double-allocation of the global context?

and a nicer API that allows for cleaning up any global objects (with
the chartables being an easy one to tackle next) and might
help in the future to make the worker threads less messy (ex: compile vs match)

> > +#endif
> > +                     character_tables = pcre2_maketables(pcre2_global_context);
> >                       p->pcre2_compile_context = pcre2_compile_context_create(NULL);
>
> Don't you want to pass pcre2_global_context here as well?  And [1] even
> uses it in some more places.
>
> Oh, that's the "expected more" when "better understood" part from the
> commit message, right?

correct, was trying to be conservative and minimal (since this code
will conflict as well
with other in the fly changes), but considering that the last Azure
build from pu still failed
and I have no windows box to debug, might need to do it anyway.

> Basically I'd expect the custom global context to be used for all PCRE2
> allocations; I can't think of a reason for mixing allocators (e.g.
> system malloc for PCRE2 regexes and nedmalloc for everything else).

ok

Carlo

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

[René Scharfe]

Am 07.08.19 um 11:49 schrieb Carlo Arenas:
> was hoping will perform better but it seems that testing can be done
> only in windows

nedmalloc works on other platforms as well.  On Debian Testing with GCC
9.1.0 I need two changes to suppress some compiler warnings, though.
Will post them as replies.

"make USE_NED_ALLOCATOR=1 test" then reports these failures:

t7816-grep-binary-pattern.sh                     (Wstat: 256 Tests: 145 Failed: 5)
  Failed tests:  48, 54, 57, 60, 63
  Non-zero exit status: 1

And the first one when running that test with --verbose and --immediate
is showing:

BUG: grep.c:510: pcre2_global_context uninitialized
Aborted
not ok 48 - LC_ALL='C' git grep -P -f f -i '[æ]<NUL>ð' a
#
#				printf '[æ]Qð' | q_to_nul >f &&
#				LC_ALL='C' git grep -P -f f -i a
#

[PATCH 1/2] nedmalloc: do assignments only after the declaration section

[René Scharfe]

Avoid the following compiler warning:

In file included from compat/nedmalloc/nedmalloc.c:63:
compat/nedmalloc/malloc.c.h: In function ‘pthread_release_lock’:
compat/nedmalloc/malloc.c.h:1759:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
 1759 |     volatile unsigned int* lp = &sl->l;
      |     ^~~~~~~~

Signed_off-by: René Scharfe <l.s.r@web.de>
---
 compat/nedmalloc/malloc.c.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h
index b833ff9225..88c131ca93 100644
--- a/compat/nedmalloc/malloc.c.h
+++ b/compat/nedmalloc/malloc.c.h
@@ -1755,10 +1755,10 @@ static FORCEINLINE void pthread_release_lock (MLOCK_T *sl) {
   assert(sl->l != 0);
   assert(sl->threadid == CURRENT_THREAD);
   if (--sl->c == 0) {
-    sl->threadid = 0;
     volatile unsigned int* lp = &sl->l;
     int prev = 0;
     int ret;
+    sl->threadid = 0;
     __asm__ __volatile__ ("lock; xchgl %0, %1"
 			  : "=r" (ret)
 			  : "m" (*(lp)), "0"(prev)
--
2.22.0

[PATCH 2/2] nedmalloc: avoid compiler warning about unused value

[René Scharfe]

Cast the evaluated value of the macro INITIAL_LOCK to void to instruct
the compiler that we're not interested in said value nor the following
warning:

In file included from compat/nedmalloc/nedmalloc.c:63:
compat/nedmalloc/malloc.c.h: In function ‘init_user_mstate’:
compat/nedmalloc/malloc.c.h:1706:62: error: right-hand operand of comma expression has no effect [-Werror=unused-value]
 1706 | #define INITIAL_LOCK(sl)      (memset(sl, 0, sizeof(MLOCK_T)), 0)
      |                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
compat/nedmalloc/malloc.c.h:5020:3: note: in expansion of macro ‘INITIAL_LOCK’
 5020 |   INITIAL_LOCK(&m->mutex);
      |   ^~~~~~~~~~~~

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/nedmalloc/malloc.c.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h
index 88c131ca93..9134349590 100644
--- a/compat/nedmalloc/malloc.c.h
+++ b/compat/nedmalloc/malloc.c.h
@@ -3066,7 +3066,7 @@ static int init_mparams(void) {
 #if !ONLY_MSPACES
     /* Set up lock for main malloc area */
     gm->mflags = mparams.default_mflags;
-    INITIAL_LOCK(&gm->mutex);
+    (void)INITIAL_LOCK(&gm->mutex);
 #endif

 #if (FOOTERS && !INSECURE)
@@ -5017,7 +5017,7 @@ static mstate init_user_mstate(char* tbase, size_t tsize) {
   mchunkptr msp = align_as_chunk(tbase);
   mstate m = (mstate)(chunk2mem(msp));
   memset(m, 0, msize);
-  INITIAL_LOCK(&m->mutex);
+  (void)INITIAL_LOCK(&m->mutex);
   msp->head = (msize|PINUSE_BIT|CINUSE_BIT);
   m->seg.base = m->least_addr = tbase;
   m->seg.size = m->footprint = m->max_footprint = tsize;
--
2.22.0

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

[Carlo Arenas]

On Wed, Aug 7, 2019 at 6:03 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 07.08.19 um 11:49 schrieb Carlo Arenas:
> > was hoping will perform better but it seems that testing can be done
> > only in windows
>
> nedmalloc works on other platforms as well.

I meant[1] it works reliably enough to be useful for performance testing.

goes without saying that the fact that I am using a virtualbox with 2
CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)
and the test uses by default 8 threads, doesn't help, but to share my
pain here is the result of running p7820 with my last reroll on top of
pu, comparing a build of the same code without NED (this tree) with
one with it (HEAD)

Test                                               this tree
HEAD
-------------------------------------------------------------------------------------------
7820.1: basic grep -i 'how.to'                     0.89(1.12+0.46)
0.95(1.23+0.49) +6.7%
7820.2: extended grep -i 'how.to'                  0.90(1.12+0.49)
0.92(1.19+0.46) +2.2%
7820.3: perl grep -i 'how.to'                      0.54(0.30+0.52)
0.53(0.39+0.52) -1.9%
7820.5: basic grep -i '^how to'                    0.89(1.13+0.47)
0.91(1.13+0.49) +2.2%
7820.6: extended grep -i '^how to'                 0.84(1.04+0.49)
0.94(1.27+0.47) +11.9%
7820.7: perl grep -i '^how to'                     0.49(0.34+0.47)
0.51(0.36+0.49) +4.1%
7820.9: basic grep -i '[how] to'                   1.51(2.31+0.51)
1.55(2.38+0.51) +2.6%
7820.10: extended grep -i '[how] to'               1.50(2.20+0.59)
1.56(2.30+0.62) +4.0%
7820.11: perl grep -i '[how] to'                   0.67(0.50+0.52)
0.62(0.50+0.55) -7.5%
7820.13: basic grep -i '\(e.t[^ ]*\|v.ry\) rare'   2.58(4.39+0.56)
2.64(4.45+0.60) +2.3%
7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare'   2.60(4.41+0.56)
2.66(4.58+0.56) +2.3%
7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare'       1.17(1.66+0.53)
1.23(1.84+0.45) +5.1%
7820.17: basic grep -i 'm\(ú\|u\)lt.b\(æ\|y\)te'   1.12(1.54+0.51)
1.14(1.70+0.44) +1.8%
7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te'      1.09(1.54+0.48)
1.14(1.62+0.49) +4.6%
7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te'          0.87(1.09+0.46)
0.90(1.20+0.43) +3.4%

and here one comparing two builds (both with NED)

Test                                               origin/pu
HEAD
-------------------------------------------------------------------------------------------
7820.1: basic grep -i 'how.to'                     1.00(1.24+0.55)
0.94(1.19+0.52) -6.0%
7820.2: extended grep -i 'how.to'                  0.90(1.15+0.49)
0.93(1.23+0.44) +3.3%
7820.3: perl grep -i 'how.to'                      0.52(0.37+0.51)
0.59(0.34+0.53) +13.5%
7820.5: basic grep -i '^how to'                    0.89(1.16+0.48)
0.90(1.17+0.47) +1.1%
7820.6: extended grep -i '^how to'                 0.92(1.17+0.50)
0.92(1.20+0.45) +0.0%
7820.7: perl grep -i '^how to'                     0.45(0.33+0.42)
0.54(0.29+0.57) +20.0%
7820.9: basic grep -i '[how] to'                   1.60(2.46+0.52)
1.61(2.39+0.62) +0.6%
7820.10: extended grep -i '[how] to'               1.71(2.67+0.56)
1.57(2.41+0.54) -8.2%
7820.11: perl grep -i '[how] to'                   0.66(0.61+0.51)
0.59(0.44+0.51) -10.6%
7820.13: basic grep -i '\(e.t[^ ]*\|v.ry\) rare'   2.69(4.49+0.66)
2.67(4.49+0.60) -0.7%
7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare'   2.67(4.49+0.64)
2.64(4.54+0.54) -1.1%
7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare'       1.23(1.80+0.47)
1.25(1.89+0.46) +1.6%
7820.17: basic grep -i 'm\(ú\|u\)lt.b\(æ\|y\)te'   1.13(1.64+0.47)
1.14(1.64+0.48) +0.9%
7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te'      1.16(1.68+0.46)
1.20(1.60+0.60) +3.4%
7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te'          0.90(1.16+0.48)
0.88(1.17+0.45) -2.2%

with the only relevant line (for my code) being 7820.19 where it would
seem it performs almost the same (eventhough just adding NED made it
initially worst)

note though that the fact there are 20% swings in parts of the code
that hasn't changed
or that where explicitly #ifdef out of my code changes doesn't give me
much confidence, but since the windows guys seem to be using NED by
default, I am hoping it works better there.

note also there were no segfaults (which is what was reported
originally) so something
else must be off.

> On Debian Testing with GCC
> 9.1.0 I need two changes to suppress some compiler warnings, though.
> Will post them as replies.
>
> "make USE_NED_ALLOCATOR=1 test" then reports these failures:
>
> t7816-grep-binary-pattern.sh                     (Wstat: 256 Tests: 145 Failed: 5)
>   Failed tests:  48, 54, 57, 60, 63
>   Non-zero exit status: 1

you got me so excited I dusted and old PC and was downloading the debian ISO
when I realized the error was not a segfault[2] but my bug.

sorry about it; I would swear I run the full test and it was clean,
but it was most likely
with PCRE1 or the system malloc, and definitely too late at night.

Thanks for your help so far, and while I know it is VERY ugly v4 at
least addresses that
(and uncovered a couple more bugs), thanks also Junio for rerolling it
into pu so at least
we have a chance for it to build and run, and hopefully eventually pass.

Carlo

[1] https://public-inbox.org/git/20190806085014.47776-3-carenas@gmail.com/
[2] https://dev.azure.com/git/git/_build/results?buildId=832

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

[René Scharfe]

Am 08.08.19 um 04:35 schrieb Carlo Arenas:
> On Wed, Aug 7, 2019 at 6:03 AM René Scharfe <l.s.r@web.de> wrote:
>>
>> Am 07.08.19 um 11:49 schrieb Carlo Arenas:
>>> was hoping will perform better but it seems that testing can be done
>>> only in windows
>>
>> nedmalloc works on other platforms as well.
>
> I meant[1] it works reliably enough to be useful for performance testing.

You mentioned being concerned about performance several times and I
wondered why each time.  I'd expect no measurable difference between
using a custom global context and the internal one of PCRE2 -- setting
two function pointers surely can't take very long, can it?  But
measuring is better than guessing, of course.

> goes without saying that the fact that I am using a virtualbox with 2
> CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)
> and the test uses by default 8 threads, doesn't help,

nedmalloc is supposed to run on macOS as well.

> but to share my
> pain here is the result of running p7820 with my last reroll on top of
> pu, comparing a build of the same code without NED (this tree) with
> one with it (HEAD)
>
> Test                                               this tree
> HEAD
> -------------------------------------------------------------------------------------------
> 7820.1: basic grep -i 'how.to'                     0.89(1.12+0.46)
> 0.95(1.23+0.49) +6.7%
> 7820.2: extended grep -i 'how.to'                  0.90(1.12+0.49)
> 0.92(1.19+0.46) +2.2%
> 7820.3: perl grep -i 'how.to'                      0.54(0.30+0.52)
> 0.53(0.39+0.52) -1.9%
> 7820.5: basic grep -i '^how to'                    0.89(1.13+0.47)
> 0.91(1.13+0.49) +2.2%
> 7820.6: extended grep -i '^how to'                 0.84(1.04+0.49)
> 0.94(1.27+0.47) +11.9%
> 7820.7: perl grep -i '^how to'                     0.49(0.34+0.47)
> 0.51(0.36+0.49) +4.1%
> 7820.9: basic grep -i '[how] to'                   1.51(2.31+0.51)
> 1.55(2.38+0.51) +2.6%
> 7820.10: extended grep -i '[how] to'               1.50(2.20+0.59)
> 1.56(2.30+0.62) +4.0%
> 7820.11: perl grep -i '[how] to'                   0.67(0.50+0.52)
> 0.62(0.50+0.55) -7.5%
> 7820.13: basic grep -i '\(e.t[^ ]*\|v.ry\) rare'   2.58(4.39+0.56)
> 2.64(4.45+0.60) +2.3%
> 7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare'   2.60(4.41+0.56)
> 2.66(4.58+0.56) +2.3%
> 7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare'       1.17(1.66+0.53)
> 1.23(1.84+0.45) +5.1%
> 7820.17: basic grep -i 'm\(ú\|u\)lt.b\(æ\|y\)te'   1.12(1.54+0.51)
> 1.14(1.70+0.44) +1.8%
> 7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te'      1.09(1.54+0.48)
> 1.14(1.62+0.49) +4.6%
> 7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te'          0.87(1.09+0.46)
> 0.90(1.20+0.43) +3.4%
>
> and here one comparing two builds (both with NED)
>
> Test                                               origin/pu
> HEAD
> -------------------------------------------------------------------------------------------
> 7820.1: basic grep -i 'how.to'                     1.00(1.24+0.55)
> 0.94(1.19+0.52) -6.0%
> 7820.2: extended grep -i 'how.to'                  0.90(1.15+0.49)
> 0.93(1.23+0.44) +3.3%
> 7820.3: perl grep -i 'how.to'                      0.52(0.37+0.51)
> 0.59(0.34+0.53) +13.5%
> 7820.5: basic grep -i '^how to'                    0.89(1.16+0.48)
> 0.90(1.17+0.47) +1.1%
> 7820.6: extended grep -i '^how to'                 0.92(1.17+0.50)
> 0.92(1.20+0.45) +0.0%
> 7820.7: perl grep -i '^how to'                     0.45(0.33+0.42)
> 0.54(0.29+0.57) +20.0%
> 7820.9: basic grep -i '[how] to'                   1.60(2.46+0.52)
> 1.61(2.39+0.62) +0.6%
> 7820.10: extended grep -i '[how] to'               1.71(2.67+0.56)
> 1.57(2.41+0.54) -8.2%
> 7820.11: perl grep -i '[how] to'                   0.66(0.61+0.51)
> 0.59(0.44+0.51) -10.6%
> 7820.13: basic grep -i '\(e.t[^ ]*\|v.ry\) rare'   2.69(4.49+0.66)
> 2.67(4.49+0.60) -0.7%
> 7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare'   2.67(4.49+0.64)
> 2.64(4.54+0.54) -1.1%
> 7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare'       1.23(1.80+0.47)
> 1.25(1.89+0.46) +1.6%
> 7820.17: basic grep -i 'm\(ú\|u\)lt.b\(æ\|y\)te'   1.13(1.64+0.47)
> 1.14(1.64+0.48) +0.9%
> 7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te'      1.16(1.68+0.46)
> 1.20(1.60+0.60) +3.4%
> 7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te'          0.90(1.16+0.48)
> 0.88(1.17+0.45) -2.2%
>
> with the only relevant line (for my code) being 7820.19 where it would
> seem it performs almost the same (eventhough just adding NED made it
> initially worst)
>
> note though that the fact there are 20% swings in parts of the code
> that hasn't changed
> or that where explicitly #ifdef out of my code changes doesn't give me
> much confidence, but since the windows guys seem to be using NED by
> default, I am hoping it works better there.

These measurement results are quite noisy, so I wouldn't trust them too
much.  nedmalloc being slower than the one from a recent glibc version
is not very surprising given this statement from its home page,
https://www.nedprod.com/programs/portable/nedmalloc/:

   "Windows 7, Linux 3.x, FreeBSD 8, Mac OS X 10.6 all contain
    state-of-the-art allocators and no third party allocator is
    likely to significantly improve on them in real world results"

In particular I don't think that these results justify coupling the use
of nedmalloc to the choice of using a custom global context for PCRE2.

I'd expect:
- Without USE_NED_ALLOCATOR: xmalloc() should be used for all
  allocations, including for PCRE2.  Some special exceptions use
  malloc(3) directly, but for most uses we want the consistent
  out-of-memory handling that xmalloc() brings.
- With USE_NED_ALLOCATOR: malloc() and xmalloc() use nedmalloc
  behind the scenes and free() is similarly overridden, so all
  allocations are affected.
- If USE_NED_ALLOCATOR performs worse than the system allocator on
  some system then it's the problem of those that turn on that flag.

Makes sense?

René

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

[Carlo Arenas]

On Thu, Aug 8, 2019 at 12:07 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 08.08.19 um 04:35 schrieb Carlo Arenas:
> > On Wed, Aug 7, 2019 at 6:03 AM René Scharfe <l.s.r@web.de> wrote:
> >>
> >> Am 07.08.19 um 11:49 schrieb Carlo Arenas:
> >>> was hoping will perform better but it seems that testing can be done
> >>> only in windows
> >>
> >> nedmalloc works on other platforms as well.
> >
> > I meant[1] it works reliably enough to be useful for performance testing.
>
> You mentioned being concerned about performance several times and I
> wondered why each time.  I'd expect no measurable difference between
> using a custom global context and the internal one of PCRE2 -- setting
> two function pointers surely can't take very long, can it?  But
> measuring is better than guessing, of course.

setting the allocator is not a concern, but using it; it requires an
extra indirect function call which is usually not very friendly to
caches in our speculative execution CPU world.  our implementation
also adds the wrapper call overhead, but in this case it is just the
"cost of doing business" with PCRE2.

compilers had gotten a lot better since (mainly because of C++ and the
need for it with virtual methods) but I would rather measure.

> > goes without saying that the fact that I am using a virtualbox with 2
> > CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)
> > and the test uses by default 8 threads, doesn't help,
>
> nedmalloc is supposed to run on macOS as well.

the last version has some "fix miscompilations in macOS" fixes that
might be relevant, and the version we have in tree says it works in
the 32-bit version which latest macOS versions are working hard to
deprecate (can't even build for it anymore), eitherway trying to run
with a nedmalloc enabled git in macOS is not fun.

> > with the only relevant line (for my code) being 7820.19 where it would
> > seem it performs almost the same (eventhough just adding NED made it
> > initially worst)
> >
> > note though that the fact there are 20% swings in parts of the code
> > that hasn't changed
> > or that where explicitly #ifdef out of my code changes doesn't give me
> > much confidence, but since the windows guys seem to be using NED by
> > default, I am hoping it works better there.
>
> These measurement results are quite noisy, so I wouldn't trust them too
> much.  nedmalloc being slower than the one from a recent glibc version
> is not very surprising given this statement from its home page,
> https://www.nedprod.com/programs/portable/nedmalloc/:
>
>    "Windows 7, Linux 3.x, FreeBSD 8, Mac OS X 10.6 all contain
>     state-of-the-art allocators and no third party allocator is
>     likely to significantly improve on them in real world results"
>
> In particular I don't think that these results justify coupling the use
> of nedmalloc to the choice of using a custom global context for PCRE2.

neither did I either, the only reason I am holding on fully enabling
NED with PCRE2 in my series is just because I wan't to make sure we
have identified the bug correctly and we are fixing it (specially
since I can't reproduce it, and therefore neither debug it)

sorry for not making that clear enough, and as I said before, if we
keep seeing segfaults even after v4 then we will have to do that or I
might need to do a quick run to the nearest microsoft store hoping for
a distracted rep, instead.

> I'd expect:
> - Without USE_NED_ALLOCATOR: xmalloc() should be used for all
>   allocations, including for PCRE2.  Some special exceptions use
>   malloc(3) directly, but for most uses we want the consistent
>   out-of-memory handling that xmalloc() brings.

that is already in v4 and would expect to carry it forward.  this is
also what I had in mind when I said we will need some fixes on top of
Dscho version if we give up with these.

> - With USE_NED_ALLOCATOR: malloc() and xmalloc() use nedmalloc
>   behind the scenes and free() is similarly overridden, so all
>   allocations are affected.
> - If USE_NED_ALLOCATOR performs worse than the system allocator on
>   some system then it's the problem of those that turn on that flag.
>
> Makes sense?

completely, but note also that Dscho version would make the
performance impacts of using a custom allocator (if any) affect
everyone using PCRE2.

Carlo

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

[René Scharfe]

Am 08.08.19 um 14:38 schrieb Carlo Arenas:
> On Thu, Aug 8, 2019 at 12:07 AM René Scharfe <l.s.r@web.de> wrote:
>>
>> Am 08.08.19 um 04:35 schrieb Carlo Arenas:
>>> On Wed, Aug 7, 2019 at 6:03 AM René Scharfe <l.s.r@web.de> wrote:
>>>>
>>>> Am 07.08.19 um 11:49 schrieb Carlo Arenas:
>>>>> was hoping will perform better but it seems that testing can be done
>>>>> only in windows
>>>>
>>>> nedmalloc works on other platforms as well.
>>>
>>> I meant[1] it works reliably enough to be useful for performance testing.
>>
>> You mentioned being concerned about performance several times and I
>> wondered why each time.  I'd expect no measurable difference between
>> using a custom global context and the internal one of PCRE2 -- setting
>> two function pointers surely can't take very long, can it?  But
>> measuring is better than guessing, of course.
>
> setting the allocator is not a concern, but using it; it requires an
> extra indirect function call which is usually not very friendly to
> caches in our speculative execution CPU world.  our implementation
> also adds the wrapper call overhead, but in this case it is just the
> "cost of doing business" with PCRE2.

PCRE2 needs to allocate memory once per program run for the character
table and for each pattern compilation.  These are both rare events
compared to matching patterns against lines, and I suspect that
compilation in particular  has much more other work to do, even more so
with JIT enabled; I'd expect function call indirection to not make much
of a difference.

pcre2_compile() always calls the allocation function through a
function pointer in a context struct, by the way (see line 9695 in
https://vcs.pcre.org/pcre2/code/trunk/src/pcre2_compile.c?view=markup
or search for "malloc" in that file).

>>> goes without saying that the fact that I am using a virtualbox with 2
>>> CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)
>>> and the test uses by default 8 threads, doesn't help,
>>
>> nedmalloc is supposed to run on macOS as well.
>
> the last version has some "fix miscompilations in macOS" fixes that
> might be relevant, and the version we have in tree says it works in
> the 32-bit version which latest macOS versions are working hard to
> deprecate (can't even build for it anymore), eitherway trying to run
> with a nedmalloc enabled git in macOS is not fun.

Importing the latest version of nedmalloc might make sense in general.
The last commit in git://github.com/ned14/nedmalloc.git was done five
years ago; is it finished?  A diffstat with -b looks like this:

 malloc.c.h  | 1193 ++++++++++++++++++++++++++++-------------
 nedmalloc.c | 1720 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 nedmalloc.h | 1580 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 3840 insertions(+), 653 deletions(-)

Any nedmalloc fans interested in bringing the goodies hidden in there
to Git (presumably while retaining our local fixes)?

>> In particular I don't think that these results justify coupling the use
>> of nedmalloc to the choice of using a custom global context for PCRE2.
>
> neither did I either, the only reason I am holding on fully enabling
> NED with PCRE2 in my series is just because I wan't to make sure we
> have identified the bug correctly and we are fixing it (specially
> since I can't reproduce it, and therefore neither debug it)

That's a good reason against #ifdefs in general.  Sometimes they are
unavoidable, but they can make maintenance a lot harder.

> sorry for not making that clear enough, and as I said before, if we
> keep seeing segfaults even after v4 then we will have to do that or I
> might need to do a quick run to the nearest microsoft store hoping for
> a distracted rep, instead.

Asking to buy a license for Windows Vista might cause quite a bit of a
distraction in there -- Microsoft's support for that version ended two
years ago. :)  It still seems to be popular enough to be supported by
Git for Windows, however.

(You could buy Windows 10 and probably get a downgrade right, but
finding legit install media for Vista might be challenging.)

But I'd say do the easy thing: Custom global context for all.

>> I'd expect:
>> - Without USE_NED_ALLOCATOR: xmalloc() should be used for all
>>   allocations, including for PCRE2.  Some special exceptions use
>>   malloc(3) directly, but for most uses we want the consistent
>>   out-of-memory handling that xmalloc() brings.
>
> that is already in v4 and would expect to carry it forward.  this is
> also what I had in mind when I said we will need some fixes on top of
> Dscho version if we give up with these.
>
>> - With USE_NED_ALLOCATOR: malloc() and xmalloc() use nedmalloc
>>   behind the scenes and free() is similarly overridden, so all
>>   allocations are affected.
>> - If USE_NED_ALLOCATOR performs worse than the system allocator on
>>   some system then it's the problem of those that turn on that flag.
>>
>> Makes sense?
>
> completely, but note also that Dscho version would make the
> performance impacts of using a custom allocator (if any) affect
> everyone using PCRE2.

Sounds fine to me, if the performance numbers don't take too much of
a hit.  I'd be surprised if the needle moved at all (ignoring noise).

René

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Hi René,

On Thu, 8 Aug 2019, René Scharfe wrote:

> Importing the latest version of nedmalloc might make sense in general.
> The last commit in git://github.com/ned14/nedmalloc.git was done five
> years ago; is it finished?  A diffstat with -b looks like this:
>
>  malloc.c.h  | 1193 ++++++++++++++++++++++++++++-------------
>  nedmalloc.c | 1720 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>  nedmalloc.h | 1580 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 3840 insertions(+), 653 deletions(-)
>
> Any nedmalloc fans interested in bringing the goodies hidden in there
> to Git (presumably while retaining our local fixes)?

I had looked into this already over two years ago, and had to stop after
investigating a performance regression for two weeks and not getting
anywhere.

Also, nedmalloc fell unmaintained, so I don't necessarily think that it
would be a good idea to  spend a lot of time on it.

In the meantime, there is a much more viable contender: mi-malloc.
Preliminary tests suggest that its performance on Windows is at least as
good as nedmalloc's, and Windows was the use case for which we
integrated nedmalloc into Git's compat/ in the first place.

I have tentative patches to integrate it into Git for Windows, and
basically got side-tracked with other things. Expect to see something
regarding mi-malloc from me in September.

Ciao,
Dscho

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

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

>> > +#ifdef USE_NED_ALLOCATOR
>> > +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
>> > +{
>> > +     return malloc(size);
>>
>> Should this be xmalloc() to get consistent out-of-memory handling?
>
> good point, note though that since it is inside a USE_NED_ALLOCATOR
> ifdef it is really
> nedalloc in disguise

It would be nedalloc() wrapped inside the "die if we cannot
allocate, possibly after releasing resources held in various caches"
xmalloc() wrapper.  So (1) either xmalloc or malloc would end up
eventually calling nedalloc that can be freed with nedfree, so from
that point of view either can be used without upsetting "free()",
and (2) we should use xmalloc() here because we care about
consistent OOM behaviour throughout the system.

Thanks.

[RFC PATCH v3 3/3] grep: avoid leak of chartables in PCRE2

[Carlo Marcelo Arenas Belón]

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced
a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from
the original change and free the generated table just like it is done
for PCRE1.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index 3e5bdf94a6..3d3ea0414e 100644
--- a/grep.c
+++ b/grep.c
@@ -527,7 +527,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;
@@ -543,9 +542,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			if (!pcre2_global_context)
 				BUG("pcre2_global_context uninitialized");
 #endif
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}
@@ -649,6 +649,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
diff --git a/grep.h b/grep.h
index 526c2db9ef..533165c21a 100644
--- a/grep.h
+++ b/grep.h
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;
-- 
2.23.0.rc1

Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> Junio, could you comment in my assumption that the use of grep in
> revision.c doesn't require initializing a PCRE2 global context and
> therefore not doing the cleanup?

Many callers of setup_revisions() do so only to run two-thing diffs
(e.g. run_diff_files() that compares the index and the working tree),
but some do so to traverse the history with the grep_filter
(e.g. "git log --grep=<pattern>").  "git log -P" may affect how that
pattern is used to match the contents of the log messages, no?

[RFC PATCH v4 0/3] grep: no leaks or crashes (windows testing needed)

[Carlo Marcelo Arenas Belón]

This series is a candidate reroll for cb/pcre2-chartables-leakfix, that
hopefully addresses the root cause of the problem reported by Dscho in
Windows, where the PCRE2 library wasn't aware of the custom allocator and
was returning a pointer created with the system malloc but passing it to
NED's free, resulting in a segfault.

The reason why it was triggered by the original leak fix is the layering
violation reported by René and that is exclusive to PCRE2 (hence why it
hasn't been reported with PCRE1).  Additional work might be available
in a future release of PCRE2 to address that as detailed in the upstream
bug[1] report.

Eitherway, since I am unable to replicate the original bug or take
performance numbers in a representative environment without Windows
this is only published as an RFC,

Changes since v3 (mostly in patch 2):

* git log also calls the "destructor" for grep API
* no more "bug" being triggered by `make test`, sorry René
* hopefully no more crashes in windows (I was expecting at most a BUG)

Future work (other than the needed refactoring explained in the
second patch) and adjacent bugs, includes:

* tracking more possible users of the grep API that might need to call
  grep_destroy()
* completely moving PCRE2 to use NED (as is done with PCRE1 and was
  proposed on the original patch[2] this is based on
* build on top of the new API so that other work could be shared
  (for example the chartables that started this whole mess)

or (hopefully not)

* ignore the original leak (maybe with an UNLEAK) as René suggested [3]
* discard this work and just use Dscho's fix (probably with some improvements)

Carlo Marcelo Arenas Belón (3):
  grep: make PCRE1 aware of custom allocator
  grep: make PCRE2 aware of custom allocator
  grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 builtin/log.c  |  1 +
 grep.c         | 77 ++++++++++++++++++++++++++++++++++++++++++++------
 grep.h         |  2 ++
 5 files changed, 73 insertions(+), 10 deletions(-)

[1] https://bugs.exim.org/show_bug.cgi?id=2429
[2] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/
[3] https://public-inbox.org/git/7ec60d57-9940-35f2-f7b5-c87d4dc7cdd5@web.de/

base-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd
-- 
2.23.0.rc1

[RFC PATCH v4 1/3] grep: make PCRE1 aware of custom allocator

[Carlo Marcelo Arenas Belón]

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
to override the system alocator, and so it is incompatible with
USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported
by extending grep_init to set the PCRE1 specific functions to the NED
versions (using the aliased names though) and therefore making sure all
alocations are done inside PCRE1 with the same allocator than the rest
of git.

This change might have performance impact (hopefully for the best) and
so will be good idea to test it in a platform where NED might have a
positive impact (ex: Windows 7)

[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bd246f2989..4b384f3759 100644
--- a/Makefile
+++ b/Makefile
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif
diff --git a/grep.c b/grep.c
index cd952ef5d3..0154998695 100644
--- a/grep.c
+++ b/grep.c
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;
-- 
2.23.0.rc1

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

[Carlo Marcelo Arenas Belón]

94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include
a way to override the system allocator, and so it is incompatible with
USE_NED_ALLOCATOR.  The problem was made visible when an attempt to
avoid a leak in a data structure that is created by the library was
passed to NED's free for disposal triggering a segfault in Windows.

PCRE2 requires the use of a general context to override the allocator
and therefore, there is a lot more code needed than in PCRE1, including
a couple of wrapper functions.

Extend the grep API with a "destructor" that could be called to cleanup
any objects that were created and used globally.

Update builtin/{grep,log} to use that new API, but any other future
users should make sure to have matching grep_init/grep_destroy calls
if they are using the pattern matching functionality (currently only
relevant when using both NED and PCRE2)

Move the logic to decide if a general context will be needed to an
earlier phase so it will only be done once per pattern (instead of
at least once per worker thread) avoiding then the need for locking.

This change does the minimum change required to hopefully solve the
crash, with the rest of the users of it added later.

Helped-by: René Scharfe <l.s.r@web.de>
Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
V4:
* use xmalloc instead as suggested by René and Junio
* "fix" for regression in t7816 as reported by René

 builtin/grep.c |  1 +
 builtin/log.c  |  1 +
 grep.c         | 62 ++++++++++++++++++++++++++++++++++++++++++++------
 grep.h         |  1 +
 4 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 560051784e..e49c20df60 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }
diff --git a/builtin/log.c b/builtin/log.c
index 1cf9e37736..139b8770e7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2146,6 +2146,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)
 		list = list->next;
 	}
 
+	grep_destroy();
 	free_patch_ids(&ids);
 	return 0;
 }
diff --git a/grep.c b/grep.c
index 0154998695..8ee982e2e8 100644
--- a/grep.c
+++ b/grep.c
@@ -16,6 +16,22 @@ static int grep_source_is_binary(struct grep_source *gs,
 
 static struct grep_opt grep_defaults;
 
+#ifdef USE_LIBPCRE2
+static pcre2_general_context *pcre2_global_context;
+
+#ifdef USE_NED_ALLOCATOR
+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
+{
+	return xmalloc(size); /* will use nedalloc underneath */
+}
+
+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
+{
+	return free(pointer);
+}
+#endif
+#endif
+
 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
 	[GREP_COLOR_FILENAME]	    = "filename",
@@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)
  *
  * If using PCRE make sure that the library is configured
  * to use the right allocator (ex: NED)
+ * if any object is created it should be cleaned up in grep_destroy()
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
@@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 		color_set(opt->colors[i], def->colors[i]);
 }
 
+void grep_destroy(void)
+{
+#ifdef USE_LIBPCRE2
+	pcre2_general_context_free(pcre2_global_context);
+#endif
+}
+
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	/*
@@ -254,12 +278,29 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o
 		grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
 }
 
-static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,
+static struct grep_pat *create_grep_pat(struct grep_opt *opt,
+					const char *pat, size_t patlen,
 					const char *origin, int no,
 					enum grep_pat_token t,
 					enum grep_header_field field)
 {
 	struct grep_pat *p = xcalloc(1, sizeof(*p));
+
+#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
+	/* BUG: this is technically not needed if we will do UTF matching
+	 *      but UTF locale detection is currently broken  */
+	/* SMELL: opt shouldn't be needed at this level but it is used
+	 *        here to keep the way we were detecting the need for
+	 *        the chartable consistent and until it can be refactored
+	 *        properly.  the NULL check is needed as a workaround
+	 *        for segfaulting in t7810.102 and should also go */
+	/* TODO: has_non_ascii doesn't support NUL in pattern */
+	if (!pcre2_global_context && opt && opt->ignore_case &&
+		has_non_ascii(pat))
+		pcre2_global_context = pcre2_general_context_create(
+					pcre2_malloc, pcre2_free, NULL);
+#endif
+
 	p->pattern = xmemdupz(pat, patlen);
 	p->patternlen = patlen;
 	p->origin = origin;
@@ -291,8 +332,10 @@ static void do_append_grep_pat(struct grep_pat ***tail, struct grep_pat *p)
 			}
 			if (!nl)
 				break;
-			new_pat = create_grep_pat(nl + 1, len - 1, p->origin,
-						  p->no, p->token, p->field);
+
+			new_pat = create_grep_pat(NULL, nl + 1, len - 1,
+						p->origin, p->no, p->token,
+						p->field);
 			new_pat->next = p->next;
 			if (!p->next)
 				*tail = &new_pat->next;
@@ -309,8 +352,8 @@ static void do_append_grep_pat(struct grep_pat ***tail, struct grep_pat *p)
 void append_header_grep_pattern(struct grep_opt *opt,
 				enum grep_header_field field, const char *pat)
 {
-	struct grep_pat *p = create_grep_pat(pat, strlen(pat), "header", 0,
-					     GREP_PATTERN_HEAD, field);
+	struct grep_pat *p = create_grep_pat(opt, pat, strlen(pat), "header",
+					 0, GREP_PATTERN_HEAD, field);
 	if (field == GREP_HEADER_REFLOG)
 		opt->use_reflog_filter = 1;
 	do_append_grep_pat(&opt->header_tail, p);
@@ -325,7 +368,7 @@ void append_grep_pattern(struct grep_opt *opt, const char *pat,
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen,
 		     const char *origin, int no, enum grep_pat_token t)
 {
-	struct grep_pat *p = create_grep_pat(pat, patlen, origin, no, t, 0);
+	struct grep_pat *p = create_grep_pat(opt, pat, patlen, origin, no, t, 0);
 	do_append_grep_pat(&opt->pattern_tail, p);
 }
 
@@ -507,9 +550,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	p->pcre2_compile_context = NULL;
 
+	/* pcre2_global_context is initialized in append_grep_pattern */
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
+#ifdef USE_NED_ALLOCATOR
+			if (!pcre2_global_context)
+				BUG("pcre2_global_context uninitialized");
+#endif
+			character_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}
diff --git a/grep.h b/grep.h
index 1875880f37..526c2db9ef 100644
--- a/grep.h
+++ b/grep.h
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
-- 
2.23.0.rc1

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

[Junio C Hamano]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

As we already have such an ifdef block here...

> +#ifdef USE_LIBPCRE2
> +static pcre2_general_context *pcre2_global_context;
> +
> +#ifdef USE_NED_ALLOCATOR
> +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
> +{
> +	return xmalloc(size); /* will use nedalloc underneath */
> +}
> +
> +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
> +{
> +	return free(pointer);
> +}
> +#endif
> +#endif

... perhaps an ugly ifdef block like this one ...

> +static struct grep_pat *create_grep_pat(struct grep_opt *opt,
> +					const char *pat, size_t patlen,
>  					const char *origin, int no,
>  					enum grep_pat_token t,
>  					enum grep_header_field field)
>  {
>  	struct grep_pat *p = xcalloc(1, sizeof(*p));
> +
> +#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
> +	/* BUG: this is technically not needed if we will do UTF matching
> +	 *      but UTF locale detection is currently broken  */
> +	/* SMELL: opt shouldn't be needed at this level but it is used
> +	 *        here to keep the way we were detecting the need for
> +	 *        the chartable consistent and until it can be refactored
> +	 *        properly.  the NULL check is needed as a workaround
> +	 *        for segfaulting in t7810.102 and should also go */
> +	/* TODO: has_non_ascii doesn't support NUL in pattern */
> +	if (!pcre2_global_context && opt && opt->ignore_case &&
> +		has_non_ascii(pat))
> +		pcre2_global_context = pcre2_general_context_create(
> +					pcre2_malloc, pcre2_free, NULL);
> +#endif
> +

... can be abstracted away by *not* having the #if/#endif here and
instead have a line that looks like this:

	setup_pcre2_as_needed(opt, pat);

which implementation can be prepared near the top, close to where
pcre2_malloc/free are defined (or not) above, i.e.

	#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
	static void setup_pcre2_as_needed(struct grep_opt *opt,	const char *pat)
	{
		... the above one ...
	}
	#else
	#define setup_pcre2_as_needed(ignore1, ignore2) /* nothing */
	#endif

The conditional code in grep_destroy() can be eliminated in a
similar fashion, i.e.

	void grep_destroy(void)
	{
		cleanup_pcre2_as_needed();
	}

with

	#ifdef USE_LIBPCRE2
	static void cleanup_pcre2_as_needed(void)
	{
		pcre2_general_context_free(pcre2_global_context);
	}
	#else
	#define cleanup_pcre2_as_needed() /* nothing */
	#endif

near the top (the beneral idea is to "call" a helper function whose
name describes what the call is for in a more general terms, and let
only the implementation details be hidden inside ifdef blocks).

Also, 

	/* 
	 * our multi-line comments are supposed to be formatted like
	 * this, with opening and closing slash-asterisk and asterisk-slash
	 * on their own lines.
	 */

[RFC PATCH v4 3/3] grep: avoid leak of chartables in PCRE2

[Carlo Marcelo Arenas Belón]

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced
a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from
the original change and free the generated table just like it is done
for PCRE1.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index 8ee982e2e8..ccb6ab38a3 100644
--- a/grep.c
+++ b/grep.c
@@ -541,7 +541,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;
@@ -557,9 +556,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			if (!pcre2_global_context)
 				BUG("pcre2_global_context uninitialized");
 #endif
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}
@@ -663,6 +663,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
diff --git a/grep.h b/grep.h
index 526c2db9ef..533165c21a 100644
--- a/grep.h
+++ b/grep.h
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;
-- 
2.23.0.rc1

[RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[Carlo Marcelo Arenas Belón]

This series is a candidate reroll for cb/pcre2-chartables-leakfix, that
hopefully addresses the root cause of the problem reported by Dscho in
Windows, where the PCRE2 library wasn't aware of the custom allocator and
was returning a pointer created with the system malloc but passing it to
NED's free, resulting in a segfault.

The most likely reason why it was triggered by the original leak fix is
the layering violation reported by René and that is likely exclusive to PCRE2
(hence why it hasn't been reported with PCRE1). Additional work might be
available in a future release of PCRE2 to address that as detailed in an
upstream bug[1] report.

Changes since v4 (only in patch 2):

* git log change reverted, still not sure where it will fit better and worst
  case will leak a few bytes when -P is used.
  since the users of this API are doing it indirectly it might be problematic
  long term though, but luckily since it is most of the tine a NOOP and can
  be called multiple times might be ok to do it unconditionally
* slightly better looking code

Changes since v3 (mostly in patch 2):

* git log also calls the "destructor" for grep API
* no more "bug" being triggered by make test, sorry René
* hopefully no more crashes in windows (I was expecting at most a BUG)

Future work (other than the needed refactoring explained in the
second patch) and adjacent bugs, includes:

* tracking more possible users of the grep API that might need to call
  grep_destroy()
* completely moving PCRE2 to use NED (as is done with PCRE1 and was
  proposed on the original patch[2] this is based on
* build on top of the new API so that other work could be shared
  (for example the chartables that started this whole mess)

or (hopefully not)

* ignore the original leak (maybe with an UNLEAK) as René suggested [3]
* discard this work and just use Dscho's fix (with some improvements,
  like using xmalloc)

[1] https://bugs.exim.org/show_bug.cgi?id=2429
[2] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/
[3] https://public-inbox.org/git/7ec60d57-9940-35f2-f7b5-c87d4dc7cdd5@web.de/

Carlo Marcelo Arenas Belón (3):
  grep: make PCRE1 aware of custom allocator
  grep: make PCRE2 aware of custom allocator
  grep: avoid leak of chartables in PCRE2

 Makefile       |  2 +-
 builtin/grep.c |  1 +
 grep.c         | 71 +++++++++++++++++++++++++++++++++++++++++++++++---
 grep.h         |  2 ++
 4 files changed, 72 insertions(+), 4 deletions(-)

base-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd
-- 
2.23.0.rc1

[RFC PATCH v5 1/3] grep: make PCRE1 aware of custom allocator

[Carlo Marcelo Arenas Belón]

63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
to override the system alocator, and so it is incompatible with
USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)

Make the minimum change possible to ensure this combination is supported
by extending grep_init to set the PCRE1 specific functions to the NED
versions (using the aliased names though) and therefore making sure all
alocations are done inside PCRE1 with the same allocator than the rest
of git.

This change might have performance impact (hopefully for the best) and
so will be good idea to test it in a platform where NED might have a
positive impact (ex: Windows 7)

[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile |  2 +-
 grep.c   | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bd246f2989..4b384f3759 100644
--- a/Makefile
+++ b/Makefile
@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif
diff --git a/grep.c b/grep.c
index cd952ef5d3..0154998695 100644
--- a/grep.c
+++ b/grep.c
@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)
  * Initialize one instance of grep_opt and copy the
  * default values from the template we read the configuration
  * information in an earlier call to git_config(grep_config).
+ *
+ * If using PCRE make sure that the library is configured
+ * to use the right allocator (ex: NED)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
 	int i;
 
+#ifdef USE_NED_ALLOCATOR
+#ifdef USE_LIBPCRE1
+	pcre_malloc = malloc;
+	pcre_free = free;
+#endif
+#endif
+
 	memset(opt, 0, sizeof(*opt));
 	opt->repo = repo;
 	opt->prefix = prefix;
-- 
2.23.0.rc1

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

[Carlo Marcelo Arenas Belón]

94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include
a way to override the system allocator, and so it is incompatible with
USE_NED_ALLOCATOR.  The problem was made visible when an attempt to
avoid a leak in a data structure that is created by the library was
passed to NED's free for disposal triggering a segfault in Windows.

PCRE2 requires the use of a general context to override the allocator
and therefore, there is a lot more code needed than in PCRE1, including
a couple of wrapper functions.

Extend the grep API with a "destructor" that could be called to cleanup
any objects that were created and used globally.

Update builtin/grep to use that new API, but any other future
users should make sure to have matching grep_init/grep_destroy calls
if they are using the pattern matching functionality (currently only
relevant when using both NED and PCRE2)

Move the logic to decide if a general context will be needed to an
earlier phase so it will only be done once per pattern (instead of
at least once per worker thread) avoiding then the need for locking.

This change does the minimum change required to hopefully solve the
crash, with the rest of the users of it added later.

Helped-by: René Scharfe <l.s.r@web.de>
Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 builtin/grep.c |  1 +
 grep.c         | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-
 grep.h         |  1 +
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 560051784e..e49c20df60 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		run_pager(&opt, prefix);
 	clear_pathspec(&pathspec);
 	free_grep_patterns(&opt);
+	grep_destroy();
 	return !hit;
 }
diff --git a/grep.c b/grep.c
index 0154998695..8e0b838db0 100644
--- a/grep.c
+++ b/grep.c
@@ -16,6 +16,44 @@ static int grep_source_is_binary(struct grep_source *gs,
 
 static struct grep_opt grep_defaults;
 
+#ifdef USE_LIBPCRE2
+static pcre2_general_context *pcre2_global_context;
+
+#ifdef USE_NED_ALLOCATOR
+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
+{
+	return xmalloc(size); /* will use nedalloc underneath */
+}
+
+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
+{
+	return free(pointer);
+}
+
+/*
+ * BUG: this is technically not needed if we will do UTF matching
+ *      but UTF locale detection is currently broken
+ * TODO: has_non_ascii() doesn't support NUL in pattern
+ */
+void setup_pcre2_as_needed(struct grep_opt *opt, const char *pat)
+{
+	if (!pcre2_global_context && opt->ignore_case &&
+		has_non_ascii(pat))
+		pcre2_global_context = pcre2_general_context_create(
+					pcre2_malloc, pcre2_free, NULL);
+}
+
+static void cleanup_pcre2_as_needed(void)
+{
+	pcre2_general_context_free(pcre2_global_context);
+}
+
+#else
+#define setup_pcre2_as_needed(opt, pat)
+#define cleanup_pcre2_as_needed()
+#endif
+#endif
+
 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
 	[GREP_COLOR_FILENAME]	    = "filename",
@@ -153,6 +191,7 @@ int grep_config(const char *var, const char *value, void *cb)
  *
  * If using PCRE make sure that the library is configured
  * to use the right allocator (ex: NED)
+ * if any object is created it should be cleaned up in grep_destroy()
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
@@ -188,6 +227,11 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 		color_set(opt->colors[i], def->colors[i]);
 }
 
+void grep_destroy(void)
+{
+	cleanup_pcre2_as_needed();
+}
+
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
 {
 	/*
@@ -326,6 +370,7 @@ void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen,
 		     const char *origin, int no, enum grep_pat_token t)
 {
 	struct grep_pat *p = create_grep_pat(pat, patlen, origin, no, t, 0);
+	setup_pcre2_as_needed(opt, pat);
 	do_append_grep_pat(&opt->pattern_tail, p);
 }
 
@@ -507,9 +552,18 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	p->pcre2_compile_context = NULL;
 
+	/*
+	 * pcre2_global_context is initialized in append_grep_pat()
+	 * with logic from setup_pcre2_as_needed() that mimics what
+	 * is used here and with the BUG() to protect from mismatches
+	 */
 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
+#ifdef USE_NED_ALLOCATOR
+			if (!pcre2_global_context)
+				BUG("pcre2_global_context uninitialized");
+#endif
+			character_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}
diff --git a/grep.h b/grep.h
index 1875880f37..526c2db9ef 100644
--- a/grep.h
+++ b/grep.h
@@ -189,6 +189,7 @@ struct grep_opt {
 void init_grep_defaults(struct repository *);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
+void grep_destroy(void);
 void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);
 
 void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
-- 
2.23.0.rc1

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4486 bytes --]

Hi Carlo,

On Thu, 8 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> 94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include
> a way to override the system allocator, and so it is incompatible with
> USE_NED_ALLOCATOR.  The problem was made visible when an attempt to
> avoid a leak in a data structure that is created by the library was
> passed to NED's free for disposal triggering a segfault in Windows.
>
> PCRE2 requires the use of a general context to override the allocator
> and therefore, there is a lot more code needed than in PCRE1, including
> a couple of wrapper functions.
>
> Extend the grep API with a "destructor" that could be called to cleanup
> any objects that were created and used globally.
>
> Update builtin/grep to use that new API, but any other future
> users should make sure to have matching grep_init/grep_destroy calls
> if they are using the pattern matching functionality (currently only
> relevant when using both NED and PCRE2)
>
> Move the logic to decide if a general context will be needed to an
> earlier phase so it will only be done once per pattern (instead of
> at least once per worker thread) avoiding then the need for locking.
>
> This change does the minimum change required to hopefully solve the
> crash, with the rest of the users of it added later.
>
> Helped-by: René Scharfe <l.s.r@web.de>
> Reported-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---

Unfortunately, this is _still_ incorrect.

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.

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.

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

I don't claim that this is complete, you need to check carefully (for
example, I think you will want to get rid of _all_ the references to
nedmalloc), but this patch is at least a stop-gap measure to fix the CI
build (Junio, would you mind adding this as a SQUASH??? so that this
breakage won't keep the CI build of `pu` in the failing state?):

-- snipsnap --
diff --git a/grep.c b/grep.c
index ec845141bbb..4242ad0b4ae 100644
--- a/grep.c
+++ b/grep.c
@@ -19,7 +19,6 @@ static struct grep_opt grep_defaults;
 #ifdef USE_LIBPCRE2
 static pcre2_general_context *pcre2_global_context;

-#ifdef USE_NED_ALLOCATOR
 static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
 {
 	return malloc(size);
@@ -30,7 +29,6 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
 	return free(pointer);
 }
 #endif
-#endif

 static const char *color_grep_slots[] = {
 	[GREP_COLOR_CONTEXT]	    = "context",
@@ -176,6 +174,12 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	struct grep_opt *def = &grep_defaults;
 	int i;

+#if defined(USE_LIBPCRE2)
+	if (!pcre2_global_context)
+		pcre2_global_context = pcre2_general_context_create(
+					pcre2_malloc, pcre2_free, NULL);
+#endif
+
 #ifdef USE_NED_ALLOCATOR
 #ifdef USE_LIBPCRE1
 	pcre_malloc = malloc;
@@ -343,11 +347,6 @@ void append_header_grep_pattern(struct grep_opt *opt,
 void append_grep_pattern(struct grep_opt *opt, const char *pat,
 			 const char *origin, int no, enum grep_pat_token t)
 {
-#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)
-	if (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))
-		pcre2_global_context = pcre2_general_context_create(
-					pcre2_malloc, pcre2_free, NULL);
-#endif
 	append_grep_pat(opt, pat, strlen(pat), origin, no, t);
 }

--
2.23.0.windows.1

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

[Carlo Arenas]

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

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

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

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

So,... have we seen any conclusion on this?  Can any of you guys
give us a pointer to or copies of the candidate to be the final
solution of this topic, please?

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Thu, 3 Oct 2019, Junio C Hamano wrote:

> Carlo Arenas <carenas@gmail.com> writes:
>
> > On Tue, Aug 27, 2019 at 2:07 AM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> >>
> >> Unfortunately, this is _still_ incorrect.
> > ...
> > 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.
> > ...
> > 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
>
> So,... have we seen any conclusion on this?  Can any of you guys
> give us a pointer to or copies of the candidate to be the final
> solution of this topic, please?

I still need
https://github.com/git-for-windows/git/commit/719beb813e4f27f090696ce583df3e5f3c480545
and
https://github.com/git-for-windows/git/commit/3369c322bbd95820b971701fef7db44b26dd826f
to fix that part in Git for Windows' `shears/pu` branch (i.e. the
continuously rebased patch thicket).

Ciao,
Dscho

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

[Carlo Arenas]

On Thu, Oct 3, 2019 at 1:09 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> I still need
> https://github.com/git-for-windows/git/commit/719beb813e4f27f090696ce583df3e5f3c480545
> and
> https://github.com/git-for-windows/git/commit/3369c322bbd95820b971701fef7db44b26dd826f
> to fix that part in Git for Windows' `shears/pu` branch (i.e. the
> continuously rebased patch thicket).

or we could drop the current branch in pu and start again from scratch
now that all of the required dependencies had been merged.

apologize for the delays otherwise; $DAYJOB has taken a toll lately
and even my new shiny windows dev box hasn't seen much action.

will update here and in github shortly (which might mean up to this
weekend, being realistic), but should be better code (since it is
mostly Rene's) and better tested that way and hopefully won't cause
more breakage (specially in Windows, sorry Dscho)

Carlo

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

[Johannes Schindelin]

Hi Carlo,

On Thu, 3 Oct 2019, Carlo Arenas wrote:

> On Thu, Oct 3, 2019 at 1:09 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > I still need
> > https://github.com/git-for-windows/git/commit/719beb813e4f27f090696ce583df3e5f3c480545
> > and
> > https://github.com/git-for-windows/git/commit/3369c322bbd95820b971701fef7db44b26dd826f
> > to fix that part in Git for Windows' `shears/pu` branch (i.e. the
> > continuously rebased patch thicket).
>
> or we could drop the current branch in pu and start again from scratch
> now that all of the required dependencies had been merged.

I hope that your next iteration won't have any `#ifdef NED` in it?

>
> apologize for the delays otherwise; $DAYJOB has taken a toll lately
> and even my new shiny windows dev box hasn't seen much action.
>
> will update here and in github shortly (which might mean up to this
> weekend, being realistic), but should be better code (since it is
> mostly Rene's) and better tested that way and hopefully won't cause
> more breakage (specially in Windows, sorry Dscho)

I appreciate all your work!

Thanks,
Dscho

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

[Junio C Hamano]

Carlo Arenas <carenas@gmail.com> writes:

> or we could drop the current branch in pu and start again from scratch
> now that all of the required dependencies had been merged.

That would be the cleanest from my point of view.  Thanks, both.

[RFC PATCH v5 3/3] grep: avoid leak of chartables in PCRE2

[Carlo Marcelo Arenas Belón]

94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) introduced
a small memory leak visible with valgrind in t7813.

Complete the creation of a PCRE2 specific variable that was missing from
the original change and free the generated table just like it is done
for PCRE1.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 grep.c | 7 ++++---
 grep.h | 1 +
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/grep.c b/grep.c
index 8e0b838db0..146891e2bf 100644
--- a/grep.c
+++ b/grep.c
@@ -543,7 +543,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 	PCRE2_UCHAR errbuf[256];
 	PCRE2_SIZE erroffset;
 	int options = PCRE2_MULTILINE;
-	const uint8_t *character_tables = NULL;
 	int jitret;
 	int patinforet;
 	size_t jitsizearg;
@@ -563,9 +562,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			if (!pcre2_global_context)
 				BUG("pcre2_global_context uninitialized");
 #endif
-			character_tables = pcre2_maketables(pcre2_global_context);
+			p->pcre2_tables = pcre2_maketables(pcre2_global_context);
 			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
-			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
+			pcre2_set_character_tables(p->pcre2_compile_context,
+							p->pcre2_tables);
 		}
 		options |= PCRE2_CASELESS;
 	}
@@ -669,6 +669,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	free((void *)p->pcre2_tables);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
diff --git a/grep.h b/grep.h
index 526c2db9ef..533165c21a 100644
--- a/grep.h
+++ b/grep.h
@@ -96,6 +96,7 @@ struct grep_pat {
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
+	const uint8_t *pcre2_tables;
 	uint32_t pcre2_jit_on;
 	kwset_t kws;
 	unsigned fixed:1;
-- 
2.23.0.rc1

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[Carlo Arenas]

disregard this version, it still broken (and wouldn't even build on
some cases), the reasons why are still unclear though but at least it
might
seem from the last known run in windows that segfaults were prevented
at last and something was still off enough to trigger a BUG (shouldn't
be a concern with V6 or later that do PCRE2 migration to NED fully, as
agreed)

Thanks to Dscho's github integration to Azure pipelines got finally a
RC[1] for V6 that at least passes all tests and will need to get in
shape for submission.

as René pointed out in three my performance concerns might be a red
herring as well, but would be nice if someone with windows would
definitely get some numbers so we can't be sure there were truly no
regressions

Carlo

[1] https://github.com/carenas/git/tree/pcre2-chartables-leakfix

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[René Scharfe]

Am 09.08.19 um 13:24 schrieb Carlo Arenas:
> disregard this version, it still broken (and wouldn't even build on
> some cases), the reasons why are still unclear though but at least it
> might
> seem from the last known run in windows that segfaults were prevented
> at last and something was still off enough to trigger a BUG (shouldn't
> be a concern with V6 or later that do PCRE2 migration to NED fully, as
> agreed)

So how about starting stupidly simple?  You can test it everywhere, it
just needs libpcre2.  It works without that library as well (tested with
"make USE_LIBPCRE= USE_LIBPCRE2= test"), but doesn't allocate anything
in that case, of course.  The character tables leak fix should be safe
on top.  If you detect performance issues then we can address them in
additional patches.

-- >8 --
Subject: [PATCH] grep: use xmalloc() for all PCRE2 allocations

Build a PCRE2 global custom context when compiling a pattern and use it
to tell the library to use xmalloc() for allocations.  This provides
consistent out-of-memory handling and makes sure it uses a custom
allocator, e.g. with USE_NED_ALLOCATOR.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
The function names are ridiculously long, I tried to stay within 80
characters per line but gave up in the end and just kept going without
line breaks.  Fits the "stupidly simple" approach..

 grep.c | 23 +++++++++++++++++------
 grep.h |  2 ++
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/grep.c b/grep.c
index cd952ef5d3..44f4e38657 100644
--- a/grep.c
+++ b/grep.c
@@ -482,6 +482,16 @@ static void free_pcre1_regexp(struct grep_pat *p)
 #endif /* !USE_LIBPCRE1 */

 #ifdef USE_LIBPCRE2
+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
+{
+	return xmalloc(size);
+}
+
+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
+{
+	free(pointer);
+}
+
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
 {
 	int error;
@@ -495,12 +505,12 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt

 	assert(opt->pcre2);

-	p->pcre2_compile_context = NULL;
+	p->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);
+	p->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);

 	if (opt->ignore_case) {
 		if (has_non_ascii(p->pattern)) {
-			character_tables = pcre2_maketables(NULL);
-			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
+			character_tables = pcre2_maketables(p->pcre2_general_context);
 			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
 		}
 		options |= PCRE2_CASELESS;
@@ -513,7 +523,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 					 p->pcre2_compile_context);

 	if (p->pcre2_pattern) {
-		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);
+		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, p->pcre2_general_context);
 		if (!p->pcre2_match_data)
 			die("Couldn't allocate PCRE2 match data");
 	} else {
@@ -550,10 +560,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 			return;
 		}

-		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
+		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, p->pcre2_general_context);
 		if (!p->pcre2_jit_stack)
 			die("Couldn't allocate PCRE2 JIT stack");
-		p->pcre2_match_context = pcre2_match_context_create(NULL);
+		p->pcre2_match_context = pcre2_match_context_create(p->pcre2_general_context);
 		if (!p->pcre2_match_context)
 			die("Couldn't allocate PCRE2 match context");
 		pcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack);
@@ -605,6 +615,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
 	pcre2_match_data_free(p->pcre2_match_data);
 	pcre2_jit_stack_free(p->pcre2_jit_stack);
 	pcre2_match_context_free(p->pcre2_match_context);
+	pcre2_general_context_free(p->pcre2_general_context);
 }
 #else /* !USE_LIBPCRE2 */
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
diff --git a/grep.h b/grep.h
index 1875880f37..73b8b87a3a 100644
--- a/grep.h
+++ b/grep.h
@@ -28,6 +28,7 @@ typedef int pcre_jit_stack;
 #else
 typedef int pcre2_code;
 typedef int pcre2_match_data;
+typedef int pcre2_general_context;
 typedef int pcre2_compile_context;
 typedef int pcre2_match_context;
 typedef int pcre2_jit_stack;
@@ -93,6 +94,7 @@ struct grep_pat {
 	int pcre1_jit_on;
 	pcre2_code *pcre2_pattern;
 	pcre2_match_data *pcre2_match_data;
+	pcre2_general_context *pcre2_general_context;
 	pcre2_compile_context *pcre2_compile_context;
 	pcre2_match_context *pcre2_match_context;
 	pcre2_jit_stack *pcre2_jit_stack;
--
2.22.0

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> So how about starting stupidly simple?  You can test it everywhere, it
> just needs libpcre2.  It works without that library as well (tested with
> "make USE_LIBPCRE= USE_LIBPCRE2= test"), but doesn't allocate anything
> in that case, of course.  The character tables leak fix should be safe
> on top.  If you detect performance issues then we can address them in
> additional patches.
>
> -- >8 --
> Subject: [PATCH] grep: use xmalloc() for all PCRE2 allocations
>
> Build a PCRE2 global custom context when compiling a pattern and use it
> to tell the library to use xmalloc() for allocations.  This provides
> consistent out-of-memory handling and makes sure it uses a custom
> allocator, e.g. with USE_NED_ALLOCATOR.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> The function names are ridiculously long, I tried to stay within 80
> characters per line but gave up in the end and just kept going without
> line breaks.  Fits the "stupidly simple" approach..

;-)  Thanks for keeping the conversation going.

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 5283 bytes --]

Hi,


On Fri, 9 Aug 2019, René Scharfe wrote:

> Am 09.08.19 um 13:24 schrieb Carlo Arenas:
> > disregard this version, it still broken (and wouldn't even build on
> > some cases), the reasons why are still unclear though but at least it
> > might
> > seem from the last known run in windows that segfaults were prevented
> > at last and something was still off enough to trigger a BUG (shouldn't
> > be a concern with V6 or later that do PCRE2 migration to NED fully, as
> > agreed)
>
> So how about starting stupidly simple?

FWIW I am very much in favor of this approach.

Thanks,
Dscho

> You can test it everywhere, it just needs libpcre2.  It works without
> that library as well (tested with "make USE_LIBPCRE= USE_LIBPCRE2=
> test"), but doesn't allocate anything in that case, of course.  The
> character tables leak fix should be safe on top.  If you detect
> performance issues then we can address them in additional patches.
>
> -- >8 --
> Subject: [PATCH] grep: use xmalloc() for all PCRE2 allocations
>
> Build a PCRE2 global custom context when compiling a pattern and use it
> to tell the library to use xmalloc() for allocations.  This provides
> consistent out-of-memory handling and makes sure it uses a custom
> allocator, e.g. with USE_NED_ALLOCATOR.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> The function names are ridiculously long, I tried to stay within 80
> characters per line but gave up in the end and just kept going without
> line breaks.  Fits the "stupidly simple" approach..
>
>  grep.c | 23 +++++++++++++++++------
>  grep.h |  2 ++
>  2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index cd952ef5d3..44f4e38657 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -482,6 +482,16 @@ static void free_pcre1_regexp(struct grep_pat *p)
>  #endif /* !USE_LIBPCRE1 */
>
>  #ifdef USE_LIBPCRE2
> +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
> +{
> +	return xmalloc(size);
> +}
> +
> +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
> +{
> +	free(pointer);
> +}
> +
>  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
>  {
>  	int error;
> @@ -495,12 +505,12 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>
>  	assert(opt->pcre2);
>
> -	p->pcre2_compile_context = NULL;
> +	p->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);
> +	p->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);
>
>  	if (opt->ignore_case) {
>  		if (has_non_ascii(p->pattern)) {
> -			character_tables = pcre2_maketables(NULL);
> -			p->pcre2_compile_context = pcre2_compile_context_create(NULL);
> +			character_tables = pcre2_maketables(p->pcre2_general_context);
>  			pcre2_set_character_tables(p->pcre2_compile_context, character_tables);
>  		}
>  		options |= PCRE2_CASELESS;
> @@ -513,7 +523,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>  					 p->pcre2_compile_context);
>
>  	if (p->pcre2_pattern) {
> -		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);
> +		p->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, p->pcre2_general_context);
>  		if (!p->pcre2_match_data)
>  			die("Couldn't allocate PCRE2 match data");
>  	} else {
> @@ -550,10 +560,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>  			return;
>  		}
>
> -		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);
> +		p->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, p->pcre2_general_context);
>  		if (!p->pcre2_jit_stack)
>  			die("Couldn't allocate PCRE2 JIT stack");
> -		p->pcre2_match_context = pcre2_match_context_create(NULL);
> +		p->pcre2_match_context = pcre2_match_context_create(p->pcre2_general_context);
>  		if (!p->pcre2_match_context)
>  			die("Couldn't allocate PCRE2 match context");
>  		pcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack);
> @@ -605,6 +615,7 @@ static void free_pcre2_pattern(struct grep_pat *p)
>  	pcre2_match_data_free(p->pcre2_match_data);
>  	pcre2_jit_stack_free(p->pcre2_jit_stack);
>  	pcre2_match_context_free(p->pcre2_match_context);
> +	pcre2_general_context_free(p->pcre2_general_context);
>  }
>  #else /* !USE_LIBPCRE2 */
>  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
> diff --git a/grep.h b/grep.h
> index 1875880f37..73b8b87a3a 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -28,6 +28,7 @@ typedef int pcre_jit_stack;
>  #else
>  typedef int pcre2_code;
>  typedef int pcre2_match_data;
> +typedef int pcre2_general_context;
>  typedef int pcre2_compile_context;
>  typedef int pcre2_match_context;
>  typedef int pcre2_jit_stack;
> @@ -93,6 +94,7 @@ struct grep_pat {
>  	int pcre1_jit_on;
>  	pcre2_code *pcre2_pattern;
>  	pcre2_match_data *pcre2_match_data;
> +	pcre2_general_context *pcre2_general_context;
>  	pcre2_compile_context *pcre2_compile_context;
>  	pcre2_match_context *pcre2_match_context;
>  	pcre2_jit_stack *pcre2_jit_stack;
> --
> 2.22.0
>

[PATCH] SQUASH

[Carlo Marcelo Arenas Belón]

Make using a general context (that is only needed with NED) to depend
on NED being selected at compile time.

the compile_context could be also make conditional but it gets ugly
really fasts with #ifdef
---
 Makefile | 2 +-
 grep.c   | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 8a7e235352..995e6c9351 100644
--- a/Makefile
+++ b/Makefile
@@ -1774,7 +1774,7 @@ ifdef NATIVE_CRLF
 endif
 
 ifdef USE_NED_ALLOCATOR
-	COMPAT_CFLAGS += -Icompat/nedmalloc
+	COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
 	COMPAT_OBJS += compat/nedmalloc/nedmalloc.o
 	OVERRIDE_STRDUP = YesPlease
 endif
diff --git a/grep.c b/grep.c
index 8255ec956e..233072ed80 100644
--- a/grep.c
+++ b/grep.c
@@ -482,6 +482,7 @@ static void free_pcre1_regexp(struct grep_pat *p)
 #endif /* !USE_LIBPCRE1 */
 
 #ifdef USE_LIBPCRE2
+#ifdef USE_NED_ALLOCATOR
 static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
 {
 	return xmalloc(size);
@@ -491,6 +492,7 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
 {
 	free(pointer);
 }
+#endif
 
 static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
 {
@@ -505,7 +507,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
 
 	assert(opt->pcre2);
 
+#ifdef USE_NED_ALLOCATOR
 	p->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);
+#endif
 	p->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);
 
 	if (opt->ignore_case) {
-- 
2.23.0.rc2

Re: [PATCH] SQUASH

[René Scharfe]

Am 10.08.19 um 05:03 schrieb Carlo Marcelo Arenas Belón:
> Make using a general context (that is only needed with NED) to depend
> on NED being selected at compile time.

A custom general context is needed to convince PCRE2 to use xmalloc()
instead of mallo().  That is independent of the choice of allocator.
So this change would be a step backwards?

René

Re: [PATCH] SQUASH

[Carlo Arenas]

On Sat, Aug 10, 2019 at 12:57 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 10.08.19 um 05:03 schrieb Carlo Marcelo Arenas Belón:
> > Make using a general context (that is only needed with NED) to depend
> > on NED being selected at compile time.
>
> A custom general context is needed to convince PCRE2 to use xmalloc()
> instead of mallo().  That is independent of the choice of allocator.
> So this change would be a step backwards?

My bad, you are correct.

Do you mind then if I "adopt" your patch and submit a reroll with it,
will also add an "equivalent" one to fix PCRE1 as well then, and we
will tackle any performance deficiencies or other issues in a future
series.

Carlo

Re: [PATCH] SQUASH

[René Scharfe]

Am 10.08.19 um 10:42 schrieb Carlo Arenas:
> On Sat, Aug 10, 2019 at 12:57 AM René Scharfe <l.s.r@web.de> wrote:
>>
>> Am 10.08.19 um 05:03 schrieb Carlo Marcelo Arenas Belón:
>>> Make using a general context (that is only needed with NED) to depend
>>> on NED being selected at compile time.
>>
>> A custom general context is needed to convince PCRE2 to use xmalloc()
>> instead of mallo().  That is independent of the choice of allocator.
>> So this change would be a step backwards?
>
> My bad, you are correct.
>
> Do you mind then if I "adopt" your patch and submit a reroll with it,
> will also add an "equivalent" one to fix PCRE1 as well then, and we
> will tackle any performance deficiencies or other issues in a future
> series.

I don't mind, sounds good.

René

Re: [PATCH] SQUASH

[Carlo Arenas]

On Sat, Aug 10, 2019 at 12:48 PM René Scharfe <l.s.r@web.de> wrote:
> > On Sat, Aug 10, 2019 at 12:57 AM René Scharfe <l.s.r@web.de> wrote:
> >
> > Do you mind then if I "adopt" your patch and submit a reroll with it,
>
> I don't mind, sounds good.

I had to squash another fix that was reported[1] before but wasn't picked up
and that ironically might explain the last segfaults from my old V4

Carlo

[1] https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/

Re: [PATCH] SQUASH

[René Scharfe]

Am 12.08.19 um 09:35 schrieb Carlo Arenas:
> On Sat, Aug 10, 2019 at 12:48 PM René Scharfe <l.s.r@web.de> wrote:
>>> On Sat, Aug 10, 2019 at 12:57 AM René Scharfe <l.s.r@web.de> wrote:
>>>
>>> Do you mind then if I "adopt" your patch and submit a reroll with it,
>>
>> I don't mind, sounds good.
>
> I had to squash another fix that was reported[1] before but wasn't picked up
> and that ironically might explain the last segfaults from my old V4
>
> Carlo
>
> [1] https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/

That looks like an issue worth its own commit.

I wonder if we'd want to pass pcre2_match_context to pcre2_match() a few
lines down as well, for consistency.

René

Re: [PATCH] SQUASH

[Carlo Arenas]

On Mon, Aug 12, 2019 at 5:14 AM René Scharfe <l.s.r@web.de> wrote:
>
> That looks like an issue worth its own commit.

OK, but then will make my topic interesting; indeed it almost feels
like it should be 3 different topics all depending of each other in a
chain:

* really use the match context
* move to xmalloc (2 patches, one for each PCRE library)
* fix leak

Which then will conflict with ab/pcre-jit-fixes before it can get merged to pu

> I wonder if we'd want to pass pcre2_match_context to pcre2_match() a few
> lines down as well, for consistency.

yes, that is actually what was tested[1] as can be seen in github
where Dscho integration did most of the heavy lifting with Windows

Carlo

[1] https://github.com/git/git/pull/627

Re: [PATCH] SQUASH

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

Hi Carlo,

On Fri, 9 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> diff --git a/grep.c b/grep.c
> index 8255ec956e..233072ed80 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -482,6 +482,7 @@ static void free_pcre1_regexp(struct grep_pat *p)
>  #endif /* !USE_LIBPCRE1 */
>
>  #ifdef USE_LIBPCRE2
> +#ifdef USE_NED_ALLOCATOR

I really, really, really, really, _really_ don't want this to be a
compile-time thing. Really. No, really. I mean it.

Ciao,
Dscho

>  static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)
>  {
>  	return xmalloc(size);
> @@ -491,6 +492,7 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)
>  {
>  	free(pointer);
>  }
> +#endif
>
>  static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)
>  {
> @@ -505,7 +507,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>
>  	assert(opt->pcre2);
>
> +#ifdef USE_NED_ALLOCATOR
>  	p->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);
> +#endif
>  	p->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);
>
>  	if (opt->ignore_case) {
> --
> 2.23.0.rc2
>
>

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[Carlo Arenas]

On Fri, Aug 9, 2019 at 2:26 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> FWIW I am very much in favor of this approach.

FWIW this is (almost) exactly what I had in mind with my first reply,
except I wanted to make setting of the global context be conditioned
to having NED enabled to avoid possible performance issues in
environments were NED is not even usable.

in macOS (obviously testing without NED) the following is the output
of (a hacked version) of p7801 for maint (against chromium's
repository), with René's patch on top

Test                                       HEAD^               HEAD
--------------------------------------------------------------------------------------
7820.1: perl grep 'how.to'                 0.51(0.35+1.11)
0.48(0.33+1.16) -5.9%
7820.2: perl grep '^how to'                0.47(0.33+1.08)
0.45(0.34+1.11) -4.3%
7820.3: perl grep '[how] to'               0.49(0.40+1.11)
0.53(0.41+1.13) +8.2%
7820.4: perl grep '(e.t[^ ]*|v.ry) rare'   68.72(68.77+1.14)
72.10(72.15+1.20) +4.9%
7820.5: perl grep 'm(ú|u)lt.b(æ|y)te'      0.48(0.35+1.12)
0.50(0.40+1.23) +4.2%

and this is with my squashed[2] changed on top of that :

Test                                       HEAD^               HEAD
--------------------------------------------------------------------------------------
7820.1: perl grep 'how.to'                 0.48(0.36+1.16)
0.46(0.33+1.09) -4.2%
7820.2: perl grep '^how to'                0.45(0.34+1.12)
0.42(0.29+0.99) -6.7%
7820.3: perl grep '[how] to'               0.48(0.40+1.13)
0.52(0.43+1.16) +8.3%
7820.4: perl grep '(e.t[^ ]*|v.ry) rare'   69.12(69.10+1.07)
69.19(69.19+1.18) +0.1%
7820.5: perl grep 'm(ú|u)lt.b(æ|y)te'      0.49(0.38+1.17)
0.46(0.35+1.13) -6.1%

the degenerate case is not something we can't fix anyway, since it is
likely a locking issue inside PCRE2 (I see at most 1 CPU doing work),
and the numbers are noisy because of the other problems I mentioned
before (hardcoded to 8 threads, running in a laptop with low number of
cores), which is why testing for performance regressions in windows is
strongly encouraged, regardless

Carlo

[1] https://public-inbox.org/git/CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com/
[2] https://public-inbox.org/git/20190810030315.7519-1-carenas@gmail.com/

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[René Scharfe]

Am 10.08.19 um 05:05 schrieb Carlo Arenas:
> in macOS (obviously testing without NED) the following is the output
> of (a hacked version) of p7801 for maint (against chromium's
> repository), with René's patch on top

Do you mean p7820?  And what did you change?  Looking at the results you
removed basic and extended from the list of regex engines, right?

Ugh, cloning https://chromium.googlesource.com/chromium/src.git sends
more than 16GB across the wire.  Is that even the right repository?  Not
sure if my machine can keep the relevant parts cached while grepping --
I/O times could drown out any difference due to context allocation and
memory allocator choice.  Let's see...

>
> Test                                       HEAD^               HEAD
> --------------------------------------------------------------------------------------
> 7820.1: perl grep 'how.to'                 0.51(0.35+1.11)
> 0.48(0.33+1.16) -5.9%
> 7820.2: perl grep '^how to'                0.47(0.33+1.08)
> 0.45(0.34+1.11) -4.3%
> 7820.3: perl grep '[how] to'               0.49(0.40+1.11)
> 0.53(0.41+1.13) +8.2%
> 7820.4: perl grep '(e.t[^ ]*|v.ry) rare'   68.72(68.77+1.14)
> 72.10(72.15+1.20) +4.9%
> 7820.5: perl grep 'm(ú|u)lt.b(æ|y)te'      0.48(0.35+1.12)
> 0.50(0.40+1.23) +4.2%
>
> and this is with my squashed[2] changed on top of that :
>
> Test                                       HEAD^               HEAD
> --------------------------------------------------------------------------------------
> 7820.1: perl grep 'how.to'                 0.48(0.36+1.16)
> 0.46(0.33+1.09) -4.2%
> 7820.2: perl grep '^how to'                0.45(0.34+1.12)
> 0.42(0.29+0.99) -6.7%
> 7820.3: perl grep '[how] to'               0.48(0.40+1.13)
> 0.52(0.43+1.16) +8.3%
> 7820.4: perl grep '(e.t[^ ]*|v.ry) rare'   69.12(69.10+1.07)
> 69.19(69.19+1.18) +0.1%
> 7820.5: perl grep 'm(ú|u)lt.b(æ|y)te'      0.49(0.38+1.17)
> 0.46(0.35+1.13) -6.1%
>
> the degenerate case is not something we can't fix anyway, since it is
> likely a locking issue inside PCRE2 (I see at most 1 CPU doing work),
> and the numbers are noisy because of the other problems I mentioned
> before (hardcoded to 8 threads, running in a laptop with low number of
> cores), which is why testing for performance regressions in windows is
> strongly encouraged, regardless
>
> Carlo
>
> [1] https://public-inbox.org/git/CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com/
> [2] https://public-inbox.org/git/20190810030315.7519-1-carenas@gmail.com/
>

So I pointed GIT_PERF_LARGE_REPO to the monster mentioned above, ran the
test once for warmup and here are the results of the second run:

Test                                            origin/master       pcre2-xmalloc             pcre2-xmalloc+nedmalloc
---------------------------------------------------------------------------------------------------------------------
7820.1: basic grep 'how.to'                     1.59(2.93+1.75)     1.60(3.04+1.74) +0.6%     1.64(2.87+1.90) +3.1%
7820.2: extended grep 'how.to'                  1.59(2.98+1.66)     1.55(2.83+1.76) -2.5%     1.67(3.15+1.70) +5.0%
7820.3: perl grep 'how.to'                      1.25(1.21+2.13)     1.25(1.24+2.08) +0.0%     1.29(1.32+2.08) +3.2%
7820.5: basic grep '^how to'                    1.52(2.82+1.66)     1.51(2.68+1.77) -0.7%     1.64(3.07+1.69) +7.9%
7820.6: extended grep '^how to'                 1.57(2.84+1.75)     1.51(2.76+1.73) -3.8%     1.61(2.95+1.75) +2.5%
7820.7: perl grep '^how to'                     1.21(1.15+2.10)     1.22(1.26+1.98) +0.8%     1.27(1.22+2.09) +5.0%
7820.9: basic grep '[how] to'                   1.95(4.51+1.68)     1.96(4.48+1.69) +0.5%     2.00(4.66+1.65) +2.6%
7820.10: extended grep '[how] to'               1.96(4.54+1.65)     1.94(4.46+1.70) -1.0%     2.04(4.78+1.65) +4.1%
7820.11: perl grep '[how] to'                   1.29(1.58+1.88)     1.28(1.50+1.94) -0.8%     1.34(1.51+2.06) +3.9%
7820.13: basic grep '\(e.t[^ ]*\|v.ry\) rare'   8.17(13.18+1.50)    8.29(13.36+1.37) +1.5%    8.31(13.33+1.60) +1.7%
7820.14: extended grep '(e.t[^ ]*|v.ry) rare'   8.13(13.03+1.59)    8.14(13.12+1.47) +0.1%    8.30(13.35+1.56) +2.1%
7820.15: perl grep '(e.t[^ ]*|v.ry) rare'       34.96(35.80+1.68)   34.99(35.60+1.91) +0.1%   35.18(35.83+1.90) +0.6%
7820.17: basic grep 'm\(ú\|u\)lt.b\(æ\|y\)te'   1.57(3.03+1.64)     1.53(2.76+1.75) -2.5%     1.60(2.89+1.77) +1.9%
7820.18: extended grep 'm(ú|u)lt.b(æ|y)te'      1.52(2.83+1.69)     1.52(2.89+1.63) +0.0%     1.58(2.80+1.84) +3.9%
7820.19: perl grep 'm(ú|u)lt.b(æ|y)te'          1.20(1.25+2.02)     1.21(1.30+1.96) +0.8%     1.25(1.22+2.11) +4.2%

pcre2-xmalloc is my patch on top of master, +nedmalloc has the warning
fixes I sent earlier and sets USE_NED_MALLOC.

I don't understand why my performance is lower by factor 2.5 than yours
for all perl regex tests except 7820.15 (your 7820.4), where my system
is two times faster.  Debian Testing, GCC 9.1.0, i5-9600K, 16GB RAM.

Anyway, nedmalloc is slower across the board, but the impact of my
patch is in the noise.  Right?

René

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[Carlo Arenas]

On Sat, Aug 10, 2019 at 12:57 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 10.08.19 um 05:05 schrieb Carlo Arenas:
> > in macOS (obviously testing without NED) the following is the output
> > of (a hacked version) of p7801 for maint (against chromium's
> > repository), with René's patch on top
>
> Do you mean p7820?  And what did you change?  Looking at the results you
> removed basic and extended from the list of regex engines, right?

correct, that was a weird typo there; apologize for the confusion.
you were correct about my changes; ironically, I didn't spell those
changes out to avoid confusion.

> Ugh, cloning https://chromium.googlesource.com/chromium/src.git sends
> more than 16GB across the wire.  Is that even the right repository?

noticed it was mentioned before by people[1] doing performance testing
on grep specifically and
thought it was better than try to come with another one, the linux
kernel wouldn't work in macOS
because it breaks "run" as it fails to checkout in a case insensitive
filesystem.

> Not
> sure if my machine can keep the relevant parts cached while grepping --
> I/O times could drown out any difference due to context allocation and
> memory allocator choice.  Let's see...

;), try setting /proc/sys/vm/swappiness to 0 or get more RAM

> I don't understand why my performance is lower by factor 2.5 than yours
> for all perl regex tests except 7820.15 (your 7820.4), where my system
> is two times faster.  Debian Testing, GCC 9.1.0, i5-9600K, 16GB RAM.

interesting; did you also see at most 100% of one CPU being used?
yours seem to be faster than mine so this might be representative of
single threaded performance.

> Anyway, nedmalloc is slower across the board, but the impact of my
> patch is in the noise.  Right?

yes, and there is a lot of noise.

Carlo

[1] https://public-inbox.org/git/e72330c6747218545cce1b6b1edfd1e448141a8f.1563570204.git.matheus.bernardino@usp.br/

Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes

[René Scharfe]

Am 10.08.19 um 14:40 schrieb Carlo Arenas:
> On Sat, Aug 10, 2019 at 12:57 AM René Scharfe <l.s.r@web.de> wrote:
>> I don't understand why my performance is lower by factor 2.5 than yours
>> for all perl regex tests except 7820.15 (your 7820.4), where my system
>> is two times faster.  Debian Testing, GCC 9.1.0, i5-9600K, 16GB RAM.
>
> interesting; did you also see at most 100% of one CPU being used?

For 7820.15 yes -- 100% of a single core, the rest idling.  The other
tests had only brief sequential phases, if at all, and used all cores.

René

Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

Hi Carlo,

On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:

> Eitherway, since I am unable to replicate the original bug or take
> performance numbers in a representative environment without Windows
> this is only published as an RFC, eventhough it has been tested and
> considered mostly complete.

Well, this is disappointing.

I worked several weeks on getting Azure Pipelines support in shape, so
that you can now open PRs against:

- https://github.com/git/git

- https://github.com/gitgitgadget/git

- https://github.com/git-for-windows/git

to get Windows/macOS/Linux testing for free.

So I guess you'd like fries with that, extra large ones, with extra
pepperoni seasoning?

Ciao,
Dscho

Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)

[Carlo Arenas]

On Thu, Aug 8, 2019 at 1:21 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:
> > Eitherway, since I am unable to replicate the original bug or take
> > performance numbers in a representative environment without Windows
> > this is only published as an RFC, eventhough it has been tested and
> > considered mostly complete.
>
> Well, this is disappointing.

Apologies

> I worked several weeks on getting Azure Pipelines support in shape, so
> that you can now open PRs against:

Thanks, I owe you a drink

  https://dev.azure.com/git/git/_build/results?buildId=862

Carlo

PS. I might had broken something here as I can't see the test results
that failed
https://dev.azure.com/git/git/_build/results?buildId=857&view=ms.vss-test-web.build-test-results-tab

Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)

[Johannes Schindelin]

Hi Carlo,

On Thu, 8 Aug 2019, Carlo Arenas wrote:

> PS. I might had broken something here as I can't see the test results
> that failed
> https://dev.azure.com/git/git/_build/results?buildId=857&view=ms.vss-test-web.build-test-results-tab

The test results load dynamically, so you'll probably just have to wait
for a couple moments. After that, just click on the lines indicating the
failed test cases to pop up a verbose trace of it.

Ciao,
Dscho

[PATCHv2 4/4] submodule: add embed-git-dir function

[Stefan Beller]

When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.

Add functionality to migrate the git directory to be embedded
into the superprojects git directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt   | 14 +++++++
 builtin/submodule--helper.c       | 33 ++++++++++++++++-
 git-submodule.sh                  |  7 +++-
 submodule.c                       | 65 ++++++++++++++++++++++++++++++++
 submodule.h                       |  2 +
 t/t7412-submodule-embedgitdirs.sh | 78 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 197 insertions(+), 2 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..34791cfc65 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
+'git submodule' [--quiet] embedgitdirs [--] [<path>...]
 
 
 DESCRIPTION
@@ -245,6 +246,19 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+embedgitdirs::
+	Move the git directory of submodules into its superprojects
+	`$GIT_DIR/modules` path and then connect the git directory and
+	its working directory by setting the `core.worktree` and adding
+	a .git file pointing to the git directory interned into the
+	superproject.
++
+A repository that was cloned independently and later added as a submodule or
+old setups have the submodules git directory inside the submodule instead of
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
 OPTIONS
 -------
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 806e29ce4e..e94dd68a0e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,36 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+static int embed_git_dir(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+
+	struct option embed_gitdir_options[] = {
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper embed-git-dir [<path>...]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, embed_gitdir_options,
+			     git_submodule_helper_usage, 0);
+
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	for (i = 0; i < list.nr; i++)
+		migrate_submodule_gitdir(prefix, list.entries[i]->name, 1);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1093,7 +1123,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url", resolve_relative_url, 0},
 	{"resolve-relative-url-test", resolve_relative_url_test, 0},
 	{"init", module_init, 0},
-	{"remote-branch", resolve_remote_submodule_branch, 0}
+	{"remote-branch", resolve_remote_submodule_branch, 0},
+	{"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..2178248287 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
 	done
 }
 
+cmd_embedgitdirs()
+{
+	git submodule--helper --prefix "$wt_prefix" embed-git-dirs "$@"
+}
+
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
 # options are primarily done by the subcommand implementations.
@@ -1140,7 +1145,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | status | summary | sync)
+	add | foreach | init | deinit | update | status | summary | sync | embedgitdirs)
 		command=$1
 		;;
 	-q|--quiet)
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..d330b567a3 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1263,3 +1263,68 @@ void prepare_submodule_repo_env(struct argv_array *out)
 	}
 	argv_array_push(out, "GIT_DIR=.git");
 }
+
+/*
+ * Migrate the given submodule (and all its submodules recursively) from
+ * having its git directory within the working tree to the git dir nested
+ * in its superprojects git dir under modules/.
+ */
+void migrate_submodule_gitdir(const char *prefix, const char *path,
+			      int recursive)
+{
+	char *old_git_dir;
+	const char *new_git_dir;
+	const struct submodule *sub;
+
+	old_git_dir = xstrfmt("%s/.git", path);
+	if (read_gitfile(old_git_dir))
+		/* If it is an actual gitfile, it doesn't need migration. */
+		goto out;
+
+	sub = submodule_from_path(null_sha1, path);
+	if (!sub)
+		die(_("Could not lookup name for submodule '%s'"),
+		      path);
+
+	new_git_dir = git_common_path("modules/%s", sub->name);
+	if (safe_create_leading_directories_const(new_git_dir) < 0)
+		die(_("could not create directory '%s'"), new_git_dir);
+
+	if (!prefix)
+		prefix = get_super_prefix();
+	printf("Migrating git directory of %s%s from\n'%s' to\n'%s'\n",
+		prefix ? prefix : "", path,
+		real_path(old_git_dir), new_git_dir);
+
+	if (rename(old_git_dir, new_git_dir) < 0)
+		die_errno(_("Could not migrate git directory from '%s' to '%s'"),
+			old_git_dir, new_git_dir);
+
+	connect_work_tree_and_git_dir(path, new_git_dir);
+
+out:
+	if (recursive) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		struct strbuf sb = STRBUF_INIT;
+
+		if (get_super_prefix())
+			strbuf_addstr(&sb, get_super_prefix());
+		strbuf_addstr(&sb, path);
+		strbuf_addch(&sb, '/');
+
+		cp.dir = path;
+		cp.git_cmd = 1;
+		cp.no_stdin = 1;
+		argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
+					    "submodule--helper",
+					   "embed-git-dirs", NULL);
+		prepare_submodule_repo_env(&cp.env_array);
+		if (run_command(&cp))
+			die(_("Could not migrate git directory in submodule '%s'"),
+			    path);
+
+		strbuf_release(&sb);
+	}
+
+	free(old_git_dir);
+}
diff --git a/submodule.h b/submodule.h
index d9e197a948..e5975d1f3d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -75,4 +75,6 @@ int parallel_submodules(void);
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 
+extern void migrate_submodule_gitdir(const char *prefix,
+				     const char *path, int recursive);
 #endif
diff --git a/t/t7412-submodule-embedgitdirs.sh b/t/t7412-submodule-embedgitdirs.sh
new file mode 100755
index 0000000000..3153dbc069
--- /dev/null
+++ b/t/t7412-submodule-embedgitdirs.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='Test submodule embedgitdirs
+
+This test verifies that `git submodue embedgitdirs` moves a submodules git
+directory into the superproject.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a real submodule' '
+	git init sub1 &&
+	test_commit -C sub1 first &&
+	git submodule add ./sub1 &&
+	test_tick &&
+	git commit -m superproject
+'
+
+test_expect_success 'embed the git dir' '
+	>expect.1 &&
+	>expect.2 &&
+	>actual.1 &&
+	>actual.2 &&
+	git status >expect.1 &&
+	git -C sub1 rev-parse HEAD >expect.2 &&
+	git submodule embedgitdirs &&
+	git fsck &&
+	test -f sub1/.git &&
+	test -d .git/modules/sub1 &&
+	git status >actual.1 &&
+	git -C sub1 rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup nested submodule' '
+	git init sub1/nested &&
+	test_commit -C sub1/nested first_nested &&
+	git -C sub1 submodule add ./nested &&
+	test_tick &&
+	git -C sub1 commit -m "add nested" &&
+	git add sub1 &&
+	git commit -m "sub1 to include nested submodule"
+'
+
+test_expect_success 'embed the git dir in a nested submodule' '
+	git status >expect.1 &&
+	git -C sub1/nested rev-parse HEAD >expect.2 &&
+	git submodule embedgitdirs &&
+	test -f sub1/nested/.git &&
+	test -d .git/modules/sub1/modules/nested &&
+	git status >actual.1 &&
+	git -C sub1/nested rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup a gitlink with missing .gitmodules entry' '
+	git init sub2 &&
+	test_commit -C sub2 first &&
+	git add sub2 &&
+	git commit -m superproject
+'
+
+test_expect_success 'embedding the git dir fails for incomplete submodules' '
+	git status >expect.1 &&
+	git -C sub2 rev-parse HEAD >expect.2 &&
+	test_must_fail git submodule embedgitdirs &&
+	git -C sub2 fsck &&
+	test -d sub2/.git &&
+	git status >actual &&
+	git -C sub2 rev-parse HEAD >actual.2 &&
+	test_cmp expect.1 actual.1 &&
+	test_cmp expect.2 actual.2
+'
+
+test_done
+
-- 
2.11.0.rc2.4.g3396b6f.dirty

[PATCH] SQUASH

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

On Tue, Nov 22, 2016 at 11:22 AM, Stefan Beller <sbeller@google.com> wrote:
> When a submodule has its git dir inside the working dir, the submodule
> support for checkout that we plan to add in a later patch will fail.
>
> Add functionality to migrate the git directory to be embedded
> into the superprojects git directory.
>

I spoke too early, this needs to be squashed. :(

Stefan

 builtin/submodule--helper.c | 3 +++
 git-submodule.sh            | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e94dd68a0e..75cdbf45b8 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1083,6 +1083,9 @@ static int embed_git_dir(int argc, const char **argv, const char *prefix)
 	struct module_list list = MODULE_LIST_INIT;
 
 	struct option embed_gitdir_options[] = {
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
 		OPT_END()
 	};
 
diff --git a/git-submodule.sh b/git-submodule.sh
index 2178248287..b7e124f340 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1133,7 +1133,7 @@ cmd_sync()
 
 cmd_embedgitdirs()
 {
-	git submodule--helper --prefix "$wt_prefix" embed-git-dirs "$@"
+	git submodule--helper embed-git-dirs --prefix "$wt_prefix" "$@"
 }
 
 # This loop parses the command line arguments to find the
-- 
2.11.0.rc2.4.g3396b6f.dirty