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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- 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
>
>