diff mbox series

SQUASH

Message ID 20190810030315.7519-1-carenas@gmail.com (mailing list archive)
State New, archived
Headers show
Series SQUASH | expand

Commit Message

Carlo Marcelo Arenas Belón Aug. 10, 2019, 3:03 a.m. UTC 
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(-)

Comments

René Scharfe Aug. 10, 2019, 7:57 a.m. UTC | #1 
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é
Carlo Marcelo Arenas Belón Aug. 10, 2019, 8:42 a.m. UTC | #2 
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
Johannes Schindelin Aug. 10, 2019, 1:57 p.m. UTC | #3 
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
>
>
René Scharfe Aug. 10, 2019, 7:47 p.m. UTC | #4 
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é
Carlo Marcelo Arenas Belón Aug. 12, 2019, 7:35 a.m. UTC | #5 
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/
René Scharfe Aug. 12, 2019, 12:14 p.m. UTC | #6 
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é
Carlo Marcelo Arenas Belón Aug. 12, 2019, 12:28 p.m. UTC | #7 
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
diff mbox series

Patch

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