Re: [PATCH 1/3] grep: make PCRE1 aware of custom allocator
- From
Carlo Arenas <carenas@gmail.com>
- Date
- Aug 8, 2019, 15:19 UTC
- Message-ID
- <CAPUEsphLZ+x1hL+=8P+bh34mR6pUcXB5Qd2htj4AzUjjrKC=nQ@mail.gmail.com>
- In-Reply-To
- <nycvar.QRO.7.76.6.1908081552440.46@tvgsbejvaqbjf.bet>
On Thu, Aug 8, 2019 at 6:55 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
Show 31 quoted lines
> On Tue, 6 Aug 2019, Carlo Marcelo Arenas Belón wrote:
>
> > 63e7e9d8b6 ("git-grep: Learn PCRE", 2011-05-09) didn't include a way
> > to override the system allocator, and so it is incompatible with
> > USE_NED_ALLOCATOR as reported by Dscho[1] (in similar code from PCRE2)
> >
> > Make the minimum change possible to ensure this combination is supported
> >
> > [1] https://public-inbox.org/git/pull.306.git.gitgitgadget@gmail.com
> >
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > ---
> > Makefile | 2 +-
> > grep.c | 10 ++++++++++
> > 2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index bd246f2989..4b384f3759 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1764,7 +1764,7 @@ ifdef NATIVE_CRLF
> > endif
> >
> > ifdef USE_NED_ALLOCATOR
> > - COMPAT_CFLAGS += -Icompat/nedmalloc
> > + COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -Icompat/nedmalloc
>
> This pretends that all custom allocators are selected at build time,
> something I tried to stress in my commit message as not true. You can
> pre-load not only nedmalloc, but also jemalloc and unless I am mistaken
> also tcmalloc. And mi-malloc.the patch only fixes the case when NED was chosen at build time, which before this patch meant that the system allocator was used by PCRE1 and NED by git.
That combination happened to work unless any pointer crossed over but neglected any benefit of using NED instead of the system allocator with PCRE1.
the PCRE1 API was therefore safer to use, PCRE2 is not and that will be addressed[1] in a future version by making sure this "layering violation" is covered
runtime custom allocator changes are not affected and will keep working as usual, which (at least in Linux when using LD_PRELOAD which is probably what you are refer to by "preload") means that the dynamic linker will replace all calls to malloc/free with the ones you provided at exec time and before it starts the binary (including whichever libraries it loaded) and therefore will make both git and PCRE2 to use the same allocator.
nedmalloc seems (at least in Linux) to be smart enough to cope with pointers that were not allocated by itself and doesn't crash so presume there is something else in Windows (maybe the dynamic linker, if there is one) that might be also part of the reason for this segfault.
of course, the trigger of this is my patch, so I'll take the blame for this bug regardless and I am looking forward to a fix
> So the premise of this patch, that you can tell at compile time that a > different allocator than the system one will be in use is simply > incorrect.
in the context that the only available (and integrated with our build system) allocator is NED at compile time, I think it is correct. you are correct though that if another allocator will be added as a compile time option, we will need to do something similar as well.
Carlo
[1] https://bugs.exim.org/show_bug.cgi?id=2429