| Message ID | 20190806085014.47776-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | grep: no leaks (WIP) | expand |
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 > >
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/
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);