{"thread":{"id":"51590","subject":"[PATCH 0/1] Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines","startedAt":"2019-08-05T11:51:13Z","lastAt":"2019-10-03T22:57:30Z","messageCount":67,"participants":["Johannes Schindelin via GitGitGadget","Carlo Arenas","Johannes Schindelin","Junio C Hamano","Carlo Marcelo Arenas Belón","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"379934","messageId":"pull.306.git.gitgitgadget@gmail.com","threadId":"51590","inReplyTo":null,"subject":"[PATCH 0/1] Fix a problem with PCRE2 and nedmalloc, found via Azure Pipelines","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-05T11:51:08Z","receivedAt":"2019-08-05T11:51:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"For a couple of days, maybe even a week, pu fails consistently, in the\nWindows job where it tests t7816. The symptom is a segmentation fault.\n\nI finally got to diagnose this, and it looked at first as if there was yet\nanother buffer overrun that was so small that valgrind failed to detect it\n(see https://github.com/gitgitgadget/git/pull/178). The problem is another\none, though: we ask PCRE2 to allocate a table (and it uses the system\nallocator for that), and then try to release it using nedmalloc's free() \nreplacement.\n\nThis is squarely a problem with cb/pcre2-chartables-leakfix in conjunction\nwith an overridden allocator.\n\nJunio, for your convenience, I rebased this patch directly on top of \nab/pcre-v2 and pushed out the let-pcre2-respect-nedmalloc branch at \nhttps://github.com/dscho/git ready to be pulled. The rebased version is not\ntechnically a bug fix, as I do not see any way that ab/pcre-v2 uses\nmismatched allocators for malloc()/free(), but just in case that you wanted\nto have it in v2.23.0 and not on top cb/pcre2-chartables-leakfix...\n\nJohannes Schindelin (1):\n pcre2: allow overriding the system allocator\n\n grep.c | 31 +++++++++++++++++++++++++++----\n 1 file changed, 27 insertions(+), 4 deletions(-)\n\n\nbase-commit: 7d3bf76999452ff64c84cec25fd95a7a95744b78\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-306%2Fdscho%2Ffix-for-pcre-chartables-leakfix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-306/dscho/fix-for-pcre-chartables-leakfix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/306\n-- \ngitgitgadget\n"},{"id":"379935","messageId":"3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com","threadId":"51590","inReplyTo":"pull.306.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] pcre2: allow overriding the system allocator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-08-05T11:51:09Z","receivedAt":"2019-08-05T11:51:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nSince 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),\nwe try to release the UTF-8 tables obtained via `pcre2_maketables()`. To\ndo that, we use the function `free()`. That is all fine and dandy as\nlong as that refers to the system allocator.\n\nHowever, when we compile Git with `USE_NED_ALLOCATOR` (notably on\nWindows), then `free()` actually calls `nedfree()`. But\n`pcre2_maketables()` allocated the tables using the system allocator\nbecause we did not tell it to use nedmalloc instead.\n\nThis leads to segmentation faults when the UTF-8 tables are released,\nmost notably in the `t7816-grep-binary-pattern.sh` test script.\n\nPCRE2 does have an option to override the allocator it should use, and\nthis patch calls upon it.\n\nAs there are other ways to override the system allocator than\n`USE_NED_ALLOCATOR`, we choose to specify the allocator we want to use\nspecifically, even if we still use the system allocator.\n\nSigned-off-by: Johannes Schindelin \n---\n grep.c | 31 +++++++++++++++++++++++++++----\n 1 file changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex d4c5d464ad..d6d29fc724 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -482,6 +482,27 @@ static void free_pcre1_regexp(struct grep_pat *p)\n #endif /* !USE_LIBPCRE1 */\n \n #ifdef USE_LIBPCRE2\n+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)\n+{\n+\treturn malloc(size);\n+}\n+\n+static void pcre2_free(void *pointer, void *memory_data)\n+{\n+\treturn free(pointer);\n+}\n+\n+static pcre2_general_context *get_pcre2_context(void)\n+{\n+\tstatic pcre2_general_context *context;\n+\n+\tif (!context)\n+\t\tcontext = pcre2_general_context_create(pcre2_malloc,\n+\t\t\t\t\t\t pcre2_free, NULL);\n+\n+\treturn context;\n+}\n+\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tint error;\n@@ -498,8 +519,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tp->pcre2_tables = pcre2_maketables(NULL);\n-\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n+\t\t\tp->pcre2_tables = pcre2_maketables(get_pcre2_context());\n+\t\t\tp->pcre2_compile_context =\n+\t\t\t\tpcre2_compile_context_create(get_pcre2_context());\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n \t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n@@ -513,7 +535,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\t\t\t p->pcre2_compile_context);\n \n \tif (p->pcre2_pattern) {\n-\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n+\t\tp->pcre2_match_data =\n+\t\t\tpcre2_match_data_create_from_pattern(p->pcre2_pattern, get_pcre2_context());\n \t\tif (!p->pcre2_match_data)\n \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n \t} else {\n@@ -550,7 +573,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\treturn;\n \t\t}\n \n-\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);\n+\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, get_pcre2_context());\n \t\tif (!p->pcre2_jit_stack)\n \t\t\tdie(\"Couldn't allocate PCRE2 JIT stack\");\n \t\tp->pcre2_match_context = pcre2_match_context_create(NULL);\n-- \ngitgitgadget\n"},{"id":"379938","messageId":"CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com","threadId":"51590","inReplyTo":"3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] pcre2: allow overriding the system allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-05T16:19:24Z","receivedAt":"2019-08-05T16:19:37Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),\n> we try to release the UTF-8 tables obtained via `pcre2_maketables()`. To\n> do that, we use the function `free()`. That is all fine and dandy as\n> long as that refers to the system allocator.\n\nSorry; I should have thought of this, but assumed was safe since it\nwould be broken\nthe same way with PCRE1.\n\nPresume git in windows only builds against PCRE2?\n\nLGTM except from the suggestion below that might make the code more \"standard\"\nand probably be a good base for a similar PCRE1 fix\n>\n> +static pcre2_general_context *get_pcre2_context(void)\n> +{\n> + static pcre2_general_context *context;\n> +\n> + if (!context)\n> + context = pcre2_general_context_create(pcre2_malloc,\n> + pcre2_free, NULL);\n> +\n> + return context;\n> +}\n\ninstead of using a static variable inside this helper function it\nmight be better to use\none extra field inside the (struct grep_pat *p), where all other\nvariables are kept\n\nAdditionally to being more consistent will avoid creating the global\ncontext for the\nmost common case (when the locale is either C/POSIX or UTF-8) and therefore\nhave a smaller impact on performance.\n\nCarlo\n"},{"id":"379939","messageId":"CAPUEspi2i_ZYoseQ8SkJm6k-YPva3fR4G5kFsGOT0KyNZNkQEw@mail.gmail.com","threadId":"51590","inReplyTo":"CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com","subject":"Re: [PATCH 1/1] pcre2: allow overriding the system allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-05T16:27:59Z","receivedAt":"2019-08-05T16:28:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"And forgot to mention that technically these are not UTF-8 tables but\nsingle byte tables like for example the ones used with en_US.ISO8859-1\n\nCarlo\n"},{"id":"379949","messageId":"nycvar.QRO.7.76.6.1908052120302.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com","subject":"Re: [PATCH 1/1] pcre2: allow overriding the system allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-05T19:26:00Z","receivedAt":"2019-08-05T19:26:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Mon, 5 Aug 2019, Carlo Arenas wrote:\n\n> On Mon, Aug 5, 2019 at 4:51 AM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > Since 7d3bf769994 (grep: avoid leak of chartables in PCRE2, 2019-08-01),\n> > we try to release the UTF-8 tables obtained via `pcre2_maketables()`. To\n> > do that, we use the function `free()`. That is all fine and dandy as\n> > long as that refers to the system allocator.\n>\n> Sorry; I should have thought of this, but assumed was safe since it\n> would be broken\n> the same way with PCRE1.\n>\n> Presume git in windows only builds against PCRE2?\n\nThat's right, we only build against PCRE2.\n\n> LGTM except from the suggestion below that might make the code more \"standard\"\n> and probably be a good base for a similar PCRE1 fix\n> >\n> > +static pcre2_general_context *get_pcre2_context(void)\n> > +{\n> > + static pcre2_general_context *context;\n> > +\n> > + if (!context)\n> > + context = pcre2_general_context_create(pcre2_malloc,\n> > + pcre2_free, NULL);\n> > +\n> > + return context;\n> > +}\n>\n> instead of using a static variable inside this helper function it\n> might be better to use one extra field inside the (struct grep_pat\n> *p), where all other variables are kept\n\nMy thinking about that was that this would add more code, and thus more\nopportunities to introduce bugs. Also, it's not like the general context\nreally has any _state_ per se. It just registers the current allocator,\nwhich we want to assume is constant over the life-time of the process.\n\nSo does it really make sense to create one general context per grep\npattern? (Or even per grep pattern and thread?)\n\n> Additionally to being more consistent will avoid creating the global\n> context for the most common case (when the locale is either C/POSIX or\n> UTF-8) and therefore have a smaller impact on performance.\n\nGiven that my patch does _less_ than what you suggest (it really only\ncreates at most _one_ general context per process, not one per\ninternal-grep invocation, possibly per-thread, and it also never calls\n`pcre2_general_context_free()`), I highly doubt that your proposed\nversion would have a _smaller_ impact on performance.\n\nCiao,\nDscho\n"},{"id":"379950","messageId":"nycvar.QRO.7.76.6.1908052132020.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"CAPUEspi2i_ZYoseQ8SkJm6k-YPva3fR4G5kFsGOT0KyNZNkQEw@mail.gmail.com","subject":"Re: [PATCH 1/1] pcre2: allow overriding the system allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-05T19:32:35Z","receivedAt":"2019-08-05T19:33:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Mon, 5 Aug 2019, Carlo Arenas wrote:\n\n> And forgot to mention that technically these are not UTF-8 tables but\n> single byte tables like for example the ones used with en_US.ISO8859-1\n\nThank you for pointing that out, I completely missed that.\n\nJunio, do you want me to re-send, or can you touch up the commit message\nwhile applying?\n\nCiao,\nDscho\n"},{"id":"379959","messageId":"xmqq36ifmg7v.fsf@gitster-ct.c.googlers.com","threadId":"51590","inReplyTo":"CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com","subject":"Re: [PATCH 1/1] pcre2: allow overriding the system allocator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-05T21:53:40Z","receivedAt":"2019-08-05T21:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> LGTM except from the suggestion below that might make the code more \"standard\"\n> and probably be a good base for a similar PCRE1 fix\n>>\n>> +static pcre2_general_context *get_pcre2_context(void)\n>> +{\n>> + static pcre2_general_context *context;\n>> +\n>> + if (!context)\n>> + context = pcre2_general_context_create(pcre2_malloc,\n>> + pcre2_free, NULL);\n>> +\n>> + return context;\n>> +}\n>\n> instead of using a static variable inside this helper function it\n> might be better to use\n> one extra field inside the (struct grep_pat *p), where all other\n> variables are kept\n>\n> Additionally to being more consistent will avoid creating the global\n\n\"standard\" and \"more consistent\" are good things, but I am not sure\nI should agree with the argument without knowing what you are\ncomparing your suggested improvement with. Whose standard practice\nare we trying to be consistent with? Keeping dynamic resources hooked\nto \"struct grep_pat\" so that (1) different patterns could use different\nsettings when they desire and (2) the resources are not hidden behind\na function-scope static and can be discarded when we are done with\nthe pattern, which is the standard in our \"grep\" subsystem?\n\nI think general context probably corresponds to a bit higher level\nthan individual grep_pat. E.g. when running \"grep -e foo -e bar\",\ndo we expect resources needed by patterns \"foo\" and \"bar\" would want\nto be allocated and freed by potentially separate \nfunction pairs?\n\n> context for the\n> most common case (when the locale is either C/POSIX or UTF-8) and therefore\n> have a smaller impact on performance.\n\nI am not sure about the impact on performance, but if it helps us\nkeep the subsystem reusable by avoiding function-scope static that\nwe cannot clear, that would be a good thing. But \"struct grep_pat\"\nmay be a bit too fine-grained to control general context.\n\n"},{"id":"379982","messageId":"CAPUEsphMh_ZqcH3M7PXC9jHTfEdQN3mhTAK2JDkdvKBp53YBoA@mail.gmail.com","threadId":"51590","inReplyTo":"xmqq36ifmg7v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] pcre2: allow overriding the system allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T06:24:55Z","receivedAt":"2019-08-06T06:25:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 5, 2019 at 2:53 PM Junio C Hamano wrote:\n>\n> Carlo Arenas writes:\n>\n> > LGTM except from the suggestion below that might make the code more \"standard\"\n> > and probably be a good base for a similar PCRE1 fix\n> >>\n> >> +static pcre2_general_context *get_pcre2_context(void)\n> >> +{\n> >> + static pcre2_general_context *context;\n> >> +\n> >> + if (!context)\n> >> + context = pcre2_general_context_create(pcre2_malloc,\n> >> + pcre2_free, NULL);\n> >> +\n> >> + return context;\n> >> +}\n> >\n> > instead of using a static variable inside this helper function it\n> > might be better to use\n> > one extra field inside the (struct grep_pat *p), where all other\n> > variables are kept\n> >\n> > Additionally to being more consistent will avoid creating the global\n>\n> \"standard\" and \"more consistent\" are good things, but I am not sure\n> I should agree with the argument without knowing what you are\n> comparing your suggested improvement with. Whose standard practice\n> are we trying to be consistent with? Keeping dynamic resources hooked\n> to \"struct grep_pat\" so that (1) different patterns could use different\n> settings when they desire and (2) the resources are not hidden behind\n> a function-scope static and can be discarded when we are done with\n> the pattern, which is the standard in our \"grep\" subsystem?\n\nIt was my impression that we were abusing the struct grep_pat to avoid\nhaving to deal properly with threading and interlocks.\n\nI agree my wording wasn't clear enough and my hinting a little obscure\nbut the original code is racy and it wouldn't be if the \"global context\" will be\ninitialized/maintained there; as an added benefit it will be straight forward to\nclear (together with the rest)\n\nI am not advocating that as a good design, but also think the code will be\nshorter (which was another rationale for the proposed change, to avoid\nintroducing yet more bugs and since it was even suggested for inclusion\nin the next release)\n\n> I think general context probably corresponds to a bit higher level\n> than individual grep_pat. E.g. when running \"grep -e foo -e bar\",\n> do we expect resources needed by patterns \"foo\" and \"bar\" would want\n> to be allocated and freed by potentially separate \n> function pairs?\n\nno with a different design; but currently even if almost all the time\nwe have the same pattern for all workers (ex: -e foo), why\nare we doing the compilation (plus JIT translation) and creating this\ntable and all other context pointers (plus a jit stack) once per\nthread?\n\njust so we can move forward with a better design will send a proposed\npatch that does things\na little be better as an RFC\n\n> > context for the\n> > most common case (when the locale is either C/POSIX or UTF-8) and therefore\n> > have a smaller impact on performance.\n>\n> I am not sure about the impact on performance, but if it helps us\n> keep the subsystem reusable by avoiding function-scope static that\n> we cannot clear, that would be a good thing. But \"struct grep_pat\"\n> may be a bit too fine-grained to control general context.\n\nthe \"performance\" point I was making was that with the current code\nthe chartable is only created when it is strictly needed (meaning the\npattern/haystack will do matching in non UTF-8 mode but with characters\nwith code higher than 127 and therefore MUST agree in a codepage)\n\nmost of the time (like when using UTF-8) the chartable (and therefore the\nglobal context) is not needed (even when using alternate allocators)\n\nthere is a chance that PCRE2 might perform better with NED, but not\nin my system and since we haven't been using NED with PCRE2 until\nthis proposed change it might be better to do that independently anyway.\n\nCarlo\n"},{"id":"379984","messageId":"20190806085014.47776-1-carenas@gmail.com","threadId":"51590","inReplyTo":"xmqq36ifmg7v.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/3] grep: no leaks (WIP)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T08:50:11Z","receivedAt":"2019-08-06T08:50:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This is an incomplete reroll for cb/pcre2-chartables-leakfix that attempts\nto address the root cause of the problem reported[1] by Dscho with PCRE2\nand that is that until now PCRE and NED never worked together.\n\nThe first patch is likely correct but since I'd been unable to replicate\nthe issue I can't be completely sure, if a kind soul with access to a\nwindows developer environment could give it a try we will know for sure\nbut FWIW it runs fine and doesn't introduce any failures in our tests\n\nThe second patch is obviously incomplete but should address the problem\nreported; there are still more things to consider as explained there\n\nThe third patch is the original leak patch rebased on top.\n\nCarlo Marcelo Arenas Belón (3):\n grep: make PCRE1 aware of custom allocator\n grep: make PCRE2 aware of custom allocator\n grep: avoid leak of chartables in PCRE2\n\n Makefile | 2 +-\n builtin/grep.c | 1 +\n grep.c | 46 ++++++++++++++++++++++++++++++++++++++++++----\n grep.h | 2 ++\n 4 files changed, 46 insertions(+), 5 deletions(-)\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/\n-- \n2.23.0.rc1\n"},{"id":"379985","messageId":"20190806085014.47776-2-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806085014.47776-1-carenas@gmail.com","subject":"[PATCH 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T08:50:12Z","receivedAt":"2019-08-06T08:50:18Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\nto override the system alocator, and so it is incompatible with\nUSE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n\nMake the minimum change possible to ensure this combination is supported\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 2 +-\n grep.c | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex bd246f2989..4b384f3759 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF\n endif\n \n ifdef USE_NED_ALLOCATOR\n-\tCOMPAT_CFLAGS += -Icompat/nedmalloc\n+\tCOMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n \tCOMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n \tOVERRIDE_STRDUP = YesPlease\n endif\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..0154998695 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n+ *\n+ * If using PCRE make sure that the library is configured\n+ * to use the right allocator (ex: NED)\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#ifdef USE_NED_ALLOCATOR\n+#ifdef USE_LIBPCRE1\n+\tpcre_malloc = malloc;\n+\tpcre_free = free;\n+#endif\n+#endif\n+\n \tmemset(opt, 0, sizeof(*opt));\n \topt->repo = repo;\n \topt->prefix = prefix;\n-- \n2.23.0.rc1\n\n"},{"id":"379986","messageId":"20190806085014.47776-4-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806085014.47776-1-carenas@gmail.com","subject":"[PATCH 3/3] grep: avoid leak of chartables in PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T08:50:14Z","receivedAt":"2019-08-06T08:50:22Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\na small memory leak visible with valgrind in t7813.\n\nComplete the creation of a PCRE2 specific variable that was missing from\nthe original change and free the generated table just like it is done\nfor PCRE1.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n grep.c | 7 ++++---\n grep.h | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex e748a6d68c..3e14ec91a6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -524,7 +524,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -535,9 +534,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n+\t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n@@ -642,6 +642,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 526c2db9ef..533165c21a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -96,6 +96,7 @@ struct grep_pat {\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n+\tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n-- \n2.23.0.rc1\n\n"},{"id":"379987","messageId":"20190806085014.47776-3-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806085014.47776-1-carenas@gmail.com","subject":"[PATCH 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T08:50:13Z","receivedAt":"2019-08-06T08:50:22Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Most of the code stolen from[1] to easy on comparison and including\nthe deficiency of setting the global context even for patterns that\nwon't need it.\n\nIdeally, the call from grep_init could be moved to a place where it\ncould be set without needing a lock and at least with this approach\nwe have a place to clear it (which is obviously missing more callers,\nbut at least shows how it will look for the grep subcommand)\n\nI had also dropped most other users of the global context in a failed\nattempt to make the change smaller, but also to keep the current\nbehaviour so that we could see the effect of enabling NED for PCRE2\nmore clearly.\n\nSadly, that will likely require a Windows box, as NED (at least our\nversion) is horribly broken in macOS (maybe it wasn't 64 bit clean)\nand in Linux builds, but I can't reproduce your crasher and it is\nmost likely slower than the system malloc.\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/\n\nSuggested-by: Johannes Schindelin \n---\n builtin/grep.c | 1 +\n grep.c | 31 +++++++++++++++++++++++++++++--\n grep.h | 1 +\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..e49c20df60 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/grep.c b/grep.c\nindex 0154998695..e748a6d68c 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,\n \n static struct grep_opt grep_defaults;\n \n+#ifdef USE_LIBPCRE2\n+static pcre2_general_context *pcre2_global_context;\n+\n+static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)\n+{\n+\treturn malloc(size);\n+}\n+\n+static void pcre2_free(void *pointer, void *memory_data)\n+{\n+\treturn free(pointer);\n+}\n+#endif\n+\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -153,6 +167,7 @@ int grep_config(const char *var, const char *value, void *cb)\n *\n * If using PCRE make sure that the library is configured\n * to use the right allocator (ex: NED)\n+ * if any object is created it should be cleaned up in grep_destroy()\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n@@ -164,6 +179,10 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \tpcre_malloc = malloc;\n \tpcre_free = free;\n #endif\n+#ifdef USE_LIBPCRE2\n+\tpcre2_global_context = pcre2_general_context_create(pcre2_malloc,\n+\t\t\t\t\t\t\tpcre2_free, NULL);\n+#endif\n #endif\n \n \tmemset(opt, 0, sizeof(*opt));\n@@ -188,6 +207,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n+void grep_destroy(void)\n+{\n+#ifdef USE_LIBPCRE2\n+\tpcre2_general_context_free(pcre2_global_context);\n+#endif\n+}\n+\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -509,7 +535,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n \t\t}\n@@ -560,7 +586,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\treturn;\n \t\t}\n \n-\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);\n+\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024,\n+\t\t\t\t\tpcre2_global_context);\n \t\tif (!p->pcre2_jit_stack)\n \t\t\tdie(\"Couldn't allocate PCRE2 JIT stack\");\n \t\tp->pcre2_match_context = pcre2_match_context_create(NULL);\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..526c2db9ef 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -189,6 +189,7 @@ struct grep_opt {\n void init_grep_defaults(struct repository *);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n+void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \n2.23.0.rc1\n\n"},{"id":"380011","messageId":"20190806163658.66932-1-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806085014.47776-1-carenas@gmail.com","subject":"[RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T16:36:55Z","receivedAt":"2019-08-06T16:37:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series is a candidate reroll for cb/pcre2-chartables-leakfix, that\nhopefully addresses the root cause of the problem reported by Dscho in\nWindows, where the PCRE2 library wasn't aware of the custom allocator and\nwas returning a pointer created with the system malloc but passing it to\nNED's free, resulting in a segfault.\n\nThe reason why it was triggered by the original leak fix is the layering\nviolation reported by René and that is exclusive to PCRE2 (hence why it\nhasn't been reported with PCRE1). The first patch could be droped and\nthen used in a different series that will fully integrate the custom\nallocator with the PCRE library and that is currently missing with PCRE2.\n\nEitherway, since I am unable to replicate the original bug or take\nperformance numbers in a representative environment without Windows\nthis is only published as an RFC, eventhough it has been tested and\nconsidered mostly complete.\n\nJunio, could you comment in my assumption that the use of grep in\nrevision.c doesn't require initializing a PCRE2 global context and\ntherefore not doing the cleanup?\n\nCarlo Marcelo Arenas Belón (3):\n grep: make PCRE1 aware of custom allocator\n grep: make PCRE2 aware of custom allocator\n grep: avoid leak of chartables in PCRE2\n\n Makefile | 2 +-\n builtin/grep.c | 1 +\n grep.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---\n grep.h | 2 ++\n 4 files changed, 52 insertions(+), 4 deletions(-)\n\n-- \n2.23.0.rc1\n"},{"id":"380012","messageId":"20190806163658.66932-3-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806163658.66932-1-carenas@gmail.com","subject":"[RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T16:36:57Z","receivedAt":"2019-08-06T16:37:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\na way to override the system allocator, and so it is incompatible with\nUSE_NED_ALLOCATOR. The problem was made visible when an attempt to\navoid a leak in a data structure that is created by the library was\npassed to NED's free for disposal triggering a segfault in Windows.\n\nPCRE2 requires the use of a general context to override the allocator\nand therefore, there is a lot more code needed than in PCRE1, including\na couple of wrapper functions.\n\nExtend the grep API with a \"destructor\" that could be called to cleanup\nany objects that were created and used globally.\n\nUpdate builtin/grep to use that new API, but any other future\nusers should make sure to have matching grep_init/grep_destroy calls\nif they are using the pattern matching functionality (currently only\nrelevant when using both NED and PCRE2)\n\nMove some of the logic that was before done per thread (in the workers)\ninto an earlier phase to avoid degrading performance, but as the use\nof PCRE2 with NED is better understood it is expected more of its\nfunctions will be instructed to use the custom allocator as well as\nwas done in the original code[1] this work was based on.\n\n[1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n\nReported-by: Johannes Schindelin \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/grep.c | 1 +\n grep.c | 36 +++++++++++++++++++++++++++++++++++-\n grep.h | 1 +\n 3 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..e49c20df60 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/grep.c b/grep.c\nindex 0154998695..3e5bdf94a6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -16,6 +16,22 @@ static int grep_source_is_binary(struct grep_source *gs,\n \n static struct grep_opt grep_defaults;\n \n+#ifdef USE_LIBPCRE2\n+static pcre2_general_context *pcre2_global_context;\n+\n+#ifdef USE_NED_ALLOCATOR\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn malloc(size);\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn free(pointer);\n+}\n+#endif\n+#endif\n+\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)\n *\n * If using PCRE make sure that the library is configured\n * to use the right allocator (ex: NED)\n+ * if any object is created it should be cleaned up in grep_destroy()\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n@@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n+void grep_destroy(void)\n+{\n+#ifdef USE_LIBPCRE2\n+\tpcre2_general_context_free(pcre2_global_context);\n+#endif\n+}\n+\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -319,6 +343,11 @@ void append_header_grep_pattern(struct grep_opt *opt,\n void append_grep_pattern(struct grep_opt *opt, const char *pat,\n \t\t\t const char *origin, int no, enum grep_pat_token t)\n {\n+#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n+\tif (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))\n+\t\tpcre2_global_context = pcre2_general_context_create(\n+\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n+#endif\n \tappend_grep_pat(opt, pat, strlen(pat), origin, no, t);\n }\n \n@@ -507,9 +536,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tp->pcre2_compile_context = NULL;\n \n+\t/* pcre2_global_context is initialized in append_grep_pattern */\n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+#ifdef USE_NED_ALLOCATOR\n+\t\t\tif (!pcre2_global_context)\n+\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n+#endif\n+\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n \t\t}\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..526c2db9ef 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -189,6 +189,7 @@ struct grep_opt {\n void init_grep_defaults(struct repository *);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n+void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \n2.23.0.rc1\n\n"},{"id":"380013","messageId":"20190806163658.66932-2-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806163658.66932-1-carenas@gmail.com","subject":"[RFC PATCH v3 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T16:36:56Z","receivedAt":"2019-08-06T16:37:04Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\nto override the system alocator, and so it is incompatible with\nUSE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n\nMake the minimum change possible to ensure this combination is supported\nby extending grep_init to set the PCRE1 specific functions to the NED\nversions (using the aliased names though) and therefore making sure all\nalocations are done inside PCRE1 with the same allocator than the rest\nof git.\n\nThis change might have performance impact (hopefully for the best) and\nso will be good idea to test it in a platform where NED might have a\npositive impact (ex: Windows 7)\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Makefile | 2 +-\n grep.c | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex bd246f2989..4b384f3759 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF\n endif\n \n ifdef USE_NED_ALLOCATOR\n-\tCOMPAT_CFLAGS += -Icompat/nedmalloc\n+\tCOMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n \tCOMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n \tOVERRIDE_STRDUP = YesPlease\n endif\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..0154998695 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n+ *\n+ * If using PCRE make sure that the library is configured\n+ * to use the right allocator (ex: NED)\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#ifdef USE_NED_ALLOCATOR\n+#ifdef USE_LIBPCRE1\n+\tpcre_malloc = malloc;\n+\tpcre_free = free;\n+#endif\n+#endif\n+\n \tmemset(opt, 0, sizeof(*opt));\n \topt->repo = repo;\n \topt->prefix = prefix;\n-- \n2.23.0.rc1\n\n"},{"id":"380014","messageId":"20190806163658.66932-4-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806163658.66932-1-carenas@gmail.com","subject":"[RFC PATCH v3 3/3] grep: avoid leak of chartables in PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-06T16:36:58Z","receivedAt":"2019-08-06T16:37:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\na small memory leak visible with valgrind in t7813.\n\nComplete the creation of a PCRE2 specific variable that was missing from\nthe original change and free the generated table just like it is done\nfor PCRE1.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n grep.c | 7 ++++---\n grep.h | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 3e5bdf94a6..3d3ea0414e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -527,7 +527,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -543,9 +542,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tif (!pcre2_global_context)\n \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n #endif\n-\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n+\t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n@@ -649,6 +649,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 526c2db9ef..533165c21a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -96,6 +96,7 @@ struct grep_pat {\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n+\tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n-- \n2.23.0.rc1\n\n"},{"id":"380015","messageId":"xmqq5znakzpb.fsf@gitster-ct.c.googlers.com","threadId":"51590","inReplyTo":"20190806163658.66932-1-carenas@gmail.com","subject":"Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-06T16:48:00Z","receivedAt":"2019-08-06T16:48:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> Junio, could you comment in my assumption that the use of grep in\n> revision.c doesn't require initializing a PCRE2 global context and\n> therefore not doing the cleanup?\n\nMany callers of setup_revisions() do so only to run two-thing diffs\n(e.g. run_diff_files() that compares the index and the working tree),\nbut some do so to traverse the history with the grep_filter\n(e.g. \"git log --grep=\"). \"git log -P\" may affect how that\npattern is used to match the contents of the log messages, no?\n"},{"id":"380059","messageId":"ab8a378c-0a60-9554-b2dd-ecb3d05229cb@web.de","threadId":"51590","inReplyTo":"20190806163658.66932-3-carenas@gmail.com","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-07T05:38:09Z","receivedAt":"2019-08-07T05:38:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.08.19 um 18:36 schrieb Carlo Marcelo Arenas Belón:\n> 94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\n> a way to override the system allocator, and so it is incompatible with\n> USE_NED_ALLOCATOR. The problem was made visible when an attempt to\n> avoid a leak in a data structure that is created by the library was\n> passed to NED's free for disposal triggering a segfault in Windows.\n>\n> PCRE2 requires the use of a general context to override the allocator\n> and therefore, there is a lot more code needed than in PCRE1, including\n> a couple of wrapper functions.\n>\n> Extend the grep API with a \"destructor\" that could be called to cleanup\n> any objects that were created and used globally.\n>\n> Update builtin/grep to use that new API, but any other future\n> users should make sure to have matching grep_init/grep_destroy calls\n> if they are using the pattern matching functionality (currently only\n> relevant when using both NED and PCRE2)\n>\n> Move some of the logic that was before done per thread (in the workers)\n> into an earlier phase to avoid degrading performance\n\nWhich logic is moved? In the patch I basically only see additions.\n\n>, but as the use\n> of PCRE2 with NED is better understood it is expected more of its\n> functions will be instructed to use the custom allocator as well as\n> was done in the original code[1] this work was based on.\n>\n> [1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n\nI'm not sure I understand that part. Do you mean there are gaps of\nknowledge about nedmalloc and/or PCRE2 and/or their interaction? And\nsomeone is working on closing those gaps, and is going to submit more\npatches in the process?\n\nYour patch uses a custom global context only if USE_NED_ALLOCATOR is\ndefined, while [1] does it unconditionally. The latter is easier to\ndebug and requires less preprocessor directives. What's the upside\nof your approach?\n\n>\n> Reported-by: Johannes Schindelin \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> builtin/grep.c | 1 +\n> grep.c | 36 +++++++++++++++++++++++++++++++++++-\n> grep.h | 1 +\n> 3 files changed, 37 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 560051784e..e49c20df60 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\trun_pager(&opt, prefix);\n> \tclear_pathspec(&pathspec);\n> \tfree_grep_patterns(&opt);\n> +\tgrep_destroy();\n> \treturn !hit;\n> }\n> diff --git a/grep.c b/grep.c\n> index 0154998695..3e5bdf94a6 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -16,6 +16,22 @@ static int grep_source_is_binary(struct grep_source *gs,\n>\n> static struct grep_opt grep_defaults;\n>\n> +#ifdef USE_LIBPCRE2\n> +static pcre2_general_context *pcre2_global_context;\n> +\n> +#ifdef USE_NED_ALLOCATOR\n> +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> +{\n> +\treturn malloc(size);\n\nShould this be xmalloc() to get consistent out-of-memory handling?\n\n> +}\n> +\n> +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> +{\n> +\treturn free(pointer);\n> +}\n> +#endif\n> +#endif\n> +\n> static const char *color_grep_slots[] = {\n> \t[GREP_COLOR_CONTEXT]\t = \"context\",\n> \t[GREP_COLOR_FILENAME]\t = \"filename\",\n> @@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)\n> *\n> * If using PCRE make sure that the library is configured\n> * to use the right allocator (ex: NED)\n> + * if any object is created it should be cleaned up in grep_destroy()\n> */\n> void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n> {\n> @@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n> \t\tcolor_set(opt->colors[i], def->colors[i]);\n> }\n>\n> +void grep_destroy(void)\n> +{\n> +#ifdef USE_LIBPCRE2\n> +\tpcre2_general_context_free(pcre2_global_context);\n> +#endif\n> +}\n> +\n> static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n> {\n> \t/*\n> @@ -319,6 +343,11 @@ void append_header_grep_pattern(struct grep_opt *opt,\n> void append_grep_pattern(struct grep_opt *opt, const char *pat,\n> \t\t\t const char *origin, int no, enum grep_pat_token t)\n> {\n> +#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n> +\tif (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))\n> +\t\tpcre2_global_context = pcre2_general_context_create(\n> +\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n> +#endif\n> \tappend_grep_pat(opt, pat, strlen(pat), origin, no, t);\n> }\n>\n> @@ -507,9 +536,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>\n> \tp->pcre2_compile_context = NULL;\n>\n> +\t/* pcre2_global_context is initialized in append_grep_pattern */\n> \tif (opt->ignore_case) {\n> \t\tif (has_non_ascii(p->pattern)) {\n> -\t\t\tcharacter_tables = pcre2_maketables(NULL);\n> +#ifdef USE_NED_ALLOCATOR\n> +\t\t\tif (!pcre2_global_context)\n> +\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n\n[1] initializes on demand. Why not do that? To avoid race conditions\nthat would lead to occasional double-allocation of the global context?\n\n> +#endif\n> +\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n> \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n\nDon't you want to pass pcre2_global_context here as well? And [1] even\nuses it in some more places.\n\nOh, that's the \"expected more\" when \"better understood\" part from the\ncommit message, right?\n\nBasically I'd expect the custom global context to be used for all PCRE2\nallocations; I can't think of a reason for mixing allocators (e.g.\nsystem malloc for PCRE2 regexes and nedmalloc for everything else).\n\n> \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n> \t\t}\n> diff --git a/grep.h b/grep.h\n> index 1875880f37..526c2db9ef 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -189,6 +189,7 @@ struct grep_opt {\n> void init_grep_defaults(struct repository *);\n> int grep_config(const char *var, const char *value, void *);\n> void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n> +void grep_destroy(void);\n> void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n>\n> void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n>\n"},{"id":"380060","messageId":"CAPUEspip98Mq8FrKTOkEikZhaLPprZXf=E2x3d0b7=c7e5+Gyw@mail.gmail.com","threadId":"51590","inReplyTo":"ab8a378c-0a60-9554-b2dd-ecb3d05229cb@web.de","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-07T09:49:56Z","receivedAt":"2019-08-07T09:50:10Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Aug 6, 2019 at 10:38 PM René Scharfe wrote:\n>\n> Am 06.08.19 um 18:36 schrieb Carlo Marcelo Arenas Belón:\n> > Move some of the logic that was before done per thread (in the workers)\n> > into an earlier phase to avoid degrading performance\n>\n> Which logic is moved? In the patch I basically only see additions.\n\nthe decision of if we want to create a global context or not, which is now being\ndone only once per pattern. \"moving\" there was to imply the logic now\nwas no longer going to always use state in the struct_pat that then\nwill be evaluated at least once per thread, but could use state\nglobally and reuse some of the work (as I did for the generation of\nthe chartables in a later patch, that I had yet to send)\n\n> >, but as the use\n> > of PCRE2 with NED is better understood it is expected more of its\n> > functions will be instructed to use the custom allocator as well as\n> > was done in the original code[1] this work was based on.\n> >\n> > [1] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n>\n> I'm not sure I understand that part. Do you mean there are gaps of\n> knowledge about nedmalloc and/or PCRE2 and/or their interaction? And\n> someone is working on closing those gaps, and is going to submit more\n> patches in the process?\n>\n> Your patch uses a custom global context only if USE_NED_ALLOCATOR is\n> defined, while [1] does it unconditionally. The latter is easier to\n> debug and requires less preprocessor directives. What's the upside\n> of your approach?\n\nwas hoping will perform better but it seems that testing can be done\nonly in windows\n\n> > +#ifdef USE_NED_ALLOCATOR\n> > +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> > +{\n> > + return malloc(size);\n>\n> Should this be xmalloc() to get consistent out-of-memory handling?\n\ngood point, note though that since it is inside a USE_NED_ALLOCATOR\nifdef it is really\nnedalloc in disguise\n>\n> > +}\n> > +\n> > +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> > +{\n> > + return free(pointer);\n> > +}\n> > +#endif\n> > +#endif\n> > +\n> > static const char *color_grep_slots[] = {\n> > [GREP_COLOR_CONTEXT] = \"context\",\n> > [GREP_COLOR_FILENAME] = \"filename\",\n> > @@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)\n> > *\n> > * If using PCRE make sure that the library is configured\n> > * to use the right allocator (ex: NED)\n> > + * if any object is created it should be cleaned up in grep_destroy()\n> > */\n> > void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n> > {\n> > @@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n> > color_set(opt->colors[i], def->colors[i]);\n> > }\n> >\n> > +void grep_destroy(void)\n> > +{\n> > +#ifdef USE_LIBPCRE2\n> > + pcre2_general_context_free(pcre2_global_context);\n> > +#endif\n> > +}\n> > +\n> > static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n> > {\n> > /*\n> > @@ -319,6 +343,11 @@ void append_header_grep_pattern(struct grep_opt *opt,\n> > void append_grep_pattern(struct grep_opt *opt, const char *pat,\n> > const char *origin, int no, enum grep_pat_token t)\n> > {\n> > +#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n> > + if (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))\n> > + pcre2_global_context = pcre2_general_context_create(\n> > + pcre2_malloc, pcre2_free, NULL);\n> > +#endif\n> > append_grep_pat(opt, pat, strlen(pat), origin, no, t);\n> > }\n> >\n> > @@ -507,9 +536,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> >\n> > p->pcre2_compile_context = NULL;\n> >\n> > + /* pcre2_global_context is initialized in append_grep_pattern */\n> > if (opt->ignore_case) {\n> > if (has_non_ascii(p->pattern)) {\n> > - character_tables = pcre2_maketables(NULL);\n> > +#ifdef USE_NED_ALLOCATOR\n> > + if (!pcre2_global_context)\n> > + BUG(\"pcre2_global_context uninitialized\");\n>\n> [1] initializes on demand. Why not do that? To avoid race conditions\n\nthis was just for help migrating the code, could go away even now, but\nthough it was nice to keep for safety (as someone mentioned recently)\n\n> that would lead to occasional double-allocation of the global context?\n\nand a nicer API that allows for cleaning up any global objects (with\nthe chartables being an easy one to tackle next) and might\nhelp in the future to make the worker threads less messy (ex: compile vs match)\n\n> > +#endif\n> > + character_tables = pcre2_maketables(pcre2_global_context);\n> > p->pcre2_compile_context = pcre2_compile_context_create(NULL);\n>\n> Don't you want to pass pcre2_global_context here as well? And [1] even\n> uses it in some more places.\n>\n> Oh, that's the \"expected more\" when \"better understood\" part from the\n> commit message, right?\n\ncorrect, was trying to be conservative and minimal (since this code\nwill conflict as well\nwith other in the fly changes), but considering that the last Azure\nbuild from pu still failed\nand I have no windows box to debug, might need to do it anyway.\n\n> Basically I'd expect the custom global context to be used for all PCRE2\n> allocations; I can't think of a reason for mixing allocators (e.g.\n> system malloc for PCRE2 regexes and nedmalloc for everything else).\n\nok\n\nCarlo\n"},{"id":"380068","messageId":"c7f08e19-88a7-ca7f-90b9-54465e621d49@web.de","threadId":"51590","inReplyTo":"CAPUEspip98Mq8FrKTOkEikZhaLPprZXf=E2x3d0b7=c7e5+Gyw@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-07T13:02:57Z","receivedAt":"2019-08-07T13:03:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.08.19 um 11:49 schrieb Carlo Arenas:\n> was hoping will perform better but it seems that testing can be done\n> only in windows\n\nnedmalloc works on other platforms as well. On Debian Testing with GCC\n9.1.0 I need two changes to suppress some compiler warnings, though.\nWill post them as replies.\n\n\"make USE_NED_ALLOCATOR=1 test\" then reports these failures:\n\nt7816-grep-binary-pattern.sh (Wstat: 256 Tests: 145 Failed: 5)\n Failed tests: 48, 54, 57, 60, 63\n Non-zero exit status: 1\n\nAnd the first one when running that test with --verbose and --immediate\nis showing:\n\nBUG: grep.c:510: pcre2_global_context uninitialized\nAborted\nnot ok 48 - LC_ALL='C' git grep -P -f f -i '[æ]ð' a\n#\n#\t\t\t\tprintf '[æ]Qð' | q_to_nul >f &&\n#\t\t\t\tLC_ALL='C' git grep -P -f f -i a\n#\n"},{"id":"380069","messageId":"5077f91b-958b-bf00-565f-7b96aa05a614@web.de","threadId":"51590","inReplyTo":"c7f08e19-88a7-ca7f-90b9-54465e621d49@web.de","subject":"[PATCH 1/2] nedmalloc: do assignments only after the declaration section","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-07T13:08:51Z","receivedAt":"2019-08-07T13:08:58Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid the following compiler warning:\n\nIn file included from compat/nedmalloc/nedmalloc.c:63:\ncompat/nedmalloc/malloc.c.h: In function ‘pthread_release_lock’:\ncompat/nedmalloc/malloc.c.h:1759:5: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]\n 1759 | volatile unsigned int* lp = &sl->l;\n | ^~~~~~~~\n\nSigned_off-by: René Scharfe \n---\n compat/nedmalloc/malloc.c.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h\nindex b833ff9225..88c131ca93 100644\n--- a/compat/nedmalloc/malloc.c.h\n+++ b/compat/nedmalloc/malloc.c.h\n@@ -1755,10 +1755,10 @@ static FORCEINLINE void pthread_release_lock (MLOCK_T *sl) {\n assert(sl->l != 0);\n assert(sl->threadid == CURRENT_THREAD);\n if (--sl->c == 0) {\n- sl->threadid = 0;\n volatile unsigned int* lp = &sl->l;\n int prev = 0;\n int ret;\n+ sl->threadid = 0;\n __asm__ __volatile__ (\"lock; xchgl %0, %1\"\n \t\t\t : \"=r\" (ret)\n \t\t\t : \"m\" (*(lp)), \"0\"(prev)\n--\n2.22.0\n"},{"id":"380070","messageId":"5933e4d2-8141-efd9-9ed4-76946a84974b@web.de","threadId":"51590","inReplyTo":"c7f08e19-88a7-ca7f-90b9-54465e621d49@web.de","subject":"[PATCH 2/2] nedmalloc: avoid compiler warning about unused value","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-07T13:09:02Z","receivedAt":"2019-08-07T13:09:07Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Cast the evaluated value of the macro INITIAL_LOCK to void to instruct\nthe compiler that we're not interested in said value nor the following\nwarning:\n\nIn file included from compat/nedmalloc/nedmalloc.c:63:\ncompat/nedmalloc/malloc.c.h: In function ‘init_user_mstate’:\ncompat/nedmalloc/malloc.c.h:1706:62: error: right-hand operand of comma expression has no effect [-Werror=unused-value]\n 1706 | #define INITIAL_LOCK(sl) (memset(sl, 0, sizeof(MLOCK_T)), 0)\n | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~\ncompat/nedmalloc/malloc.c.h:5020:3: note: in expansion of macro ‘INITIAL_LOCK’\n 5020 | INITIAL_LOCK(&m->mutex);\n | ^~~~~~~~~~~~\n\nSigned-off-by: René Scharfe \n---\n compat/nedmalloc/malloc.c.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h\nindex 88c131ca93..9134349590 100644\n--- a/compat/nedmalloc/malloc.c.h\n+++ b/compat/nedmalloc/malloc.c.h\n@@ -3066,7 +3066,7 @@ static int init_mparams(void) {\n #if !ONLY_MSPACES\n /* Set up lock for main malloc area */\n gm->mflags = mparams.default_mflags;\n- INITIAL_LOCK(&gm->mutex);\n+ (void)INITIAL_LOCK(&gm->mutex);\n #endif\n\n #if (FOOTERS && !INSECURE)\n@@ -5017,7 +5017,7 @@ static mstate init_user_mstate(char* tbase, size_t tsize) {\n mchunkptr msp = align_as_chunk(tbase);\n mstate m = (mstate)(chunk2mem(msp));\n memset(m, 0, msize);\n- INITIAL_LOCK(&m->mutex);\n+ (void)INITIAL_LOCK(&m->mutex);\n msp->head = (msize|PINUSE_BIT|CINUSE_BIT);\n m->seg.base = m->least_addr = tbase;\n m->seg.size = m->footprint = m->max_footprint = tsize;\n--\n2.22.0\n"},{"id":"380085","messageId":"xmqq1rxwj0zb.fsf@gitster-ct.c.googlers.com","threadId":"51590","inReplyTo":"CAPUEspip98Mq8FrKTOkEikZhaLPprZXf=E2x3d0b7=c7e5+Gyw@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-07T18:15:36Z","receivedAt":"2019-08-07T18:15:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n>> > +#ifdef USE_NED_ALLOCATOR\n>> > +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n>> > +{\n>> > + return malloc(size);\n>>\n>> Should this be xmalloc() to get consistent out-of-memory handling?\n>\n> good point, note though that since it is inside a USE_NED_ALLOCATOR\n> ifdef it is really\n> nedalloc in disguise\n\nIt would be nedalloc() wrapped inside the \"die if we cannot\nallocate, possibly after releasing resources held in various caches\"\nxmalloc() wrapper. So (1) either xmalloc or malloc would end up\neventually calling nedalloc that can be freed with nedfree, so from\nthat point of view either can be used without upsetting \"free()\",\nand (2) we should use xmalloc() here because we care about\nconsistent OOM behaviour throughout the system.\n\nThanks.\n"},{"id":"380100","messageId":"20190807213945.10464-1-carenas@gmail.com","threadId":"51590","inReplyTo":"20190806163658.66932-1-carenas@gmail.com","subject":"[RFC PATCH v4 0/3] grep: no leaks or crashes (windows testing needed)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-07T21:39:42Z","receivedAt":"2019-08-07T21:39:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series is a candidate reroll for cb/pcre2-chartables-leakfix, that\nhopefully addresses the root cause of the problem reported by Dscho in\nWindows, where the PCRE2 library wasn't aware of the custom allocator and\nwas returning a pointer created with the system malloc but passing it to\nNED's free, resulting in a segfault.\n\nThe reason why it was triggered by the original leak fix is the layering\nviolation reported by René and that is exclusive to PCRE2 (hence why it\nhasn't been reported with PCRE1). Additional work might be available\nin a future release of PCRE2 to address that as detailed in the upstream\nbug[1] report.\n\nEitherway, since I am unable to replicate the original bug or take\nperformance numbers in a representative environment without Windows\nthis is only published as an RFC,\n\nChanges since v3 (mostly in patch 2):\n\n* git log also calls the \"destructor\" for grep API\n* no more \"bug\" being triggered by `make test`, sorry René\n* hopefully no more crashes in windows (I was expecting at most a BUG)\n\nFuture work (other than the needed refactoring explained in the\nsecond patch) and adjacent bugs, includes:\n\n* tracking more possible users of the grep API that might need to call\n grep_destroy()\n* completely moving PCRE2 to use NED (as is done with PCRE1 and was\n proposed on the original patch[2] this is based on\n* build on top of the new API so that other work could be shared\n (for example the chartables that started this whole mess)\n\nor (hopefully not)\n\n* ignore the original leak (maybe with an UNLEAK) as René suggested [3]\n* discard this work and just use Dscho's fix (probably with some improvements)\n\nCarlo Marcelo Arenas Belón (3):\n grep: make PCRE1 aware of custom allocator\n grep: make PCRE2 aware of custom allocator\n grep: avoid leak of chartables in PCRE2\n\n Makefile | 2 +-\n builtin/grep.c | 1 +\n builtin/log.c | 1 +\n grep.c | 77 ++++++++++++++++++++++++++++++++++++++++++++------\n grep.h | 2 ++\n 5 files changed, 73 insertions(+), 10 deletions(-)\n\n[1] https://bugs.exim.org/show_bug.cgi?id=2429\n[2] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n[3] https://public-inbox.org/git/7ec60d57-9940-35f2-f7b5-c87d4dc7cdd5@web.de/\n\nbase-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd\n-- \n2.23.0.rc1\n"},{"id":"380101","messageId":"20190807213945.10464-2-carenas@gmail.com","threadId":"51590","inReplyTo":"20190807213945.10464-1-carenas@gmail.com","subject":"[RFC PATCH v4 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-07T21:39:43Z","receivedAt":"2019-08-07T21:39:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\nto override the system alocator, and so it is incompatible with\nUSE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n\nMake the minimum change possible to ensure this combination is supported\nby extending grep_init to set the PCRE1 specific functions to the NED\nversions (using the aliased names though) and therefore making sure all\nalocations are done inside PCRE1 with the same allocator than the rest\nof git.\n\nThis change might have performance impact (hopefully for the best) and\nso will be good idea to test it in a platform where NED might have a\npositive impact (ex: Windows 7)\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Junio C Hamano \n---\n Makefile | 2 +-\n grep.c | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex bd246f2989..4b384f3759 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF\n endif\n \n ifdef USE_NED_ALLOCATOR\n-\tCOMPAT_CFLAGS += -Icompat/nedmalloc\n+\tCOMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n \tCOMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n \tOVERRIDE_STRDUP = YesPlease\n endif\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..0154998695 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n+ *\n+ * If using PCRE make sure that the library is configured\n+ * to use the right allocator (ex: NED)\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#ifdef USE_NED_ALLOCATOR\n+#ifdef USE_LIBPCRE1\n+\tpcre_malloc = malloc;\n+\tpcre_free = free;\n+#endif\n+#endif\n+\n \tmemset(opt, 0, sizeof(*opt));\n \topt->repo = repo;\n \topt->prefix = prefix;\n-- \n2.23.0.rc1\n\n"},{"id":"380102","messageId":"20190807213945.10464-3-carenas@gmail.com","threadId":"51590","inReplyTo":"20190807213945.10464-1-carenas@gmail.com","subject":"[RFC PATCH v4 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-07T21:39:44Z","receivedAt":"2019-08-07T21:40:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\na way to override the system allocator, and so it is incompatible with\nUSE_NED_ALLOCATOR. The problem was made visible when an attempt to\navoid a leak in a data structure that is created by the library was\npassed to NED's free for disposal triggering a segfault in Windows.\n\nPCRE2 requires the use of a general context to override the allocator\nand therefore, there is a lot more code needed than in PCRE1, including\na couple of wrapper functions.\n\nExtend the grep API with a \"destructor\" that could be called to cleanup\nany objects that were created and used globally.\n\nUpdate builtin/{grep,log} to use that new API, but any other future\nusers should make sure to have matching grep_init/grep_destroy calls\nif they are using the pattern matching functionality (currently only\nrelevant when using both NED and PCRE2)\n\nMove the logic to decide if a general context will be needed to an\nearlier phase so it will only be done once per pattern (instead of\nat least once per worker thread) avoiding then the need for locking.\n\nThis change does the minimum change required to hopefully solve the\ncrash, with the rest of the users of it added later.\n\nHelped-by: René Scharfe \nReported-by: Johannes Schindelin \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nV4:\n* use xmalloc instead as suggested by René and Junio\n* \"fix\" for regression in t7816 as reported by René\n\n builtin/grep.c | 1 +\n builtin/log.c | 1 +\n grep.c | 62 ++++++++++++++++++++++++++++++++++++++++++++------\n grep.h | 1 +\n 4 files changed, 58 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..e49c20df60 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 1cf9e37736..139b8770e7 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2146,6 +2146,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \t\tlist = list->next;\n \t}\n \n+\tgrep_destroy();\n \tfree_patch_ids(&ids);\n \treturn 0;\n }\ndiff --git a/grep.c b/grep.c\nindex 0154998695..8ee982e2e8 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -16,6 +16,22 @@ static int grep_source_is_binary(struct grep_source *gs,\n \n static struct grep_opt grep_defaults;\n \n+#ifdef USE_LIBPCRE2\n+static pcre2_general_context *pcre2_global_context;\n+\n+#ifdef USE_NED_ALLOCATOR\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn xmalloc(size); /* will use nedalloc underneath */\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn free(pointer);\n+}\n+#endif\n+#endif\n+\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -153,6 +169,7 @@ int grep_config(const char *var, const char *value, void *cb)\n *\n * If using PCRE make sure that the library is configured\n * to use the right allocator (ex: NED)\n+ * if any object is created it should be cleaned up in grep_destroy()\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n@@ -188,6 +205,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n+void grep_destroy(void)\n+{\n+#ifdef USE_LIBPCRE2\n+\tpcre2_general_context_free(pcre2_global_context);\n+#endif\n+}\n+\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -254,12 +278,29 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o\n \t\tgrep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);\n }\n \n-static struct grep_pat *create_grep_pat(const char *pat, size_t patlen,\n+static struct grep_pat *create_grep_pat(struct grep_opt *opt,\n+\t\t\t\t\tconst char *pat, size_t patlen,\n \t\t\t\t\tconst char *origin, int no,\n \t\t\t\t\tenum grep_pat_token t,\n \t\t\t\t\tenum grep_header_field field)\n {\n \tstruct grep_pat *p = xcalloc(1, sizeof(*p));\n+\n+#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n+\t/* BUG: this is technically not needed if we will do UTF matching\n+\t * but UTF locale detection is currently broken */\n+\t/* SMELL: opt shouldn't be needed at this level but it is used\n+\t * here to keep the way we were detecting the need for\n+\t * the chartable consistent and until it can be refactored\n+\t * properly. the NULL check is needed as a workaround\n+\t * for segfaulting in t7810.102 and should also go */\n+\t/* TODO: has_non_ascii doesn't support NUL in pattern */\n+\tif (!pcre2_global_context && opt && opt->ignore_case &&\n+\t\thas_non_ascii(pat))\n+\t\tpcre2_global_context = pcre2_general_context_create(\n+\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n+#endif\n+\n \tp->pattern = xmemdupz(pat, patlen);\n \tp->patternlen = patlen;\n \tp->origin = origin;\n@@ -291,8 +332,10 @@ static void do_append_grep_pat(struct grep_pat ***tail, struct grep_pat *p)\n \t\t\t}\n \t\t\tif (!nl)\n \t\t\t\tbreak;\n-\t\t\tnew_pat = create_grep_pat(nl + 1, len - 1, p->origin,\n-\t\t\t\t\t\t p->no, p->token, p->field);\n+\n+\t\t\tnew_pat = create_grep_pat(NULL, nl + 1, len - 1,\n+\t\t\t\t\t\tp->origin, p->no, p->token,\n+\t\t\t\t\t\tp->field);\n \t\t\tnew_pat->next = p->next;\n \t\t\tif (!p->next)\n \t\t\t\t*tail = &new_pat->next;\n@@ -309,8 +352,8 @@ static void do_append_grep_pat(struct grep_pat ***tail, struct grep_pat *p)\n void append_header_grep_pattern(struct grep_opt *opt,\n \t\t\t\tenum grep_header_field field, const char *pat)\n {\n-\tstruct grep_pat *p = create_grep_pat(pat, strlen(pat), \"header\", 0,\n-\t\t\t\t\t GREP_PATTERN_HEAD, field);\n+\tstruct grep_pat *p = create_grep_pat(opt, pat, strlen(pat), \"header\",\n+\t\t\t\t\t 0, GREP_PATTERN_HEAD, field);\n \tif (field == GREP_HEADER_REFLOG)\n \t\topt->use_reflog_filter = 1;\n \tdo_append_grep_pat(&opt->header_tail, p);\n@@ -325,7 +368,7 @@ void append_grep_pattern(struct grep_opt *opt, const char *pat,\n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen,\n \t\t const char *origin, int no, enum grep_pat_token t)\n {\n-\tstruct grep_pat *p = create_grep_pat(pat, patlen, origin, no, t, 0);\n+\tstruct grep_pat *p = create_grep_pat(opt, pat, patlen, origin, no, t, 0);\n \tdo_append_grep_pat(&opt->pattern_tail, p);\n }\n \n@@ -507,9 +550,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tp->pcre2_compile_context = NULL;\n \n+\t/* pcre2_global_context is initialized in append_grep_pattern */\n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+#ifdef USE_NED_ALLOCATOR\n+\t\t\tif (!pcre2_global_context)\n+\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n+#endif\n+\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n \t\t}\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..526c2db9ef 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -189,6 +189,7 @@ struct grep_opt {\n void init_grep_defaults(struct repository *);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n+void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \n2.23.0.rc1\n\n"},{"id":"380103","messageId":"20190807213945.10464-4-carenas@gmail.com","threadId":"51590","inReplyTo":"20190807213945.10464-1-carenas@gmail.com","subject":"[RFC PATCH v4 3/3] grep: avoid leak of chartables in PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-07T21:39:45Z","receivedAt":"2019-08-07T21:40:06Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\na small memory leak visible with valgrind in t7813.\n\nComplete the creation of a PCRE2 specific variable that was missing from\nthe original change and free the generated table just like it is done\nfor PCRE1.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Junio C Hamano \n---\n grep.c | 7 ++++---\n grep.h | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 8ee982e2e8..ccb6ab38a3 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -541,7 +541,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -557,9 +556,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tif (!pcre2_global_context)\n \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n #endif\n-\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n+\t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n@@ -663,6 +663,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 526c2db9ef..533165c21a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -96,6 +96,7 @@ struct grep_pat {\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n+\tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n-- \n2.23.0.rc1\n\n"},{"id":"380105","messageId":"xmqq4l2shap0.fsf@gitster-ct.c.googlers.com","threadId":"51590","inReplyTo":"20190807213945.10464-3-carenas@gmail.com","subject":"Re: [RFC PATCH v4 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-07T22:28:43Z","receivedAt":"2019-08-07T22:28:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\nAs we already have such an ifdef block here...\n\n> +#ifdef USE_LIBPCRE2\n> +static pcre2_general_context *pcre2_global_context;\n> +\n> +#ifdef USE_NED_ALLOCATOR\n> +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> +{\n> +\treturn xmalloc(size); /* will use nedalloc underneath */\n> +}\n> +\n> +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> +{\n> +\treturn free(pointer);\n> +}\n> +#endif\n> +#endif\n\n... perhaps an ugly ifdef block like this one ...\n\n> +static struct grep_pat *create_grep_pat(struct grep_opt *opt,\n> +\t\t\t\t\tconst char *pat, size_t patlen,\n> \t\t\t\t\tconst char *origin, int no,\n> \t\t\t\t\tenum grep_pat_token t,\n> \t\t\t\t\tenum grep_header_field field)\n> {\n> \tstruct grep_pat *p = xcalloc(1, sizeof(*p));\n> +\n> +#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n> +\t/* BUG: this is technically not needed if we will do UTF matching\n> +\t * but UTF locale detection is currently broken */\n> +\t/* SMELL: opt shouldn't be needed at this level but it is used\n> +\t * here to keep the way we were detecting the need for\n> +\t * the chartable consistent and until it can be refactored\n> +\t * properly. the NULL check is needed as a workaround\n> +\t * for segfaulting in t7810.102 and should also go */\n> +\t/* TODO: has_non_ascii doesn't support NUL in pattern */\n> +\tif (!pcre2_global_context && opt && opt->ignore_case &&\n> +\t\thas_non_ascii(pat))\n> +\t\tpcre2_global_context = pcre2_general_context_create(\n> +\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n> +#endif\n> +\n\n... can be abstracted away by *not* having the #if/#endif here and\ninstead have a line that looks like this:\n\n\tsetup_pcre2_as_needed(opt, pat);\n\nwhich implementation can be prepared near the top, close to where\npcre2_malloc/free are defined (or not) above, i.e.\n\n\t#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n\tstatic void setup_pcre2_as_needed(struct grep_opt *opt,\tconst char *pat)\n\t{\n\t\t... the above one ...\n\t}\n\t#else\n\t#define setup_pcre2_as_needed(ignore1, ignore2) /* nothing */\n\t#endif\n\nThe conditional code in grep_destroy() can be eliminated in a\nsimilar fashion, i.e.\n\n\tvoid grep_destroy(void)\n\t{\n\t\tcleanup_pcre2_as_needed();\n\t}\n\nwith\n\n\t#ifdef USE_LIBPCRE2\n\tstatic void cleanup_pcre2_as_needed(void)\n\t{\n\t\tpcre2_general_context_free(pcre2_global_context);\n\t}\n\t#else\n\t#define cleanup_pcre2_as_needed() /* nothing */\n\t#endif\n\nnear the top (the beneral idea is to \"call\" a helper function whose\nname describes what the call is for in a more general terms, and let\nonly the implementation details be hidden inside ifdef blocks).\n\nAlso, \n\n\t/* \n\t * our multi-line comments are supposed to be formatted like\n\t * this, with opening and closing slash-asterisk and asterisk-slash\n\t * on their own lines.\n\t */\n\n"},{"id":"380107","messageId":"CAPUEspj4CrEk6u4+8a5UBisxWsXcwOrOPQ5s9TktA6dZx5s+uQ@mail.gmail.com","threadId":"51590","inReplyTo":"c7f08e19-88a7-ca7f-90b9-54465e621d49@web.de","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-08T02:35:28Z","receivedAt":"2019-08-08T02:35:42Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Aug 7, 2019 at 6:03 AM René Scharfe wrote:\n>\n> Am 07.08.19 um 11:49 schrieb Carlo Arenas:\n> > was hoping will perform better but it seems that testing can be done\n> > only in windows\n>\n> nedmalloc works on other platforms as well.\n\nI meant[1] it works reliably enough to be useful for performance testing.\n\ngoes without saying that the fact that I am using a virtualbox with 2\nCPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)\nand the test uses by default 8 threads, doesn't help, but to share my\npain here is the result of running p7820 with my last reroll on top of\npu, comparing a build of the same code without NED (this tree) with\none with it (HEAD)\n\nTest this tree\nHEAD\n-------------------------------------------------------------------------------------------\n7820.1: basic grep -i 'how.to' 0.89(1.12+0.46)\n0.95(1.23+0.49) +6.7%\n7820.2: extended grep -i 'how.to' 0.90(1.12+0.49)\n0.92(1.19+0.46) +2.2%\n7820.3: perl grep -i 'how.to' 0.54(0.30+0.52)\n0.53(0.39+0.52) -1.9%\n7820.5: basic grep -i '^how to' 0.89(1.13+0.47)\n0.91(1.13+0.49) +2.2%\n7820.6: extended grep -i '^how to' 0.84(1.04+0.49)\n0.94(1.27+0.47) +11.9%\n7820.7: perl grep -i '^how to' 0.49(0.34+0.47)\n0.51(0.36+0.49) +4.1%\n7820.9: basic grep -i '[how] to' 1.51(2.31+0.51)\n1.55(2.38+0.51) +2.6%\n7820.10: extended grep -i '[how] to' 1.50(2.20+0.59)\n1.56(2.30+0.62) +4.0%\n7820.11: perl grep -i '[how] to' 0.67(0.50+0.52)\n0.62(0.50+0.55) -7.5%\n7820.13: basic grep -i '\\(e.t[^ ]*\\|v.ry\\) rare' 2.58(4.39+0.56)\n2.64(4.45+0.60) +2.3%\n7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 2.60(4.41+0.56)\n2.66(4.58+0.56) +2.3%\n7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.17(1.66+0.53)\n1.23(1.84+0.45) +5.1%\n7820.17: basic grep -i 'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 1.12(1.54+0.51)\n1.14(1.70+0.44) +1.8%\n7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 1.09(1.54+0.48)\n1.14(1.62+0.49) +4.6%\n7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.87(1.09+0.46)\n0.90(1.20+0.43) +3.4%\n\nand here one comparing two builds (both with NED)\n\nTest origin/pu\nHEAD\n-------------------------------------------------------------------------------------------\n7820.1: basic grep -i 'how.to' 1.00(1.24+0.55)\n0.94(1.19+0.52) -6.0%\n7820.2: extended grep -i 'how.to' 0.90(1.15+0.49)\n0.93(1.23+0.44) +3.3%\n7820.3: perl grep -i 'how.to' 0.52(0.37+0.51)\n0.59(0.34+0.53) +13.5%\n7820.5: basic grep -i '^how to' 0.89(1.16+0.48)\n0.90(1.17+0.47) +1.1%\n7820.6: extended grep -i '^how to' 0.92(1.17+0.50)\n0.92(1.20+0.45) +0.0%\n7820.7: perl grep -i '^how to' 0.45(0.33+0.42)\n0.54(0.29+0.57) +20.0%\n7820.9: basic grep -i '[how] to' 1.60(2.46+0.52)\n1.61(2.39+0.62) +0.6%\n7820.10: extended grep -i '[how] to' 1.71(2.67+0.56)\n1.57(2.41+0.54) -8.2%\n7820.11: perl grep -i '[how] to' 0.66(0.61+0.51)\n0.59(0.44+0.51) -10.6%\n7820.13: basic grep -i '\\(e.t[^ ]*\\|v.ry\\) rare' 2.69(4.49+0.66)\n2.67(4.49+0.60) -0.7%\n7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 2.67(4.49+0.64)\n2.64(4.54+0.54) -1.1%\n7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.23(1.80+0.47)\n1.25(1.89+0.46) +1.6%\n7820.17: basic grep -i 'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 1.13(1.64+0.47)\n1.14(1.64+0.48) +0.9%\n7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 1.16(1.68+0.46)\n1.20(1.60+0.60) +3.4%\n7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.90(1.16+0.48)\n0.88(1.17+0.45) -2.2%\n\nwith the only relevant line (for my code) being 7820.19 where it would\nseem it performs almost the same (eventhough just adding NED made it\ninitially worst)\n\nnote though that the fact there are 20% swings in parts of the code\nthat hasn't changed\nor that where explicitly #ifdef out of my code changes doesn't give me\nmuch confidence, but since the windows guys seem to be using NED by\ndefault, I am hoping it works better there.\n\nnote also there were no segfaults (which is what was reported\noriginally) so something\nelse must be off.\n\n> On Debian Testing with GCC\n> 9.1.0 I need two changes to suppress some compiler warnings, though.\n> Will post them as replies.\n>\n> \"make USE_NED_ALLOCATOR=1 test\" then reports these failures:\n>\n> t7816-grep-binary-pattern.sh (Wstat: 256 Tests: 145 Failed: 5)\n> Failed tests: 48, 54, 57, 60, 63\n> Non-zero exit status: 1\n\nyou got me so excited I dusted and old PC and was downloading the debian ISO\nwhen I realized the error was not a segfault[2] but my bug.\n\nsorry about it; I would swear I run the full test and it was clean,\nbut it was most likely\nwith PCRE1 or the system malloc, and definitely too late at night.\n\nThanks for your help so far, and while I know it is VERY ugly v4 at\nleast addresses that\n(and uncovered a couple more bugs), thanks also Junio for rerolling it\ninto pu so at least\nwe have a chance for it to build and run, and hopefully eventually pass.\n\nCarlo\n\n[1] https://public-inbox.org/git/20190806085014.47776-3-carenas@gmail.com/\n[2] https://dev.azure.com/git/git/_build/results?buildId=832\n"},{"id":"380110","messageId":"f6056e0c-fe7f-30b3-c2d7-bba2b44d6888@web.de","threadId":"51590","inReplyTo":"CAPUEspj4CrEk6u4+8a5UBisxWsXcwOrOPQ5s9TktA6dZx5s+uQ@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-08T07:07:40Z","receivedAt":"2019-08-08T07:07:53Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.08.19 um 04:35 schrieb Carlo Arenas:\n> On Wed, Aug 7, 2019 at 6:03 AM René Scharfe wrote:\n>>\n>> Am 07.08.19 um 11:49 schrieb Carlo Arenas:\n>>> was hoping will perform better but it seems that testing can be done\n>>> only in windows\n>>\n>> nedmalloc works on other platforms as well.\n>\n> I meant[1] it works reliably enough to be useful for performance testing.\n\nYou mentioned being concerned about performance several times and I\nwondered why each time. I'd expect no measurable difference between\nusing a custom global context and the internal one of PCRE2 -- setting\ntwo function pointers surely can't take very long, can it? But\nmeasuring is better than guessing, of course.\n\n> goes without saying that the fact that I am using a virtualbox with 2\n> CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)\n> and the test uses by default 8 threads, doesn't help,\n\nnedmalloc is supposed to run on macOS as well.\n\n> but to share my\n> pain here is the result of running p7820 with my last reroll on top of\n> pu, comparing a build of the same code without NED (this tree) with\n> one with it (HEAD)\n>\n> Test this tree\n> HEAD\n> -------------------------------------------------------------------------------------------\n> 7820.1: basic grep -i 'how.to' 0.89(1.12+0.46)\n> 0.95(1.23+0.49) +6.7%\n> 7820.2: extended grep -i 'how.to' 0.90(1.12+0.49)\n> 0.92(1.19+0.46) +2.2%\n> 7820.3: perl grep -i 'how.to' 0.54(0.30+0.52)\n> 0.53(0.39+0.52) -1.9%\n> 7820.5: basic grep -i '^how to' 0.89(1.13+0.47)\n> 0.91(1.13+0.49) +2.2%\n> 7820.6: extended grep -i '^how to' 0.84(1.04+0.49)\n> 0.94(1.27+0.47) +11.9%\n> 7820.7: perl grep -i '^how to' 0.49(0.34+0.47)\n> 0.51(0.36+0.49) +4.1%\n> 7820.9: basic grep -i '[how] to' 1.51(2.31+0.51)\n> 1.55(2.38+0.51) +2.6%\n> 7820.10: extended grep -i '[how] to' 1.50(2.20+0.59)\n> 1.56(2.30+0.62) +4.0%\n> 7820.11: perl grep -i '[how] to' 0.67(0.50+0.52)\n> 0.62(0.50+0.55) -7.5%\n> 7820.13: basic grep -i '\\(e.t[^ ]*\\|v.ry\\) rare' 2.58(4.39+0.56)\n> 2.64(4.45+0.60) +2.3%\n> 7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 2.60(4.41+0.56)\n> 2.66(4.58+0.56) +2.3%\n> 7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.17(1.66+0.53)\n> 1.23(1.84+0.45) +5.1%\n> 7820.17: basic grep -i 'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 1.12(1.54+0.51)\n> 1.14(1.70+0.44) +1.8%\n> 7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 1.09(1.54+0.48)\n> 1.14(1.62+0.49) +4.6%\n> 7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.87(1.09+0.46)\n> 0.90(1.20+0.43) +3.4%\n>\n> and here one comparing two builds (both with NED)\n>\n> Test origin/pu\n> HEAD\n> -------------------------------------------------------------------------------------------\n> 7820.1: basic grep -i 'how.to' 1.00(1.24+0.55)\n> 0.94(1.19+0.52) -6.0%\n> 7820.2: extended grep -i 'how.to' 0.90(1.15+0.49)\n> 0.93(1.23+0.44) +3.3%\n> 7820.3: perl grep -i 'how.to' 0.52(0.37+0.51)\n> 0.59(0.34+0.53) +13.5%\n> 7820.5: basic grep -i '^how to' 0.89(1.16+0.48)\n> 0.90(1.17+0.47) +1.1%\n> 7820.6: extended grep -i '^how to' 0.92(1.17+0.50)\n> 0.92(1.20+0.45) +0.0%\n> 7820.7: perl grep -i '^how to' 0.45(0.33+0.42)\n> 0.54(0.29+0.57) +20.0%\n> 7820.9: basic grep -i '[how] to' 1.60(2.46+0.52)\n> 1.61(2.39+0.62) +0.6%\n> 7820.10: extended grep -i '[how] to' 1.71(2.67+0.56)\n> 1.57(2.41+0.54) -8.2%\n> 7820.11: perl grep -i '[how] to' 0.66(0.61+0.51)\n> 0.59(0.44+0.51) -10.6%\n> 7820.13: basic grep -i '\\(e.t[^ ]*\\|v.ry\\) rare' 2.69(4.49+0.66)\n> 2.67(4.49+0.60) -0.7%\n> 7820.14: extended grep -i '(e.t[^ ]*|v.ry) rare' 2.67(4.49+0.64)\n> 2.64(4.54+0.54) -1.1%\n> 7820.15: perl grep -i '(e.t[^ ]*|v.ry) rare' 1.23(1.80+0.47)\n> 1.25(1.89+0.46) +1.6%\n> 7820.17: basic grep -i 'm\\(ú\\|u\\)lt.b\\(æ\\|y\\)te' 1.13(1.64+0.47)\n> 1.14(1.64+0.48) +0.9%\n> 7820.18: extended grep -i 'm(ú|u)lt.b(æ|y)te' 1.16(1.68+0.46)\n> 1.20(1.60+0.60) +3.4%\n> 7820.19: perl grep -i 'm(ú|u)lt.b(æ|y)te' 0.90(1.16+0.48)\n> 0.88(1.17+0.45) -2.2%\n>\n> with the only relevant line (for my code) being 7820.19 where it would\n> seem it performs almost the same (eventhough just adding NED made it\n> initially worst)\n>\n> note though that the fact there are 20% swings in parts of the code\n> that hasn't changed\n> or that where explicitly #ifdef out of my code changes doesn't give me\n> much confidence, but since the windows guys seem to be using NED by\n> default, I am hoping it works better there.\n\nThese measurement results are quite noisy, so I wouldn't trust them too\nmuch. nedmalloc being slower than the one from a recent glibc version\nis not very surprising given this statement from its home page,\nhttps://www.nedprod.com/programs/portable/nedmalloc/:\n\n \"Windows 7, Linux 3.x, FreeBSD 8, Mac OS X 10.6 all contain\n state-of-the-art allocators and no third party allocator is\n likely to significantly improve on them in real world results\"\n\nIn particular I don't think that these results justify coupling the use\nof nedmalloc to the choice of using a custom global context for PCRE2.\n\nI'd expect:\n- Without USE_NED_ALLOCATOR: xmalloc() should be used for all\n allocations, including for PCRE2. Some special exceptions use\n malloc(3) directly, but for most uses we want the consistent\n out-of-memory handling that xmalloc() brings.\n- With USE_NED_ALLOCATOR: malloc() and xmalloc() use nedmalloc\n behind the scenes and free() is similarly overridden, so all\n allocations are affected.\n- If USE_NED_ALLOCATOR performs worse than the system allocator on\n some system then it's the problem of those that turn on that flag.\n\nMakes sense?\n\nRené\n"},{"id":"380123","messageId":"CAPUEspi2vu=7uLU19c-wcW+ji63gyeO7poAwS5_7ZhTiBWSxjg@mail.gmail.com","threadId":"51590","inReplyTo":"f6056e0c-fe7f-30b3-c2d7-bba2b44d6888@web.de","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-08T12:38:11Z","receivedAt":"2019-08-08T12:38:26Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Aug 8, 2019 at 12:07 AM René Scharfe wrote:\n>\n> Am 08.08.19 um 04:35 schrieb Carlo Arenas:\n> > On Wed, Aug 7, 2019 at 6:03 AM René Scharfe wrote:\n> >>\n> >> Am 07.08.19 um 11:49 schrieb Carlo Arenas:\n> >>> was hoping will perform better but it seems that testing can be done\n> >>> only in windows\n> >>\n> >> nedmalloc works on other platforms as well.\n> >\n> > I meant[1] it works reliably enough to be useful for performance testing.\n>\n> You mentioned being concerned about performance several times and I\n> wondered why each time. I'd expect no measurable difference between\n> using a custom global context and the internal one of PCRE2 -- setting\n> two function pointers surely can't take very long, can it? But\n> measuring is better than guessing, of course.\n\nsetting the allocator is not a concern, but using it; it requires an\nextra indirect function call which is usually not very friendly to\ncaches in our speculative execution CPU world. our implementation\nalso adds the wrapper call overhead, but in this case it is just the\n\"cost of doing business\" with PCRE2.\n\ncompilers had gotten a lot better since (mainly because of C++ and the\nneed for it with virtual methods) but I would rather measure.\n\n> > goes without saying that the fact that I am using a virtualbox with 2\n> > CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)\n> > and the test uses by default 8 threads, doesn't help,\n>\n> nedmalloc is supposed to run on macOS as well.\n\nthe last version has some \"fix miscompilations in macOS\" fixes that\nmight be relevant, and the version we have in tree says it works in\nthe 32-bit version which latest macOS versions are working hard to\ndeprecate (can't even build for it anymore), eitherway trying to run\nwith a nedmalloc enabled git in macOS is not fun.\n\n> > with the only relevant line (for my code) being 7820.19 where it would\n> > seem it performs almost the same (eventhough just adding NED made it\n> > initially worst)\n> >\n> > note though that the fact there are 20% swings in parts of the code\n> > that hasn't changed\n> > or that where explicitly #ifdef out of my code changes doesn't give me\n> > much confidence, but since the windows guys seem to be using NED by\n> > default, I am hoping it works better there.\n>\n> These measurement results are quite noisy, so I wouldn't trust them too\n> much. nedmalloc being slower than the one from a recent glibc version\n> is not very surprising given this statement from its home page,\n> https://www.nedprod.com/programs/portable/nedmalloc/:\n>\n> \"Windows 7, Linux 3.x, FreeBSD 8, Mac OS X 10.6 all contain\n> state-of-the-art allocators and no third party allocator is\n> likely to significantly improve on them in real world results\"\n>\n> In particular I don't think that these results justify coupling the use\n> of nedmalloc to the choice of using a custom global context for PCRE2.\n\nneither did I either, the only reason I am holding on fully enabling\nNED with PCRE2 in my series is just because I wan't to make sure we\nhave identified the bug correctly and we are fixing it (specially\nsince I can't reproduce it, and therefore neither debug it)\n\nsorry for not making that clear enough, and as I said before, if we\nkeep seeing segfaults even after v4 then we will have to do that or I\nmight need to do a quick run to the nearest microsoft store hoping for\na distracted rep, instead.\n\n> I'd expect:\n> - Without USE_NED_ALLOCATOR: xmalloc() should be used for all\n> allocations, including for PCRE2. Some special exceptions use\n> malloc(3) directly, but for most uses we want the consistent\n> out-of-memory handling that xmalloc() brings.\n\nthat is already in v4 and would expect to carry it forward. this is\nalso what I had in mind when I said we will need some fixes on top of\nDscho version if we give up with these.\n\n> - With USE_NED_ALLOCATOR: malloc() and xmalloc() use nedmalloc\n> behind the scenes and free() is similarly overridden, so all\n> allocations are affected.\n> - If USE_NED_ALLOCATOR performs worse than the system allocator on\n> some system then it's the problem of those that turn on that flag.\n>\n> Makes sense?\n\ncompletely, but note also that Dscho version would make the\nperformance impacts of using a custom allocator (if any) affect\neveryone using PCRE2.\n\nCarlo\n"},{"id":"380128","messageId":"nycvar.QRO.7.76.6.1908081552440.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"20190806085014.47776-2-carenas@gmail.com","subject":"Re: [PATCH 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-08T13:54:58Z","receivedAt":"2019-08-08T13:55:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n\n> 63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\n> to override the system alocator, and so it is incompatible with\n> USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n>\n> Make the minimum change possible to ensure this combination is supported\n>\n> [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> Makefile | 2 +-\n> grep.c | 10 ++++++++++\n> 2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index bd246f2989..4b384f3759 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF\n> endif\n>\n> ifdef USE_NED_ALLOCATOR\n> -\tCOMPAT_CFLAGS += -Icompat/nedmalloc\n> +\tCOMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n\nThis pretends that all custom allocators are selected at build time,\nsomething I tried to stress in my commit message as not true. You can\npre-load not only nedmalloc, but also jemalloc and unless I am mistaken\nalso tcmalloc. And mi-malloc.\n\nSo the premise of this patch, that you can tell at compile time that a\ndifferent allocator than the system one will be in use is simply\nincorrect.\n\nCiao,\nDscho\n\n> \tCOMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n> \tOVERRIDE_STRDUP = YesPlease\n> endif\n> diff --git a/grep.c b/grep.c\n> index cd952ef5d3..0154998695 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)\n> * Initialize one instance of grep_opt and copy the\n> * default values from the template we read the configuration\n> * information in an earlier call to git_config(grep_config).\n> + *\n> + * If using PCRE make sure that the library is configured\n> + * to use the right allocator (ex: NED)\n> */\n> void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n> {\n> \tstruct grep_opt *def = &grep_defaults;\n> \tint i;\n>\n> +#ifdef USE_NED_ALLOCATOR\n> +#ifdef USE_LIBPCRE1\n> +\tpcre_malloc = malloc;\n> +\tpcre_free = free;\n> +#endif\n> +#endif\n> +\n> \tmemset(opt, 0, sizeof(*opt));\n> \topt->repo = repo;\n> \topt->prefix = prefix;\n> --\n> 2.23.0.rc1\n>\n>\n"},{"id":"380129","messageId":"nycvar.QRO.7.76.6.1908081556020.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"20190806085014.47776-3-carenas@gmail.com","subject":"Re: [PATCH 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-08T13:56:54Z","receivedAt":"2019-08-08T13:57:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n\n> Most of the code stolen from[1] to easy on comparison and including\n> the deficiency of setting the global context even for patterns that\n> won't need it.\n>\n> Ideally, the call from grep_init could be moved to a place where it\n> could be set without needing a lock and at least with this approach\n> we have a place to clear it (which is obviously missing more callers,\n> but at least shows how it will look for the grep subcommand)\n>\n> I had also dropped most other users of the global context in a failed\n> attempt to make the change smaller, but also to keep the current\n> behaviour so that we could see the effect of enabling NED for PCRE2\n> more clearly.\n>\n> Sadly, that will likely require a Windows box, as NED (at least our\n> version) is horribly broken in macOS (maybe it wasn't 64 bit clean)\n> and in Linux builds, but I can't reproduce your crasher and it is\n> most likely slower than the system malloc.\n>\n> [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com/\n>\n> Suggested-by: Johannes Schindelin \n\nActually not so much suggested by me, as your patch still causes\ncrashes (mine didn't):\n\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=13935&view=ms.vss-test-web.build-test-results-tab\n\nCiao,\nDscho\n\n> ---\n> builtin/grep.c | 1 +\n> grep.c | 31 +++++++++++++++++++++++++++++--\n> grep.h | 1 +\n> 3 files changed, 31 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 560051784e..e49c20df60 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n> \t\trun_pager(&opt, prefix);\n> \tclear_pathspec(&pathspec);\n> \tfree_grep_patterns(&opt);\n> +\tgrep_destroy();\n> \treturn !hit;\n> }\n> diff --git a/grep.c b/grep.c\n> index 0154998695..e748a6d68c 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -16,6 +16,20 @@ static int grep_source_is_binary(struct grep_source *gs,\n>\n> static struct grep_opt grep_defaults;\n>\n> +#ifdef USE_LIBPCRE2\n> +static pcre2_general_context *pcre2_global_context;\n> +\n> +static void *pcre2_malloc(PCRE2_SIZE size, void *memory_data)\n> +{\n> +\treturn malloc(size);\n> +}\n> +\n> +static void pcre2_free(void *pointer, void *memory_data)\n> +{\n> +\treturn free(pointer);\n> +}\n> +#endif\n> +\n> static const char *color_grep_slots[] = {\n> \t[GREP_COLOR_CONTEXT]\t = \"context\",\n> \t[GREP_COLOR_FILENAME]\t = \"filename\",\n> @@ -153,6 +167,7 @@ int grep_config(const char *var, const char *value, void *cb)\n> *\n> * If using PCRE make sure that the library is configured\n> * to use the right allocator (ex: NED)\n> + * if any object is created it should be cleaned up in grep_destroy()\n> */\n> void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n> {\n> @@ -164,6 +179,10 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n> \tpcre_malloc = malloc;\n> \tpcre_free = free;\n> #endif\n> +#ifdef USE_LIBPCRE2\n> +\tpcre2_global_context = pcre2_general_context_create(pcre2_malloc,\n> +\t\t\t\t\t\t\tpcre2_free, NULL);\n> +#endif\n> #endif\n>\n> \tmemset(opt, 0, sizeof(*opt));\n> @@ -188,6 +207,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n> \t\tcolor_set(opt->colors[i], def->colors[i]);\n> }\n>\n> +void grep_destroy(void)\n> +{\n> +#ifdef USE_LIBPCRE2\n> +\tpcre2_general_context_free(pcre2_global_context);\n> +#endif\n> +}\n> +\n> static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n> {\n> \t/*\n> @@ -509,7 +535,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>\n> \tif (opt->ignore_case) {\n> \t\tif (has_non_ascii(p->pattern)) {\n> -\t\t\tcharacter_tables = pcre2_maketables(NULL);\n> +\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n> \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n> \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n> \t\t}\n> @@ -560,7 +586,8 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \t\t\treturn;\n> \t\t}\n>\n> -\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);\n> +\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024,\n> +\t\t\t\t\tpcre2_global_context);\n> \t\tif (!p->pcre2_jit_stack)\n> \t\t\tdie(\"Couldn't allocate PCRE2 JIT stack\");\n> \t\tp->pcre2_match_context = pcre2_match_context_create(NULL);\n> diff --git a/grep.h b/grep.h\n> index 1875880f37..526c2db9ef 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -189,6 +189,7 @@ struct grep_opt {\n> void init_grep_defaults(struct repository *);\n> int grep_config(const char *var, const char *value, void *);\n> void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n> +void grep_destroy(void);\n> void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n>\n> void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n> --\n> 2.23.0.rc1\n>\n>\n"},{"id":"380139","messageId":"7f42007f-911b-c570-17f6-1c6af0429586@web.de","threadId":"51590","inReplyTo":"CAPUEspi2vu=7uLU19c-wcW+ji63gyeO7poAwS5_7ZhTiBWSxjg@mail.gmail.com","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-08T14:29:33Z","receivedAt":"2019-08-08T14:29:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.08.19 um 14:38 schrieb Carlo Arenas:\n> On Thu, Aug 8, 2019 at 12:07 AM René Scharfe wrote:\n>>\n>> Am 08.08.19 um 04:35 schrieb Carlo Arenas:\n>>> On Wed, Aug 7, 2019 at 6:03 AM René Scharfe wrote:\n>>>>\n>>>> Am 07.08.19 um 11:49 schrieb Carlo Arenas:\n>>>>> was hoping will perform better but it seems that testing can be done\n>>>>> only in windows\n>>>>\n>>>> nedmalloc works on other platforms as well.\n>>>\n>>> I meant[1] it works reliably enough to be useful for performance testing.\n>>\n>> You mentioned being concerned about performance several times and I\n>> wondered why each time. I'd expect no measurable difference between\n>> using a custom global context and the internal one of PCRE2 -- setting\n>> two function pointers surely can't take very long, can it? But\n>> measuring is better than guessing, of course.\n>\n> setting the allocator is not a concern, but using it; it requires an\n> extra indirect function call which is usually not very friendly to\n> caches in our speculative execution CPU world. our implementation\n> also adds the wrapper call overhead, but in this case it is just the\n> \"cost of doing business\" with PCRE2.\n\nPCRE2 needs to allocate memory once per program run for the character\ntable and for each pattern compilation. These are both rare events\ncompared to matching patterns against lines, and I suspect that\ncompilation in particular has much more other work to do, even more so\nwith JIT enabled; I'd expect function call indirection to not make much\nof a difference.\n\npcre2_compile() always calls the allocation function through a\nfunction pointer in a context struct, by the way (see line 9695 in\nhttps://vcs.pcre.org/pcre2/code/trunk/src/pcre2_compile.c?view=markup\nor search for \"malloc\" in that file).\n\n>>> goes without saying that the fact that I am using a virtualbox with 2\n>>> CPUs running Debian 10 on top of macOS (a macbook pro with 4 cores)\n>>> and the test uses by default 8 threads, doesn't help,\n>>\n>> nedmalloc is supposed to run on macOS as well.\n>\n> the last version has some \"fix miscompilations in macOS\" fixes that\n> might be relevant, and the version we have in tree says it works in\n> the 32-bit version which latest macOS versions are working hard to\n> deprecate (can't even build for it anymore), eitherway trying to run\n> with a nedmalloc enabled git in macOS is not fun.\n\nImporting the latest version of nedmalloc might make sense in general.\nThe last commit in git://github.com/ned14/nedmalloc.git was done five\nyears ago; is it finished? A diffstat with -b looks like this:\n\n malloc.c.h | 1193 ++++++++++++++++++++++++++++-------------\n nedmalloc.c | 1720 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n nedmalloc.h | 1580 ++++++++++++++++++++++++++++++++++++++++++++++++++++---\n 3 files changed, 3840 insertions(+), 653 deletions(-)\n\nAny nedmalloc fans interested in bringing the goodies hidden in there\nto Git (presumably while retaining our local fixes)?\n\n>> In particular I don't think that these results justify coupling the use\n>> of nedmalloc to the choice of using a custom global context for PCRE2.\n>\n> neither did I either, the only reason I am holding on fully enabling\n> NED with PCRE2 in my series is just because I wan't to make sure we\n> have identified the bug correctly and we are fixing it (specially\n> since I can't reproduce it, and therefore neither debug it)\n\nThat's a good reason against #ifdefs in general. Sometimes they are\nunavoidable, but they can make maintenance a lot harder.\n\n> sorry for not making that clear enough, and as I said before, if we\n> keep seeing segfaults even after v4 then we will have to do that or I\n> might need to do a quick run to the nearest microsoft store hoping for\n> a distracted rep, instead.\n\nAsking to buy a license for Windows Vista might cause quite a bit of a\ndistraction in there -- Microsoft's support for that version ended two\nyears ago. :) It still seems to be popular enough to be supported by\nGit for Windows, however.\n\n(You could buy Windows 10 and probably get a downgrade right, but\nfinding legit install media for Vista might be challenging.)\n\nBut I'd say do the easy thing: Custom global context for all.\n\n>> I'd expect:\n>> - Without USE_NED_ALLOCATOR: xmalloc() should be used for all\n>> allocations, including for PCRE2. Some special exceptions use\n>> malloc(3) directly, but for most uses we want the consistent\n>> out-of-memory handling that xmalloc() brings.\n>\n> that is already in v4 and would expect to carry it forward. this is\n> also what I had in mind when I said we will need some fixes on top of\n> Dscho version if we give up with these.\n>\n>> - With USE_NED_ALLOCATOR: malloc() and xmalloc() use nedmalloc\n>> behind the scenes and free() is similarly overridden, so all\n>> allocations are affected.\n>> - If USE_NED_ALLOCATOR performs worse than the system allocator on\n>> some system then it's the problem of those that turn on that flag.\n>>\n>> Makes sense?\n>\n> completely, but note also that Dscho version would make the\n> performance impacts of using a custom allocator (if any) affect\n> everyone using PCRE2.\n\nSounds fine to me, if the performance numbers don't take too much of\na hit. I'd be surprised if the needle moved at all (ignoring noise).\n\nRené\n"},{"id":"380140","messageId":"CAPUEspg+04ifdjR0KyL=+MDRMUEWNBJ__bsY0Dc4mPtxbGQ-vA@mail.gmail.com","threadId":"51590","inReplyTo":"nycvar.QRO.7.76.6.1908081556020.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-08T14:32:08Z","receivedAt":"2019-08-08T14:32:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Aug 8, 2019 at 6:57 AM Johannes Schindelin\n wrote:\n> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n> > Suggested-by: Johannes Schindelin \n>\n> Actually not so much suggested by me, as your patch still causes\n> crashes (mine didn't):\n\nthe \"equivalent\" version in v4[1], that is still not on pu shouldn't,\nif my hunch is correct\n\nit also shouldn't crash if the test wouldn't be doing LC_ALL=\"C\" but\nwe have yet a couple of extra bugs related to that, including one[2] I\nCC you on which would be nice to get validated as well.\n\nCarlo\n\n[1] https://public-inbox.org/git/20190807213945.10464-3-carenas@gmail.com/\n[2] https://public-inbox.org/git/20190807095322.8988-1-carenas@gmail.com/\n"},{"id":"380141","messageId":"CAPUEsphLZ+x1hL+=8P+bh34mR6pUcXB5Qd2htj4AzUjjrKC=nQ@mail.gmail.com","threadId":"51590","inReplyTo":"nycvar.QRO.7.76.6.1908081552440.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-08T15:19:52Z","receivedAt":"2019-08-08T15:20:05Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Aug 8, 2019 at 6:55 AM Johannes Schindelin\n wrote:\n\n> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n>\n> > 63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\n> > to override the system allocator, and so it is incompatible with\n> > USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n> >\n> > Make the minimum change possible to ensure this combination is supported\n> >\n> > [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n> >\n> > Signed-off-by: Carlo Marcelo Arenas Belón \n> > ---\n> > Makefile | 2 +-\n> > grep.c | 10 ++++++++++\n> > 2 files changed, 11 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/Makefile b/Makefile\n> > index bd246f2989..4b384f3759 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF\n> > endif\n> >\n> > ifdef USE_NED_ALLOCATOR\n> > - COMPAT_CFLAGS += -Icompat/nedmalloc\n> > + COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n>\n> This pretends that all custom allocators are selected at build time,\n> something I tried to stress in my commit message as not true. You can\n> pre-load not only nedmalloc, but also jemalloc and unless I am mistaken\n> also tcmalloc. And mi-malloc.\n\nthe patch only fixes the case when NED was chosen at build time, which\nbefore this patch meant that the system allocator was used by PCRE1\nand NED by git.\n\nThat combination happened to work unless any pointer crossed over but\nneglected any benefit of using NED instead of the system allocator\nwith PCRE1.\n\nthe PCRE1 API was therefore safer to use, PCRE2 is not and that will\nbe addressed[1] in a future version by making sure this \"layering\nviolation\" is covered\n\nruntime custom allocator changes are not affected and will keep\nworking as usual, which (at least in Linux when using LD_PRELOAD which\nis probably what you are refer to by \"preload\") means that the dynamic\nlinker will replace all calls to malloc/free with the ones you\nprovided at exec time and before it starts the binary (including\nwhichever libraries it loaded) and therefore will make both git and\nPCRE2 to use the same allocator.\n\nnedmalloc seems (at least in Linux) to be smart enough to cope with\npointers that were not allocated by itself and doesn't crash so\npresume there is something else in Windows (maybe the dynamic linker,\nif there is one) that might be also part of the reason for this\nsegfault.\n\nof course, the trigger of this is my patch, so I'll take the blame for\nthis bug regardless and I am looking forward to a fix\n\n> So the premise of this patch, that you can tell at compile time that a\n> different allocator than the system one will be in use is simply\n> incorrect.\n\nin the context that the only available (and integrated with our build\nsystem) allocator is NED at compile time, I think it is correct.\nyou are correct though that if another allocator will be added as a\ncompile time option, we will need to do something similar as well.\n\nCarlo\n\n[1] https://bugs.exim.org/show_bug.cgi?id=2429\n"},{"id":"380166","messageId":"nycvar.QRO.7.76.6.1908082213400.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"7f42007f-911b-c570-17f6-1c6af0429586@web.de","subject":"Re: [RFC PATCH v3 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-08T20:18:05Z","receivedAt":"2019-08-08T20:18:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Thu, 8 Aug 2019, René Scharfe wrote:\n\n> Importing the latest version of nedmalloc might make sense in general.\n> The last commit in git://github.com/ned14/nedmalloc.git was done five\n> years ago; is it finished? A diffstat with -b looks like this:\n>\n> malloc.c.h | 1193 ++++++++++++++++++++++++++++-------------\n> nedmalloc.c | 1720 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n> nedmalloc.h | 1580 ++++++++++++++++++++++++++++++++++++++++++++++++++++---\n> 3 files changed, 3840 insertions(+), 653 deletions(-)\n>\n> Any nedmalloc fans interested in bringing the goodies hidden in there\n> to Git (presumably while retaining our local fixes)?\n\nI had looked into this already over two years ago, and had to stop after\ninvestigating a performance regression for two weeks and not getting\nanywhere.\n\nAlso, nedmalloc fell unmaintained, so I don't necessarily think that it\nwould be a good idea to spend a lot of time on it.\n\nIn the meantime, there is a much more viable contender: mi-malloc.\nPreliminary tests suggest that its performance on Windows is at least as\ngood as nedmalloc's, and Windows was the use case for which we\nintegrated nedmalloc into Git's compat/ in the first place.\n\nI have tentative patches to integrate it into Git for Windows, and\nbasically got side-tracked with other things. Expect to see something\nregarding mi-malloc from me in September.\n\nCiao,\nDscho\n"},{"id":"380167","messageId":"nycvar.QRO.7.76.6.1908082219160.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"20190806163658.66932-1-carenas@gmail.com","subject":"Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-08T20:21:16Z","receivedAt":"2019-08-08T20:21:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n\n> Eitherway, since I am unable to replicate the original bug or take\n> performance numbers in a representative environment without Windows\n> this is only published as an RFC, eventhough it has been tested and\n> considered mostly complete.\n\nWell, this is disappointing.\n\nI worked several weeks on getting Azure Pipelines support in shape, so\nthat you can now open PRs against:\n\n- https://github.com/git/git\n\n- https://github.com/gitgitgadget/git\n\n- https://github.com/git-for-windows/git\n\nto get Windows/macOS/Linux testing for free.\n\nSo I guess you'd like fries with that, extra large ones, with extra\npepperoni seasoning?\n\nCiao,\nDscho\n"},{"id":"380189","messageId":"20190809030210.18353-1-carenas@gmail.com","threadId":"51590","inReplyTo":"20190807213945.10464-1-carenas@gmail.com","subject":"[RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-09T03:02:07Z","receivedAt":"2019-08-09T03:02:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This series is a candidate reroll for cb/pcre2-chartables-leakfix, that\nhopefully addresses the root cause of the problem reported by Dscho in\nWindows, where the PCRE2 library wasn't aware of the custom allocator and\nwas returning a pointer created with the system malloc but passing it to\nNED's free, resulting in a segfault.\n\nThe most likely reason why it was triggered by the original leak fix is\nthe layering violation reported by René and that is likely exclusive to PCRE2\n(hence why it hasn't been reported with PCRE1). Additional work might be\navailable in a future release of PCRE2 to address that as detailed in an\nupstream bug[1] report.\n\nChanges since v4 (only in patch 2):\n\n* git log change reverted, still not sure where it will fit better and worst\n case will leak a few bytes when -P is used.\n since the users of this API are doing it indirectly it might be problematic\n long term though, but luckily since it is most of the tine a NOOP and can\n be called multiple times might be ok to do it unconditionally\n* slightly better looking code\n\nChanges since v3 (mostly in patch 2):\n\n* git log also calls the \"destructor\" for grep API\n* no more \"bug\" being triggered by make test, sorry René\n* hopefully no more crashes in windows (I was expecting at most a BUG)\n\nFuture work (other than the needed refactoring explained in the\nsecond patch) and adjacent bugs, includes:\n\n* tracking more possible users of the grep API that might need to call\n grep_destroy()\n* completely moving PCRE2 to use NED (as is done with PCRE1 and was\n proposed on the original patch[2] this is based on\n* build on top of the new API so that other work could be shared\n (for example the chartables that started this whole mess)\n\nor (hopefully not)\n\n* ignore the original leak (maybe with an UNLEAK) as René suggested [3]\n* discard this work and just use Dscho's fix (with some improvements,\n like using xmalloc)\n\n[1] https://bugs.exim.org/show_bug.cgi?id=2429\n[2] https://public-inbox.org/git/3397e6797f872aedd18c6d795f4976e1c579514b.1565005867.git.gitgitgadget@gmail.com/\n[3] https://public-inbox.org/git/7ec60d57-9940-35f2-f7b5-c87d4dc7cdd5@web.de/\n\nCarlo Marcelo Arenas Belón (3):\n grep: make PCRE1 aware of custom allocator\n grep: make PCRE2 aware of custom allocator\n grep: avoid leak of chartables in PCRE2\n\n Makefile | 2 +-\n builtin/grep.c | 1 +\n grep.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++---\n grep.h | 2 ++\n 4 files changed, 72 insertions(+), 4 deletions(-)\n\nbase-commit: 51cf315870bbb7254ddf06c84fe03b41bc48eebd\n-- \n2.23.0.rc1\n"},{"id":"380190","messageId":"20190809030210.18353-2-carenas@gmail.com","threadId":"51590","inReplyTo":"20190809030210.18353-1-carenas@gmail.com","subject":"[RFC PATCH v5 1/3] grep: make PCRE1 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-09T03:02:08Z","receivedAt":"2019-08-09T03:02:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"63e7e9d8b6 (\"git-grep: Learn PCRE\", 2011-05-09) didn't include a way\nto override the system alocator, and so it is incompatible with\nUSE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)\n\nMake the minimum change possible to ensure this combination is supported\nby extending grep_init to set the PCRE1 specific functions to the NED\nversions (using the aliased names though) and therefore making sure all\nalocations are done inside PCRE1 with the same allocator than the rest\nof git.\n\nThis change might have performance impact (hopefully for the best) and\nso will be good idea to test it in a platform where NED might have a\npositive impact (ex: Windows 7)\n\n[1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Junio C Hamano \n---\n Makefile | 2 +-\n grep.c | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex bd246f2989..4b384f3759 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF\n endif\n \n ifdef USE_NED_ALLOCATOR\n-\tCOMPAT_CFLAGS += -Icompat/nedmalloc\n+\tCOMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n \tCOMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n \tOVERRIDE_STRDUP = YesPlease\n endif\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..0154998695 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -150,12 +150,22 @@ int grep_config(const char *var, const char *value, void *cb)\n * Initialize one instance of grep_opt and copy the\n * default values from the template we read the configuration\n * information in an earlier call to git_config(grep_config).\n+ *\n+ * If using PCRE make sure that the library is configured\n+ * to use the right allocator (ex: NED)\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n \n+#ifdef USE_NED_ALLOCATOR\n+#ifdef USE_LIBPCRE1\n+\tpcre_malloc = malloc;\n+\tpcre_free = free;\n+#endif\n+#endif\n+\n \tmemset(opt, 0, sizeof(*opt));\n \topt->repo = repo;\n \topt->prefix = prefix;\n-- \n2.23.0.rc1\n\n"},{"id":"380191","messageId":"20190809030210.18353-4-carenas@gmail.com","threadId":"51590","inReplyTo":"20190809030210.18353-1-carenas@gmail.com","subject":"[RFC PATCH v5 3/3] grep: avoid leak of chartables in PCRE2","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-09T03:02:10Z","receivedAt":"2019-08-09T03:02:24Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) introduced\na small memory leak visible with valgrind in t7813.\n\nComplete the creation of a PCRE2 specific variable that was missing from\nthe original change and free the generated table just like it is done\nfor PCRE1.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Junio C Hamano \n---\n grep.c | 7 ++++---\n grep.h | 1 +\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 8e0b838db0..146891e2bf 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -543,7 +543,6 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tPCRE2_UCHAR errbuf[256];\n \tPCRE2_SIZE erroffset;\n \tint options = PCRE2_MULTILINE;\n-\tconst uint8_t *character_tables = NULL;\n \tint jitret;\n \tint patinforet;\n \tsize_t jitsizearg;\n@@ -563,9 +562,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\tif (!pcre2_global_context)\n \t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n #endif\n-\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n+\t\t\tp->pcre2_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n-\t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n+\t\t\tpcre2_set_character_tables(p->pcre2_compile_context,\n+\t\t\t\t\t\t\tp->pcre2_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n@@ -669,6 +669,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tfree((void *)p->pcre2_tables);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 526c2db9ef..533165c21a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -96,6 +96,7 @@ struct grep_pat {\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n+\tconst uint8_t *pcre2_tables;\n \tuint32_t pcre2_jit_on;\n \tkwset_t kws;\n \tunsigned fixed:1;\n-- \n2.23.0.rc1\n\n"},{"id":"380192","messageId":"20190809030210.18353-3-carenas@gmail.com","threadId":"51590","inReplyTo":"20190809030210.18353-1-carenas@gmail.com","subject":"[RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-09T03:02:09Z","receivedAt":"2019-08-09T03:02:24Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\na way to override the system allocator, and so it is incompatible with\nUSE_NED_ALLOCATOR. The problem was made visible when an attempt to\navoid a leak in a data structure that is created by the library was\npassed to NED's free for disposal triggering a segfault in Windows.\n\nPCRE2 requires the use of a general context to override the allocator\nand therefore, there is a lot more code needed than in PCRE1, including\na couple of wrapper functions.\n\nExtend the grep API with a \"destructor\" that could be called to cleanup\nany objects that were created and used globally.\n\nUpdate builtin/grep to use that new API, but any other future\nusers should make sure to have matching grep_init/grep_destroy calls\nif they are using the pattern matching functionality (currently only\nrelevant when using both NED and PCRE2)\n\nMove the logic to decide if a general context will be needed to an\nearlier phase so it will only be done once per pattern (instead of\nat least once per worker thread) avoiding then the need for locking.\n\nThis change does the minimum change required to hopefully solve the\ncrash, with the rest of the users of it added later.\n\nHelped-by: René Scharfe \nReported-by: Johannes Schindelin \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n builtin/grep.c | 1 +\n grep.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++-\n grep.h | 1 +\n 3 files changed, 57 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 560051784e..e49c20df60 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1145,5 +1145,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\trun_pager(&opt, prefix);\n \tclear_pathspec(&pathspec);\n \tfree_grep_patterns(&opt);\n+\tgrep_destroy();\n \treturn !hit;\n }\ndiff --git a/grep.c b/grep.c\nindex 0154998695..8e0b838db0 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -16,6 +16,44 @@ static int grep_source_is_binary(struct grep_source *gs,\n \n static struct grep_opt grep_defaults;\n \n+#ifdef USE_LIBPCRE2\n+static pcre2_general_context *pcre2_global_context;\n+\n+#ifdef USE_NED_ALLOCATOR\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn xmalloc(size); /* will use nedalloc underneath */\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn free(pointer);\n+}\n+\n+/*\n+ * BUG: this is technically not needed if we will do UTF matching\n+ * but UTF locale detection is currently broken\n+ * TODO: has_non_ascii() doesn't support NUL in pattern\n+ */\n+void setup_pcre2_as_needed(struct grep_opt *opt, const char *pat)\n+{\n+\tif (!pcre2_global_context && opt->ignore_case &&\n+\t\thas_non_ascii(pat))\n+\t\tpcre2_global_context = pcre2_general_context_create(\n+\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n+}\n+\n+static void cleanup_pcre2_as_needed(void)\n+{\n+\tpcre2_general_context_free(pcre2_global_context);\n+}\n+\n+#else\n+#define setup_pcre2_as_needed(opt, pat)\n+#define cleanup_pcre2_as_needed()\n+#endif\n+#endif\n+\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n \t[GREP_COLOR_FILENAME]\t = \"filename\",\n@@ -153,6 +191,7 @@ int grep_config(const char *var, const char *value, void *cb)\n *\n * If using PCRE make sure that the library is configured\n * to use the right allocator (ex: NED)\n+ * if any object is created it should be cleaned up in grep_destroy()\n */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n@@ -188,6 +227,11 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n+void grep_destroy(void)\n+{\n+\tcleanup_pcre2_as_needed();\n+}\n+\n static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)\n {\n \t/*\n@@ -326,6 +370,7 @@ void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen,\n \t\t const char *origin, int no, enum grep_pat_token t)\n {\n \tstruct grep_pat *p = create_grep_pat(pat, patlen, origin, no, t, 0);\n+\tsetup_pcre2_as_needed(opt, pat);\n \tdo_append_grep_pat(&opt->pattern_tail, p);\n }\n \n@@ -507,9 +552,18 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tp->pcre2_compile_context = NULL;\n \n+\t/*\n+\t * pcre2_global_context is initialized in append_grep_pat()\n+\t * with logic from setup_pcre2_as_needed() that mimics what\n+\t * is used here and with the BUG() to protect from mismatches\n+\t */\n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n+#ifdef USE_NED_ALLOCATOR\n+\t\t\tif (!pcre2_global_context)\n+\t\t\t\tBUG(\"pcre2_global_context uninitialized\");\n+#endif\n+\t\t\tcharacter_tables = pcre2_maketables(pcre2_global_context);\n \t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n \t\t}\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..526c2db9ef 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -189,6 +189,7 @@ struct grep_opt {\n void init_grep_defaults(struct repository *);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n+void grep_destroy(void);\n void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt);\n \n void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);\n-- \n2.23.0.rc1\n\n"},{"id":"380199","messageId":"CAPUEspibDcNfGk4=6Mvi2BHATGSxRPPph3F=3pLm_bLMBkBTNQ@mail.gmail.com","threadId":"51590","inReplyTo":"nycvar.QRO.7.76.6.1908082219160.46@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-09T06:52:12Z","receivedAt":"2019-08-09T06:52:26Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Aug 8, 2019 at 1:21 PM Johannes Schindelin\n wrote:\n> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n> > Eitherway, since I am unable to replicate the original bug or take\n> > performance numbers in a representative environment without Windows\n> > this is only published as an RFC, eventhough it has been tested and\n> > considered mostly complete.\n>\n> Well, this is disappointing.\n\nApologies\n\n> I worked several weeks on getting Azure Pipelines support in shape, so\n> that you can now open PRs against:\n\nThanks, I owe you a drink\n\n https://dev.azure.com/git/git/_build/results?buildId=862\n\nCarlo\n\nPS. I might had broken something here as I can't see the test results\nthat failed\nhttps://dev.azure.com/git/git/_build/results?buildId=857&view=ms.vss-test-web.build-test-results-tab\n"},{"id":"380204","messageId":"CAPUEspiK7MTZPMktbU=_C_GPOH9vQiBmVUZp7GuR97RZS3onRQ@mail.gmail.com","threadId":"51590","inReplyTo":"20190809030210.18353-1-carenas@gmail.com","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-09T11:24:56Z","receivedAt":"2019-08-09T11:25:10Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"disregard this version, it still broken (and wouldn't even build on\nsome cases), the reasons why are still unclear though but at least it\nmight\nseem from the last known run in windows that segfaults were prevented\nat last and something was still off enough to trigger a BUG (shouldn't\nbe a concern with V6 or later that do PCRE2 migration to NED fully, as\nagreed)\n\nThanks to Dscho's github integration to Azure pipelines got finally a\nRC[1] for V6 that at least passes all tests and will need to get in\nshape for submission.\n\nas René pointed out in three my performance concerns might be a red\nherring as well, but would be nice if someone with windows would\ndefinitely get some numbers so we can't be sure there were truly no\nregressions\n\nCarlo\n\n[1] https://github.com/carenas/git/tree/pcre2-chartables-leakfix\n"},{"id":"380225","messageId":"d239326e-11c3-5875-13a8-f4123baea6eb@web.de","threadId":"51590","inReplyTo":"CAPUEspiK7MTZPMktbU=_C_GPOH9vQiBmVUZp7GuR97RZS3onRQ@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-09T17:01:02Z","receivedAt":"2019-08-09T17:01:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.08.19 um 13:24 schrieb Carlo Arenas:\n> disregard this version, it still broken (and wouldn't even build on\n> some cases), the reasons why are still unclear though but at least it\n> might\n> seem from the last known run in windows that segfaults were prevented\n> at last and something was still off enough to trigger a BUG (shouldn't\n> be a concern with V6 or later that do PCRE2 migration to NED fully, as\n> agreed)\n\nSo how about starting stupidly simple? You can test it everywhere, it\njust needs libpcre2. It works without that library as well (tested with\n\"make USE_LIBPCRE= USE_LIBPCRE2= test\"), but doesn't allocate anything\nin that case, of course. The character tables leak fix should be safe\non top. If you detect performance issues then we can address them in\nadditional patches.\n\n-- >8 --\nSubject: [PATCH] grep: use xmalloc() for all PCRE2 allocations\n\nBuild a PCRE2 global custom context when compiling a pattern and use it\nto tell the library to use xmalloc() for allocations. This provides\nconsistent out-of-memory handling and makes sure it uses a custom\nallocator, e.g. with USE_NED_ALLOCATOR.\n\nSigned-off-by: René Scharfe \n---\nThe function names are ridiculously long, I tried to stay within 80\ncharacters per line but gave up in the end and just kept going without\nline breaks. Fits the \"stupidly simple\" approach..\n\n grep.c | 23 +++++++++++++++++------\n grep.h | 2 ++\n 2 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex cd952ef5d3..44f4e38657 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -482,6 +482,16 @@ static void free_pcre1_regexp(struct grep_pat *p)\n #endif /* !USE_LIBPCRE1 */\n\n #ifdef USE_LIBPCRE2\n+static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n+{\n+\treturn xmalloc(size);\n+}\n+\n+static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n+{\n+\tfree(pointer);\n+}\n+\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n {\n \tint error;\n@@ -495,12 +505,12 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n\n \tassert(opt->pcre2);\n\n-\tp->pcre2_compile_context = NULL;\n+\tp->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);\n+\tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n\n \tif (opt->ignore_case) {\n \t\tif (has_non_ascii(p->pattern)) {\n-\t\t\tcharacter_tables = pcre2_maketables(NULL);\n-\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n+\t\t\tcharacter_tables = pcre2_maketables(p->pcre2_general_context);\n \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n@@ -513,7 +523,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\t\t\t p->pcre2_compile_context);\n\n \tif (p->pcre2_pattern) {\n-\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n+\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, p->pcre2_general_context);\n \t\tif (!p->pcre2_match_data)\n \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n \t} else {\n@@ -550,10 +560,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t\treturn;\n \t\t}\n\n-\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);\n+\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, p->pcre2_general_context);\n \t\tif (!p->pcre2_jit_stack)\n \t\t\tdie(\"Couldn't allocate PCRE2 JIT stack\");\n-\t\tp->pcre2_match_context = pcre2_match_context_create(NULL);\n+\t\tp->pcre2_match_context = pcre2_match_context_create(p->pcre2_general_context);\n \t\tif (!p->pcre2_match_context)\n \t\t\tdie(\"Couldn't allocate PCRE2 match context\");\n \t\tpcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack);\n@@ -605,6 +615,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n \tpcre2_match_data_free(p->pcre2_match_data);\n \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n \tpcre2_match_context_free(p->pcre2_match_context);\n+\tpcre2_general_context_free(p->pcre2_general_context);\n }\n #else /* !USE_LIBPCRE2 */\n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..73b8b87a3a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -28,6 +28,7 @@ typedef int pcre_jit_stack;\n #else\n typedef int pcre2_code;\n typedef int pcre2_match_data;\n+typedef int pcre2_general_context;\n typedef int pcre2_compile_context;\n typedef int pcre2_match_context;\n typedef int pcre2_jit_stack;\n@@ -93,6 +94,7 @@ struct grep_pat {\n \tint pcre1_jit_on;\n \tpcre2_code *pcre2_pattern;\n \tpcre2_match_data *pcre2_match_data;\n+\tpcre2_general_context *pcre2_general_context;\n \tpcre2_compile_context *pcre2_compile_context;\n \tpcre2_match_context *pcre2_match_context;\n \tpcre2_jit_stack *pcre2_jit_stack;\n--\n2.22.0\n"},{"id":"380230","messageId":"xmqqy302cju8.fsf@gitster-ct.c.googlers.com","threadId":"51590","inReplyTo":"d239326e-11c3-5875-13a8-f4123baea6eb@web.de","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-09T17:46:55Z","receivedAt":"2019-08-09T17:47:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> So how about starting stupidly simple? You can test it everywhere, it\n> just needs libpcre2. It works without that library as well (tested with\n> \"make USE_LIBPCRE= USE_LIBPCRE2= test\"), but doesn't allocate anything\n> in that case, of course. The character tables leak fix should be safe\n> on top. If you detect performance issues then we can address them in\n> additional patches.\n>\n> -- >8 --\n> Subject: [PATCH] grep: use xmalloc() for all PCRE2 allocations\n>\n> Build a PCRE2 global custom context when compiling a pattern and use it\n> to tell the library to use xmalloc() for allocations. This provides\n> consistent out-of-memory handling and makes sure it uses a custom\n> allocator, e.g. with USE_NED_ALLOCATOR.\n>\n> Signed-off-by: René Scharfe \n> ---\n> The function names are ridiculously long, I tried to stay within 80\n> characters per line but gave up in the end and just kept going without\n> line breaks. Fits the \"stupidly simple\" approach..\n\n;-) Thanks for keeping the conversation going.\n\n"},{"id":"380243","messageId":"nycvar.QRO.7.76.6.1908092311510.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"CAPUEspibDcNfGk4=6Mvi2BHATGSxRPPph3F=3pLm_bLMBkBTNQ@mail.gmail.com","subject":"Re: [RFC PATCH v3 0/3] grep: no leaks or crashes (windows testing needed)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-09T21:13:03Z","receivedAt":"2019-08-09T21:13:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 8 Aug 2019, Carlo Arenas wrote:\n\n> PS. I might had broken something here as I can't see the test results\n> that failed\n> https://dev.azure.com/git/git/_build/results?buildId=857&view=ms.vss-test-web.build-test-results-tab\n\nThe test results load dynamically, so you'll probably just have to wait\nfor a couple moments. After that, just click on the lines indicating the\nfailed test cases to pop up a verbose trace of it.\n\nCiao,\nDscho\n"},{"id":"380246","messageId":"nycvar.QRO.7.76.6.1908092325480.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"d239326e-11c3-5875-13a8-f4123baea6eb@web.de","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-09T21:26:23Z","receivedAt":"2019-08-09T21:26:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n\nOn Fri, 9 Aug 2019, René Scharfe wrote:\n\n> Am 09.08.19 um 13:24 schrieb Carlo Arenas:\n> > disregard this version, it still broken (and wouldn't even build on\n> > some cases), the reasons why are still unclear though but at least it\n> > might\n> > seem from the last known run in windows that segfaults were prevented\n> > at last and something was still off enough to trigger a BUG (shouldn't\n> > be a concern with V6 or later that do PCRE2 migration to NED fully, as\n> > agreed)\n>\n> So how about starting stupidly simple?\n\nFWIW I am very much in favor of this approach.\n\nThanks,\nDscho\n\n> You can test it everywhere, it just needs libpcre2. It works without\n> that library as well (tested with \"make USE_LIBPCRE= USE_LIBPCRE2=\n> test\"), but doesn't allocate anything in that case, of course. The\n> character tables leak fix should be safe on top. If you detect\n> performance issues then we can address them in additional patches.\n>\n> -- >8 --\n> Subject: [PATCH] grep: use xmalloc() for all PCRE2 allocations\n>\n> Build a PCRE2 global custom context when compiling a pattern and use it\n> to tell the library to use xmalloc() for allocations. This provides\n> consistent out-of-memory handling and makes sure it uses a custom\n> allocator, e.g. with USE_NED_ALLOCATOR.\n>\n> Signed-off-by: René Scharfe \n> ---\n> The function names are ridiculously long, I tried to stay within 80\n> characters per line but gave up in the end and just kept going without\n> line breaks. Fits the \"stupidly simple\" approach..\n>\n> grep.c | 23 +++++++++++++++++------\n> grep.h | 2 ++\n> 2 files changed, 19 insertions(+), 6 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index cd952ef5d3..44f4e38657 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -482,6 +482,16 @@ static void free_pcre1_regexp(struct grep_pat *p)\n> #endif /* !USE_LIBPCRE1 */\n>\n> #ifdef USE_LIBPCRE2\n> +static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> +{\n> +\treturn xmalloc(size);\n> +}\n> +\n> +static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> +{\n> +\tfree(pointer);\n> +}\n> +\n> static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> {\n> \tint error;\n> @@ -495,12 +505,12 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>\n> \tassert(opt->pcre2);\n>\n> -\tp->pcre2_compile_context = NULL;\n> +\tp->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);\n> +\tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n>\n> \tif (opt->ignore_case) {\n> \t\tif (has_non_ascii(p->pattern)) {\n> -\t\t\tcharacter_tables = pcre2_maketables(NULL);\n> -\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(NULL);\n> +\t\t\tcharacter_tables = pcre2_maketables(p->pcre2_general_context);\n> \t\t\tpcre2_set_character_tables(p->pcre2_compile_context, character_tables);\n> \t\t}\n> \t\toptions |= PCRE2_CASELESS;\n> @@ -513,7 +523,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \t\t\t\t\t p->pcre2_compile_context);\n>\n> \tif (p->pcre2_pattern) {\n> -\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, NULL);\n> +\t\tp->pcre2_match_data = pcre2_match_data_create_from_pattern(p->pcre2_pattern, p->pcre2_general_context);\n> \t\tif (!p->pcre2_match_data)\n> \t\t\tdie(\"Couldn't allocate PCRE2 match data\");\n> \t} else {\n> @@ -550,10 +560,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> \t\t\treturn;\n> \t\t}\n>\n> -\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, NULL);\n> +\t\tp->pcre2_jit_stack = pcre2_jit_stack_create(1, 1024 * 1024, p->pcre2_general_context);\n> \t\tif (!p->pcre2_jit_stack)\n> \t\t\tdie(\"Couldn't allocate PCRE2 JIT stack\");\n> -\t\tp->pcre2_match_context = pcre2_match_context_create(NULL);\n> +\t\tp->pcre2_match_context = pcre2_match_context_create(p->pcre2_general_context);\n> \t\tif (!p->pcre2_match_context)\n> \t\t\tdie(\"Couldn't allocate PCRE2 match context\");\n> \t\tpcre2_jit_stack_assign(p->pcre2_match_context, NULL, p->pcre2_jit_stack);\n> @@ -605,6 +615,7 @@ static void free_pcre2_pattern(struct grep_pat *p)\n> \tpcre2_match_data_free(p->pcre2_match_data);\n> \tpcre2_jit_stack_free(p->pcre2_jit_stack);\n> \tpcre2_match_context_free(p->pcre2_match_context);\n> +\tpcre2_general_context_free(p->pcre2_general_context);\n> }\n> #else /* !USE_LIBPCRE2 */\n> static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> diff --git a/grep.h b/grep.h\n> index 1875880f37..73b8b87a3a 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -28,6 +28,7 @@ typedef int pcre_jit_stack;\n> #else\n> typedef int pcre2_code;\n> typedef int pcre2_match_data;\n> +typedef int pcre2_general_context;\n> typedef int pcre2_compile_context;\n> typedef int pcre2_match_context;\n> typedef int pcre2_jit_stack;\n> @@ -93,6 +94,7 @@ struct grep_pat {\n> \tint pcre1_jit_on;\n> \tpcre2_code *pcre2_pattern;\n> \tpcre2_match_data *pcre2_match_data;\n> +\tpcre2_general_context *pcre2_general_context;\n> \tpcre2_compile_context *pcre2_compile_context;\n> \tpcre2_match_context *pcre2_match_context;\n> \tpcre2_jit_stack *pcre2_jit_stack;\n> --\n> 2.22.0\n>\n"},{"id":"380253","messageId":"20190810030315.7519-1-carenas@gmail.com","threadId":"51590","inReplyTo":"nycvar.QRO.7.76.6.1908092325480.46@tvgsbejvaqbjf.bet","subject":"[PATCH] SQUASH","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-10T03:03:15Z","receivedAt":"2019-08-10T03:03:23Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Make using a general context (that is only needed with NED) to depend\non NED being selected at compile time.\n\nthe compile_context could be also make conditional but it gets ugly\nreally fasts with #ifdef\n---\n Makefile | 2 +-\n grep.c | 4 ++++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 8a7e235352..995e6c9351 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1774,7 +1774,7 @@ ifdef NATIVE_CRLF\n endif\n \n ifdef USE_NED_ALLOCATOR\n-\tCOMPAT_CFLAGS += -Icompat/nedmalloc\n+\tCOMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc\n \tCOMPAT_OBJS += compat/nedmalloc/nedmalloc.o\n \tOVERRIDE_STRDUP = YesPlease\n endif\ndiff --git a/grep.c b/grep.c\nindex 8255ec956e..233072ed80 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -482,6 +482,7 @@ static void free_pcre1_regexp(struct grep_pat *p)\n #endif /* !USE_LIBPCRE1 */\n \n #ifdef USE_LIBPCRE2\n+#ifdef USE_NED_ALLOCATOR\n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n {\n \treturn xmalloc(size);\n@@ -491,6 +492,7 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n {\n \tfree(pointer);\n }\n+#endif\n \n static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n {\n@@ -505,7 +507,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \n \tassert(opt->pcre2);\n \n+#ifdef USE_NED_ALLOCATOR\n \tp->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);\n+#endif\n \tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n \n \tif (opt->ignore_case) {\n-- \n2.23.0.rc2\n\n"},{"id":"380254","messageId":"CAPUEspjVMbjdgBrW3qS3jwbd2ZfqMinfgGiasEZGwyZB-1c+QA@mail.gmail.com","threadId":"51590","inReplyTo":"nycvar.QRO.7.76.6.1908092325480.46@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-10T03:05:16Z","receivedAt":"2019-08-10T03:05:33Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Aug 9, 2019 at 2:26 PM Johannes Schindelin\n wrote:\n>\n> FWIW I am very much in favor of this approach.\n\nFWIW this is (almost) exactly what I had in mind with my first reply,\nexcept I wanted to make setting of the global context be conditioned\nto having NED enabled to avoid possible performance issues in\nenvironments were NED is not even usable.\n\nin macOS (obviously testing without NED) the following is the output\nof (a hacked version) of p7801 for maint (against chromium's\nrepository), with René's patch on top\n\nTest HEAD^ HEAD\n--------------------------------------------------------------------------------------\n7820.1: perl grep 'how.to' 0.51(0.35+1.11)\n0.48(0.33+1.16) -5.9%\n7820.2: perl grep '^how to' 0.47(0.33+1.08)\n0.45(0.34+1.11) -4.3%\n7820.3: perl grep '[how] to' 0.49(0.40+1.11)\n0.53(0.41+1.13) +8.2%\n7820.4: perl grep '(e.t[^ ]*|v.ry) rare' 68.72(68.77+1.14)\n72.10(72.15+1.20) +4.9%\n7820.5: perl grep 'm(ú|u)lt.b(æ|y)te' 0.48(0.35+1.12)\n0.50(0.40+1.23) +4.2%\n\nand this is with my squashed[2] changed on top of that :\n\nTest HEAD^ HEAD\n--------------------------------------------------------------------------------------\n7820.1: perl grep 'how.to' 0.48(0.36+1.16)\n0.46(0.33+1.09) -4.2%\n7820.2: perl grep '^how to' 0.45(0.34+1.12)\n0.42(0.29+0.99) -6.7%\n7820.3: perl grep '[how] to' 0.48(0.40+1.13)\n0.52(0.43+1.16) +8.3%\n7820.4: perl grep '(e.t[^ ]*|v.ry) rare' 69.12(69.10+1.07)\n69.19(69.19+1.18) +0.1%\n7820.5: perl grep 'm(ú|u)lt.b(æ|y)te' 0.49(0.38+1.17)\n0.46(0.35+1.13) -6.1%\n\nthe degenerate case is not something we can't fix anyway, since it is\nlikely a locking issue inside PCRE2 (I see at most 1 CPU doing work),\nand the numbers are noisy because of the other problems I mentioned\nbefore (hardcoded to 8 threads, running in a laptop with low number of\ncores), which is why testing for performance regressions in windows is\nstrongly encouraged, regardless\n\nCarlo\n\n[1] https://public-inbox.org/git/CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com/\n[2] https://public-inbox.org/git/20190810030315.7519-1-carenas@gmail.com/\n"},{"id":"380259","messageId":"d8e55007-27bd-fcac-c918-bb712ec1bb8b@web.de","threadId":"51590","inReplyTo":"CAPUEspjVMbjdgBrW3qS3jwbd2ZfqMinfgGiasEZGwyZB-1c+QA@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-10T07:56:45Z","receivedAt":"2019-08-10T07:57:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.19 um 05:05 schrieb Carlo Arenas:\n> in macOS (obviously testing without NED) the following is the output\n> of (a hacked version) of p7801 for maint (against chromium's\n> repository), with René's patch on top\n\nDo you mean p7820? And what did you change? Looking at the results you\nremoved basic and extended from the list of regex engines, right?\n\nUgh, cloning https://chromium.googlesource.com/chromium/src.git sends\nmore than 16GB across the wire. Is that even the right repository? Not\nsure if my machine can keep the relevant parts cached while grepping --\nI/O times could drown out any difference due to context allocation and\nmemory allocator choice. Let's see...\n\n>\n> Test HEAD^ HEAD\n> --------------------------------------------------------------------------------------\n> 7820.1: perl grep 'how.to' 0.51(0.35+1.11)\n> 0.48(0.33+1.16) -5.9%\n> 7820.2: perl grep '^how to' 0.47(0.33+1.08)\n> 0.45(0.34+1.11) -4.3%\n> 7820.3: perl grep '[how] to' 0.49(0.40+1.11)\n> 0.53(0.41+1.13) +8.2%\n> 7820.4: perl grep '(e.t[^ ]*|v.ry) rare' 68.72(68.77+1.14)\n> 72.10(72.15+1.20) +4.9%\n> 7820.5: perl grep 'm(ú|u)lt.b(æ|y)te' 0.48(0.35+1.12)\n> 0.50(0.40+1.23) +4.2%\n>\n> and this is with my squashed[2] changed on top of that :\n>\n> Test HEAD^ HEAD\n> --------------------------------------------------------------------------------------\n> 7820.1: perl grep 'how.to' 0.48(0.36+1.16)\n> 0.46(0.33+1.09) -4.2%\n> 7820.2: perl grep '^how to' 0.45(0.34+1.12)\n> 0.42(0.29+0.99) -6.7%\n> 7820.3: perl grep '[how] to' 0.48(0.40+1.13)\n> 0.52(0.43+1.16) +8.3%\n> 7820.4: perl grep '(e.t[^ ]*|v.ry) rare' 69.12(69.10+1.07)\n> 69.19(69.19+1.18) +0.1%\n> 7820.5: perl grep 'm(ú|u)lt.b(æ|y)te' 0.49(0.38+1.17)\n> 0.46(0.35+1.13) -6.1%\n>\n> the degenerate case is not something we can't fix anyway, since it is\n> likely a locking issue inside PCRE2 (I see at most 1 CPU doing work),\n> and the numbers are noisy because of the other problems I mentioned\n> before (hardcoded to 8 threads, running in a laptop with low number of\n> cores), which is why testing for performance regressions in windows is\n> strongly encouraged, regardless\n>\n> Carlo\n>\n> [1] https://public-inbox.org/git/CAPUEspgH1v1zo7smzQWCV4rX9pKVKLV84gDSfCPdT7LffQxUWw@mail.gmail.com/\n> [2] https://public-inbox.org/git/20190810030315.7519-1-carenas@gmail.com/\n>\n\nSo I pointed GIT_PERF_LARGE_REPO to the monster mentioned above, ran the\ntest once for warmup and here are the results of the second run:\n\nTest origin/master pcre2-xmalloc pcre2-xmalloc+nedmalloc\n---------------------------------------------------------------------------------------------------------------------\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n7820.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%\n\npcre2-xmalloc is my patch on top of master, +nedmalloc has the warning\nfixes I sent earlier and sets USE_NED_MALLOC.\n\nI don't understand why my performance is lower by factor 2.5 than yours\nfor all perl regex tests except 7820.15 (your 7820.4), where my system\nis two times faster. Debian Testing, GCC 9.1.0, i5-9600K, 16GB RAM.\n\nAnyway, nedmalloc is slower across the board, but the impact of my\npatch is in the noise. Right?\n\nRené\n"},{"id":"380260","messageId":"f78b57a6-9ede-c87c-fc42-292851d79fad@web.de","threadId":"51590","inReplyTo":"20190810030315.7519-1-carenas@gmail.com","subject":"Re: [PATCH] SQUASH","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-10T07:57:01Z","receivedAt":"2019-08-10T07:57:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.19 um 05:03 schrieb Carlo Marcelo Arenas Belón:\n> Make using a general context (that is only needed with NED) to depend\n> on NED being selected at compile time.\n\nA custom general context is needed to convince PCRE2 to use xmalloc()\ninstead of mallo(). That is independent of the choice of allocator.\nSo this change would be a step backwards?\n\nRené\n"},{"id":"380261","messageId":"CAPUEspjtZhhynEmJu_weROaao=1zL+De1h50R-grG36ok+EAyw@mail.gmail.com","threadId":"51590","inReplyTo":"f78b57a6-9ede-c87c-fc42-292851d79fad@web.de","subject":"Re: [PATCH] SQUASH","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-10T08:42:33Z","receivedAt":"2019-08-10T08:42:48Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Aug 10, 2019 at 12:57 AM René Scharfe wrote:\n>\n> Am 10.08.19 um 05:03 schrieb Carlo Marcelo Arenas Belón:\n> > Make using a general context (that is only needed with NED) to depend\n> > on NED being selected at compile time.\n>\n> A custom general context is needed to convince PCRE2 to use xmalloc()\n> instead of mallo(). That is independent of the choice of allocator.\n> So this change would be a step backwards?\n\nMy bad, you are correct.\n\nDo you mind then if I \"adopt\" your patch and submit a reroll with it,\nwill also add an \"equivalent\" one to fix PCRE1 as well then, and we\nwill tackle any performance deficiencies or other issues in a future\nseries.\n\nCarlo\n"},{"id":"380263","messageId":"CAPUEsph+hjU2UstL=YW-2qKC3pGJrOwaw+VCiHH74HZ2=oupig@mail.gmail.com","threadId":"51590","inReplyTo":"d8e55007-27bd-fcac-c918-bb712ec1bb8b@web.de","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-10T12:40:46Z","receivedAt":"2019-08-10T12:41:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Aug 10, 2019 at 12:57 AM René Scharfe wrote:\n>\n> Am 10.08.19 um 05:05 schrieb Carlo Arenas:\n> > in macOS (obviously testing without NED) the following is the output\n> > of (a hacked version) of p7801 for maint (against chromium's\n> > repository), with René's patch on top\n>\n> Do you mean p7820? And what did you change? Looking at the results you\n> removed basic and extended from the list of regex engines, right?\n\ncorrect, that was a weird typo there; apologize for the confusion.\nyou were correct about my changes; ironically, I didn't spell those\nchanges out to avoid confusion.\n\n> Ugh, cloning https://chromium.googlesource.com/chromium/src.git sends\n> more than 16GB across the wire. Is that even the right repository?\n\nnoticed it was mentioned before by people[1] doing performance testing\non grep specifically and\nthought it was better than try to come with another one, the linux\nkernel wouldn't work in macOS\nbecause it breaks \"run\" as it fails to checkout in a case insensitive\nfilesystem.\n\n> Not\n> sure if my machine can keep the relevant parts cached while grepping --\n> I/O times could drown out any difference due to context allocation and\n> memory allocator choice. Let's see...\n\n;), try setting /proc/sys/vm/swappiness to 0 or get more RAM\n\n> I don't understand why my performance is lower by factor 2.5 than yours\n> for all perl regex tests except 7820.15 (your 7820.4), where my system\n> is two times faster. Debian Testing, GCC 9.1.0, i5-9600K, 16GB RAM.\n\ninteresting; did you also see at most 100% of one CPU being used?\nyours seem to be faster than mine so this might be representative of\nsingle threaded performance.\n\n> Anyway, nedmalloc is slower across the board, but the impact of my\n> patch is in the noise. Right?\n\nyes, and there is a lot of noise.\n\nCarlo\n\n[1] https://public-inbox.org/git/e72330c6747218545cce1b6b1edfd1e448141a8f.1563570204.git.matheus.bernardino@usp.br/\n"},{"id":"380264","messageId":"nycvar.QRO.7.76.6.1908101556530.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"20190810030315.7519-1-carenas@gmail.com","subject":"Re: [PATCH] SQUASH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-10T13:57:47Z","receivedAt":"2019-08-10T13:57:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Fri, 9 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n\n> diff --git a/grep.c b/grep.c\n> index 8255ec956e..233072ed80 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -482,6 +482,7 @@ static void free_pcre1_regexp(struct grep_pat *p)\n> #endif /* !USE_LIBPCRE1 */\n>\n> #ifdef USE_LIBPCRE2\n> +#ifdef USE_NED_ALLOCATOR\n\nI really, really, really, really, _really_ don't want this to be a\ncompile-time thing. Really. No, really. I mean it.\n\nCiao,\nDscho\n\n> static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n> {\n> \treturn xmalloc(size);\n> @@ -491,6 +492,7 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n> {\n> \tfree(pointer);\n> }\n> +#endif\n>\n> static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt)\n> {\n> @@ -505,7 +507,9 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>\n> \tassert(opt->pcre2);\n>\n> +#ifdef USE_NED_ALLOCATOR\n> \tp->pcre2_general_context = pcre2_general_context_create(pcre2_malloc, pcre2_free, NULL);\n> +#endif\n> \tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n>\n> \tif (opt->ignore_case) {\n> --\n> 2.23.0.rc2\n>\n>\n"},{"id":"380267","messageId":"501f946a-1f18-7c6a-2d15-8ff6375c4192@web.de","threadId":"51590","inReplyTo":"CAPUEspjtZhhynEmJu_weROaao=1zL+De1h50R-grG36ok+EAyw@mail.gmail.com","subject":"Re: [PATCH] SQUASH","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-10T19:47:49Z","receivedAt":"2019-08-10T19:48:06Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.19 um 10:42 schrieb Carlo Arenas:\n> On Sat, Aug 10, 2019 at 12:57 AM René Scharfe wrote:\n>>\n>> Am 10.08.19 um 05:03 schrieb Carlo Marcelo Arenas Belón:\n>>> Make using a general context (that is only needed with NED) to depend\n>>> on NED being selected at compile time.\n>>\n>> A custom general context is needed to convince PCRE2 to use xmalloc()\n>> instead of mallo(). That is independent of the choice of allocator.\n>> So this change would be a step backwards?\n>\n> My bad, you are correct.\n>\n> Do you mind then if I \"adopt\" your patch and submit a reroll with it,\n> will also add an \"equivalent\" one to fix PCRE1 as well then, and we\n> will tackle any performance deficiencies or other issues in a future\n> series.\n\nI don't mind, sounds good.\n\nRené\n"},{"id":"380274","messageId":"e127a2d0-7ce4-71af-3d91-8b5515af2dc8@web.de","threadId":"51590","inReplyTo":"CAPUEsph+hjU2UstL=YW-2qKC3pGJrOwaw+VCiHH74HZ2=oupig@mail.gmail.com","subject":"Re: [RFC PATCH v5 0/3] grep: almost no more leaks, hopefully no crashes","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-10T21:16:05Z","receivedAt":"2019-08-10T21:16:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 10.08.19 um 14:40 schrieb Carlo Arenas:\n> On Sat, Aug 10, 2019 at 12:57 AM René Scharfe wrote:\n>> I don't understand why my performance is lower by factor 2.5 than yours\n>> for all perl regex tests except 7820.15 (your 7820.4), where my system\n>> is two times faster. Debian Testing, GCC 9.1.0, i5-9600K, 16GB RAM.\n>\n> interesting; did you also see at most 100% of one CPU being used?\n\nFor 7820.15 yes -- 100% of a single core, the rest idling. The other\ntests had only brief sequential phases, if at all, and used all cores.\n\nRené\n\n"},{"id":"380298","messageId":"CAPUEspiuuCWgjuHZ7DLyH+0hXt7MqeP++xyZuB+Zd5t-H_d69Q@mail.gmail.com","threadId":"51590","inReplyTo":"501f946a-1f18-7c6a-2d15-8ff6375c4192@web.de","subject":"Re: [PATCH] SQUASH","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-12T07:35:32Z","receivedAt":"2019-08-12T07:35:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sat, Aug 10, 2019 at 12:48 PM René Scharfe wrote:\n> > On Sat, Aug 10, 2019 at 12:57 AM René Scharfe wrote:\n> >\n> > Do you mind then if I \"adopt\" your patch and submit a reroll with it,\n>\n> I don't mind, sounds good.\n\nI had to squash another fix that was reported[1] before but wasn't picked up\nand that ironically might explain the last segfaults from my old V4\n\nCarlo\n\n[1] https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/\n"},{"id":"380300","messageId":"370b308e-40a2-4edd-b0db-ab0fb84d1c9e@web.de","threadId":"51590","inReplyTo":"CAPUEspiuuCWgjuHZ7DLyH+0hXt7MqeP++xyZuB+Zd5t-H_d69Q@mail.gmail.com","subject":"Re: [PATCH] SQUASH","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-08-12T12:14:39Z","receivedAt":"2019-08-12T12:14:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.08.19 um 09:35 schrieb Carlo Arenas:\n> On Sat, Aug 10, 2019 at 12:48 PM René Scharfe wrote:\n>>> On Sat, Aug 10, 2019 at 12:57 AM René Scharfe wrote:\n>>>\n>>> Do you mind then if I \"adopt\" your patch and submit a reroll with it,\n>>\n>> I don't mind, sounds good.\n>\n> I had to squash another fix that was reported[1] before but wasn't picked up\n> and that ironically might explain the last segfaults from my old V4\n>\n> Carlo\n>\n> [1] https://public-inbox.org/git/20190721194052.15440-1-carenas@gmail.com/\n\nThat looks like an issue worth its own commit.\n\nI wonder if we'd want to pass pcre2_match_context to pcre2_match() a few\nlines down as well, for consistency.\n\nRené\n"},{"id":"380301","messageId":"CAPUEspiFuvgMQ3W1se1B=8aTTrQsJSZTyQTzG1TpiyN8HTOpkA@mail.gmail.com","threadId":"51590","inReplyTo":"370b308e-40a2-4edd-b0db-ab0fb84d1c9e@web.de","subject":"Re: [PATCH] SQUASH","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-12T12:28:06Z","receivedAt":"2019-08-12T12:28:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Aug 12, 2019 at 5:14 AM René Scharfe wrote:\n>\n> That looks like an issue worth its own commit.\n\nOK, but then will make my topic interesting; indeed it almost feels\nlike it should be 3 different topics all depending of each other in a\nchain:\n\n* really use the match context\n* move to xmalloc (2 patches, one for each PCRE library)\n* fix leak\n\nWhich then will conflict with ab/pcre-jit-fixes before it can get merged to pu\n\n> I wonder if we'd want to pass pcre2_match_context to pcre2_match() a few\n> lines down as well, for consistency.\n\nyes, that is actually what was tested[1] as can be seen in github\nwhere Dscho integration did most of the heavy lifting with Windows\n\nCarlo\n\n[1] https://github.com/git/git/pull/627\n"},{"id":"381326","messageId":"nycvar.QRO.7.76.6.1908271057280.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"20190809030210.18353-3-carenas@gmail.com","subject":"Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-08-27T09:07:06Z","receivedAt":"2019-08-27T09:07:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 8 Aug 2019, Carlo Marcelo Arenas Belón wrote:\n\n> 94da9193a6 (grep: add support for PCRE v2, 2017-06-01) didn't include\n> a way to override the system allocator, and so it is incompatible with\n> USE_NED_ALLOCATOR. The problem was made visible when an attempt to\n> avoid a leak in a data structure that is created by the library was\n> passed to NED's free for disposal triggering a segfault in Windows.\n>\n> PCRE2 requires the use of a general context to override the allocator\n> and therefore, there is a lot more code needed than in PCRE1, including\n> a couple of wrapper functions.\n>\n> Extend the grep API with a \"destructor\" that could be called to cleanup\n> any objects that were created and used globally.\n>\n> Update builtin/grep to use that new API, but any other future\n> users should make sure to have matching grep_init/grep_destroy calls\n> if they are using the pattern matching functionality (currently only\n> relevant when using both NED and PCRE2)\n>\n> Move the logic to decide if a general context will be needed to an\n> earlier phase so it will only be done once per pattern (instead of\n> at least once per worker thread) avoiding then the need for locking.\n>\n> This change does the minimum change required to hopefully solve the\n> crash, with the rest of the users of it added later.\n>\n> Helped-by: René Scharfe \n> Reported-by: Johannes Schindelin \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n\nUnfortunately, this is _still_ incorrect.\n\nI pointed out multiple times that custom allocators can be activated at\nrun-time rather than compile-time, therefore making the choice at\ncompile-time is wrong. Besides, there is nothing specific to nedmalloc\nabout this. So the patch is double-wrong on that account.\n\nThe patch has a yet even more immediate problem: t7816.48 is failing in\nthe CI build for _weeks_ now: it requires that global context to be\ninitialized, but no code path hits the initialization, resulting in a\nvery, very ugly:\n\n\tBUG: grep.c:516: pcre2_global_context uninitialized\n\nSee\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=15151&view=ms.vss-test-web.build-test-results-tab&runId=41282&resultId=101710&paneView=debug\nfor details.\n\nAll of this could be easily avoided. As I had pointed out already, the\nobvious fragility is not worth the optimization, and we should just\ninitialize the global context always, as does this patch\n(https://github.com/git-for-windows/git/commit/5e5b959169e6efee73e0b50e464166822b7d2d07).\n\nI don't claim that this is complete, you need to check carefully (for\nexample, I think you will want to get rid of _all_ the references to\nnedmalloc), but this patch is at least a stop-gap measure to fix the CI\nbuild (Junio, would you mind adding this as a SQUASH??? so that this\nbreakage won't keep the CI build of `pu` in the failing state?):\n\n-- snipsnap --\ndiff --git a/grep.c b/grep.c\nindex ec845141bbb..4242ad0b4ae 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -19,7 +19,6 @@ static struct grep_opt grep_defaults;\n #ifdef USE_LIBPCRE2\n static pcre2_general_context *pcre2_global_context;\n\n-#ifdef USE_NED_ALLOCATOR\n static void *pcre2_malloc(PCRE2_SIZE size, MAYBE_UNUSED void *memory_data)\n {\n \treturn malloc(size);\n@@ -30,7 +29,6 @@ static void pcre2_free(void *pointer, MAYBE_UNUSED void *memory_data)\n \treturn free(pointer);\n }\n #endif\n-#endif\n\n static const char *color_grep_slots[] = {\n \t[GREP_COLOR_CONTEXT]\t = \"context\",\n@@ -176,6 +174,12 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \tstruct grep_opt *def = &grep_defaults;\n \tint i;\n\n+#if defined(USE_LIBPCRE2)\n+\tif (!pcre2_global_context)\n+\t\tpcre2_global_context = pcre2_general_context_create(\n+\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n+#endif\n+\n #ifdef USE_NED_ALLOCATOR\n #ifdef USE_LIBPCRE1\n \tpcre_malloc = malloc;\n@@ -343,11 +347,6 @@ void append_header_grep_pattern(struct grep_opt *opt,\n void append_grep_pattern(struct grep_opt *opt, const char *pat,\n \t\t\t const char *origin, int no, enum grep_pat_token t)\n {\n-#if defined(USE_LIBPCRE2) && defined(USE_NED_ALLOCATOR)\n-\tif (!pcre2_global_context && opt->ignore_case && has_non_ascii(pat))\n-\t\tpcre2_global_context = pcre2_general_context_create(\n-\t\t\t\t\tpcre2_malloc, pcre2_free, NULL);\n-#endif\n \tappend_grep_pat(opt, pat, strlen(pat), origin, no, t);\n }\n\n--\n2.23.0.windows.1\n"},{"id":"381346","messageId":"CAPUEspjJNSrJQT7xV2fsdp2t5odW5fzzPdDxuar_5x_JPUtf6Q@mail.gmail.com","threadId":"51590","inReplyTo":"nycvar.QRO.7.76.6.1908271057280.46@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-27T11:51:14Z","receivedAt":"2019-08-27T11:51:28Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Aug 27, 2019 at 2:07 AM Johannes Schindelin\n wrote:\n>\n> Unfortunately, this is _still_ incorrect.\n\nI know, and that is why we worked out a v6 RC with Rene than then was\npushed to github[0] and validated to work thanks to your integration\nas detailed in [1], I never got to send an update to the list though\nbecause Rene wanted my squashing into his patch to be independent as\ndetailed there and because I assumed that when Junio didn't pull my\nreroll, it was probably because your tree had a fix already anyway.\n\nthe recent discovery that xmalloc wasn't thread safe though,\ncomplicates things further and that is why I was expecting to reroll\nthis on top of both ab/pcre-jit-fixes and jk/drop-release-pack-memory\n(later one already in next) as detailed in [2]\n\n> I pointed out multiple times that custom allocators can be activated at\n> run-time rather than compile-time, therefore making the choice at\n> compile-time is wrong. Besides, there is nothing specific to nedmalloc\n> about this. So the patch is double-wrong on that account.\n\nJust to clarify, I think my patch accounts for that (haven't tested\nthat assumption, but will do now that I have a windows box, probably\neven with mi-alloc) but yes, the only reason why there were references\nto NEDMALLOC was to isolate the code and make sure the fix was\ntackling the problem, it was not my intention to do so at the end,\nspecially once we agreed that xmalloc should be used anyway.\n\n> The patch has a yet even more immediate problem: t7816.48 is failing in\n> the CI build for _weeks_ now: it requires that global context to be\n> initialized, but no code path hits the initialization, resulting in a\n> very, very ugly:\n>\n> BUG: grep.c:516: pcre2_global_context uninitialized\n>\n> See\n> 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\n> for details.\n\nIMHO this is probably testing V3 (from pu) and which was hopefully\nfixed in the last github force push for my branch\n\n> All of this could be easily avoided. As I had pointed out already, the\n> obvious fragility is not worth the optimization, and we should just\n> initialize the global context always, as does this patch\n> (https://github.com/git-for-windows/git/commit/5e5b959169e6efee73e0b50e464166822b7d2d07).\n\nironically only found out about that patch after I got a windows box\n(running Windows Home though) and had finished testing my own squashed\nfix[3] that has succeeded being validated in GitHub\n\napologize for the delays, and will be fine using your squash, mine,\nthe V6 RC (my preference) or dropping this series from pu if that\nwould help clear the ugliness of pu for windows\n\nhopefully this won't be repeated now that I am aware of github's\nintegration and have my own (albeit very slow) windows environment as\nwell.\n\nCarlo\n\n[0] https://github.com/git/git/commit/0ca5d0550c17a68d83b8922b71aeff891958ed0e\n[1] https://public-inbox.org/git/CAPUEspiFuvgMQ3W1se1B=8aTTrQsJSZTyQTzG1TpiyN8HTOpkA@mail.gmail.com/\n[2] https://public-inbox.org/git/CAPUEspg9F7RutCUCoRAAXmRePjiunq3-zG7cN3uz_t5DVMxP=g@mail.gmail.com/\n[3] https://github.com/git/git/pull/627\n"},{"id":"383324","messageId":"xmqqwodmbewl.fsf@gitster-ct.c.googlers.com","threadId":"51590","inReplyTo":"CAPUEspjJNSrJQT7xV2fsdp2t5odW5fzzPdDxuar_5x_JPUtf6Q@mail.gmail.com","subject":"Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-03T05:02:02Z","receivedAt":"2019-10-03T05:02:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> On Tue, Aug 27, 2019 at 2:07 AM Johannes Schindelin\n> wrote:\n>>\n>> Unfortunately, this is _still_ incorrect.\n> ...\n> Just to clarify, I think my patch accounts for that (haven't tested\n> that assumption, but will do now that I have a windows box, probably\n> even with mi-alloc) but yes, the only reason why there were references\n> to NEDMALLOC was to isolate the code and make sure the fix was\n> tackling the problem, it was not my intention to do so at the end,\n> specially once we agreed that xmalloc should be used anyway.\n> ...\n> apologize for the delays, and will be fine using your squash, mine,\n> the V6 RC (my preference) or dropping this series from pu if that\n> would help clear the ugliness of pu for windows\n\nSo,... have we seen any conclusion on this? Can any of you guys\ngive us a pointer to or copies of the candidate to be the final\nsolution of this topic, please?\n\nThanks.\n"},{"id":"383328","messageId":"nycvar.QRO.7.76.6.1910031007060.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"xmqqwodmbewl.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-03T08:08:59Z","receivedAt":"2019-10-03T08:09:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 3 Oct 2019, Junio C Hamano wrote:\n\n> Carlo Arenas writes:\n>\n> > On Tue, Aug 27, 2019 at 2:07 AM Johannes Schindelin\n> > wrote:\n> >>\n> >> Unfortunately, this is _still_ incorrect.\n> > ...\n> > Just to clarify, I think my patch accounts for that (haven't tested\n> > that assumption, but will do now that I have a windows box, probably\n> > even with mi-alloc) but yes, the only reason why there were references\n> > to NEDMALLOC was to isolate the code and make sure the fix was\n> > tackling the problem, it was not my intention to do so at the end,\n> > specially once we agreed that xmalloc should be used anyway.\n> > ...\n> > apologize for the delays, and will be fine using your squash, mine,\n> > the V6 RC (my preference) or dropping this series from pu if that\n> > would help clear the ugliness of pu for windows\n>\n> So,... have we seen any conclusion on this? Can any of you guys\n> give us a pointer to or copies of the candidate to be the final\n> solution of this topic, please?\n\nI still need\nhttps://github.com/git-for-windows/git/commit/719beb813e4f27f090696ce583df3e5f3c480545\nand\nhttps://github.com/git-for-windows/git/commit/3369c322bbd95820b971701fef7db44b26dd826f\nto fix that part in Git for Windows' `shears/pu` branch (i.e. the\ncontinuously rebased patch thicket).\n\nCiao,\nDscho\n"},{"id":"383329","messageId":"CAPUEspiMOn8h0+WEkpe0CRgOQohKL-07TacM6CmiZBZh7GBCnw@mail.gmail.com","threadId":"51590","inReplyTo":"nycvar.QRO.7.76.6.1910031007060.46@tvgsbejvaqbjf.bet","subject":"Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-10-03T11:17:42Z","receivedAt":"2019-10-03T11:17:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Oct 3, 2019 at 1:09 AM Johannes Schindelin\n wrote:\n> I still need\n> https://github.com/git-for-windows/git/commit/719beb813e4f27f090696ce583df3e5f3c480545\n> and\n> https://github.com/git-for-windows/git/commit/3369c322bbd95820b971701fef7db44b26dd826f\n> to fix that part in Git for Windows' `shears/pu` branch (i.e. the\n> continuously rebased patch thicket).\n\nor we could drop the current branch in pu and start again from scratch\nnow that all of the required dependencies had been merged.\n\napologize for the delays otherwise; $DAYJOB has taken a toll lately\nand even my new shiny windows dev box hasn't seen much action.\n\nwill update here and in github shortly (which might mean up to this\nweekend, being realistic), but should be better code (since it is\nmostly Rene's) and better tested that way and hopefully won't cause\nmore breakage (specially in Windows, sorry Dscho)\n\nCarlo\n"},{"id":"383342","messageId":"nycvar.QRO.7.76.6.1910032022180.46@tvgsbejvaqbjf.bet","threadId":"51590","inReplyTo":"CAPUEspiMOn8h0+WEkpe0CRgOQohKL-07TacM6CmiZBZh7GBCnw@mail.gmail.com","subject":"Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-10-03T18:23:36Z","receivedAt":"2019-10-03T18:24:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Thu, 3 Oct 2019, Carlo Arenas wrote:\n\n> On Thu, Oct 3, 2019 at 1:09 AM Johannes Schindelin\n> wrote:\n> > I still need\n> > https://github.com/git-for-windows/git/commit/719beb813e4f27f090696ce583df3e5f3c480545\n> > and\n> > https://github.com/git-for-windows/git/commit/3369c322bbd95820b971701fef7db44b26dd826f\n> > to fix that part in Git for Windows' `shears/pu` branch (i.e. the\n> > continuously rebased patch thicket).\n>\n> or we could drop the current branch in pu and start again from scratch\n> now that all of the required dependencies had been merged.\n\nI hope that your next iteration won't have any `#ifdef NED` in it?\n\n>\n> apologize for the delays otherwise; $DAYJOB has taken a toll lately\n> and even my new shiny windows dev box hasn't seen much action.\n>\n> will update here and in github shortly (which might mean up to this\n> weekend, being realistic), but should be better code (since it is\n> mostly Rene's) and better tested that way and hopefully won't cause\n> more breakage (specially in Windows, sorry Dscho)\n\nI appreciate all your work!\n\nThanks,\nDscho\n"},{"id":"383377","messageId":"xmqqlfu1a14a.fsf@gitster-ct.c.googlers.com","threadId":"51590","inReplyTo":"CAPUEspiMOn8h0+WEkpe0CRgOQohKL-07TacM6CmiZBZh7GBCnw@mail.gmail.com","subject":"Re: [RFC PATCH v5 2/3] grep: make PCRE2 aware of custom allocator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-03T22:57:25Z","receivedAt":"2019-10-03T22:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> or we could drop the current branch in pu and start again from scratch\n> now that all of the required dependencies had been merged.\n\nThat would be the cleanest from my point of view. Thanks, both.\n\n"}]}