Re: [PATCH 7/7] xdiff: remove xdl_free(), use free() instead

[Phillip Wood <phillip.wood123@gmail.com>]

On 11/07/2022 11:02, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Jul 11 2022, Phillip Wood wrote:
> 
>> Hi Ævar
>>
>> On 08/07/2022 22:26, Ævar Arnfjörð Bjarmason wrote:
>>> On Fri, Jul 08 2022, Phillip Wood wrote:
>>>
>>>> Hi Ævar
>>>>
>>>> On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote:
>>>>> Remove the xdl_free() wrapper macro in favor of using free()
>>>>> directly. The wrapper macro was brought in with the initial import of
>>>>> xdiff/ in 3443546f6ef (Use a *real* built-in diff generator,
>>>>> 2006-03-24).
>>>>> As subsequent discussions on the topic[1] have made clear there's no
>>>>> reason to use this wrapper.
>>>>
>>>> That link is to a message where you assert that there is no reason for
>>>> the wrapper, you seem to be the only person in that thread making that
>>>> argument. The libgit2 maintainer and others are arguing that it is
>>>> worth keeping to make it easy to swap the allocator.
>>> Arguing that it's not needed for a technical reason, but an
>>> "aesthetic
>>> and ergonomic one", per:
>>> https://lore.kernel.org/git/20220217225804.GC7@edef91d97c94/;
>>> Perhaps I misread it, but I took this as Junio concurring with what
>>> I
>>> was pointing out there:
>>> https://lore.kernel.org/git/xmqqfsohbdre.fsf@gitster.g/
>>>
>>>>> Both git itself as well as any external
>>>>> users such as libgit2 compile the xdiff/* code as part of their own
>>>>> compilation, and can thus find the right malloc() and free() at
>>>>> link-time.
>>>>
>>>> I'm struggling to see how that is simpler than the current situation
>>>> with xdl_malloc().
>>> It's simpler because when maintaining that code in git.git it's less
>>> of
>>> a special case, e.g. we have coccinelle rules that match free(), now
>>> every such rule needs to account for the xdiff special-case.
>>> And it's less buggy because if you're relying on us always using a
>>> wrapper bugs will creep in, current master has this:
>>> 	
>>> 	$ git -P grep '\bfree\(' -- xdiff
>>> 	xdiff/xdiff.h:#define xdl_free(ptr) free(ptr)
>>> 	xdiff/xmerge.c:         free(c);
>>> 	xdiff/xmerge.c: free(next_m);
>>>
>>>> Perhaps you could show how you think libgit2 could
>>>> easily make xdiff use git__malloc() instead of malloc() without
>>>> changing the malloc() calls (the message you linked to essentially
>>>> suggests they do a search and replace). If people cannot swap in
>>>> another allocator without changing the code then you are imposing a
>>>> barrier to them contributing patches back to git's xdiff.
>>> I was suggesting that if libgit2 wanted to maintain a copy of xdiff
>>> that
>>> catered to the asthetic desires of the maintainer adjusting whatever
>>> import script you use to apply a trivial coccinelle transformation would
>>> give you what you wanted. Except without a maintenance burden on
>>> git.git, or the bugs you'd get now since you're not catching those two
>>> free() cases (or any future such cases).
>>> But just having the code use malloc() and free() directly and
>>> getting
>>> you what you get now is easy, and doesn't require any such
>>> search-replacement.
>>> Here's how:
>>> I imported the xdiff/*.[ch] files at the tip of my branch into
>>> current
>>> libgit2.git's src/libgit2/xdiff/, and then applied this on top, which is
>>> partially re-applying libgit2's own monkeypatches, and partially
>>> adjusting for upstream changes (including this one):
>>> 	
>>> 	diff --git a/src/libgit2/xdiff/git-xdiff.h b/src/libgit2/xdiff/git-xdiff.h
>>> 	index b75dba819..2e00764d4 100644
>>> 	--- a/src/libgit2/xdiff/git-xdiff.h
>>> 	+++ b/src/libgit2/xdiff/git-xdiff.h
>>> 	@@ -14,6 +14,7 @@
>>> 	 #ifndef INCLUDE_git_xdiff_h__
>>> 	 #define INCLUDE_git_xdiff_h__
>>> 	
>>> 	+#include <regex.h>
>>> 	 #include "regexp.h"
>>> 	
>>> 	 /* Work around C90-conformance issues */
>>> 	@@ -27,11 +28,10 @@
>>> 	 # endif
>>> 	 #endif
>>> 	
>>> 	-#define xdl_malloc(x) git__malloc(x)
>>> 	-#define xdl_free(ptr) git__free(ptr)
>>> 	-#define xdl_realloc(ptr, x) git__realloc(ptr, x)
>>> 	+#define malloc(x) git__malloc(x)
>>> 	+#define free(ptr) git__free(ptr)
>>> 	
>>> 	-#define XDL_BUG(msg) GIT_ASSERT(msg)
>>> 	+#define BUG(msg) GIT_ASSERT(msg)
>>> 	
>>> 	 #define xdl_regex_t git_regexp
>>> 	 #define xdl_regmatch_t git_regmatch
>>> 	@@ -50,4 +50,17 @@ GIT_INLINE(int) xdl_regexec_buf(
>>> 	 	return -1;
>>> 	 }
>>> 	
>>> 	+static inline size_t st_mult(size_t a, size_t b)
>>> 	+{
>>> 	+	return a * b;
>>> 	+}
>>> 	+
>>> 	+static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
>>> 	+			      size_t nmatch, regmatch_t pmatch[], int eflags)
>>> 	+{
>>> 	+	assert(nmatch > 0 && pmatch);
>>> 	+	pmatch[0].rm_so = 0;
>>> 	+	pmatch[0].rm_eo = size;
>>> 	+	return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
>>> 	+}
>>> 	 #endif
>>> 	diff --git a/src/libgit2/xdiff/xdiff.h b/src/libgit2/xdiff/xdiff.h
>>> 	index a37d89fcd..5e5b525c2 100644
>>> 	--- a/src/libgit2/xdiff/xdiff.h
>>> 	+++ b/src/libgit2/xdiff/xdiff.h
>>> 	@@ -23,6 +23,8 @@
>>> 	 #if !defined(XDIFF_H)
>>> 	 #define XDIFF_H
>>> 	
>>> 	+#include "git-xdiff.h"
>>> 	+
>>> 	 #ifdef __cplusplus
>>> 	 extern "C" {
>>> 	 #endif /* #ifdef __cplusplus */
>>> 	diff --git a/src/libgit2/xdiff/xinclude.h b/src/libgit2/xdiff/xinclude.h
>>> 	index a4285ac0e..79812ad8a 100644
>>> 	--- a/src/libgit2/xdiff/xinclude.h
>>> 	+++ b/src/libgit2/xdiff/xinclude.h
>>> 	@@ -23,7 +23,8 @@
>>> 	 #if !defined(XINCLUDE_H)
>>> 	 #define XINCLUDE_H
>>> 	
>>> 	-#include "git-compat-util.h"
>>> 	+#include "git-xdiff.h"
>>> 	+#include "git-shared-util.h"
>>> 	 #include "xmacros.h"
>>> 	 #include "xdiff.h"
>>> 	 #include "xtypes.h"
>>> If you then build it and run e.g.:
>>> 	gdb -args ./libgit2_tests -smerge::files
>>> You'll get stack traces like this if you break on stdalloc__malloc
>>> (which it uses for me in its default configuration):
>>> 	
>>> 	(gdb) bt
>>> 	#0  stdalloc__malloc (len=144, file=0x87478d "/home/avar/g/libgit2/src/libgit2/xdiff/xutils.c", line=101) at /home/avar/g/libgit2/src/util/allocators/stdalloc.c:14
>>> 	#1  0x00000000006ec15c in xdl_cha_alloc (cha=0x7fffffffcaa8) at /home/avar/g/libgit2/src/libgit2/xdiff/xutils.c:101
>>> 	#2  0x00000000006eaee9 in xdl_prepare_ctx (pass=1, mf=0x7fffffffcc98, narec=13, xpp=0x7fffffffcca8, cf=0x7fffffffc7d0, xdf=0x7fffffffcaa8)
>>> 	    at /home/avar/g/libgit2/src/libgit2/xdiff/xprepare.c:194
>>> IOW this was all seamlessly routed to your git__malloc() without us
>>> having to maintain an xdl_{malloc,free}().
>>
>> Thanks for showing this, it is really helpful to see a concrete
>> example. I was especially interested to see how you went about the
>> conversion without redefining standard library functions or
>> introducing non-namespaced identifiers in files that included
>> xdiff.h. Unfortunately you have not done that and so I don't think the
>> approach above is viable   for a well behaved library.
> 
> Why? Because there's some header where doing e.g.:
> 
> 	#include "git2/something.h"
> 
> Will directly include git-xdiff.h by proxy into the user's program?

No because you're redefining malloc() and introducing ALLOC_GROW() etc 
in 
src/libgit2/{Blame_git.c,Diff_xdiff.c,Merge_file.c,Patch_generate.c,Checkout.c} 
and
Test/libgit2/merge/files.c. It is not expected that including xdiff.h 
will change a bunch of symbols in the file it is included in.

Best Wishes

Phillip

> 
> I admit I didn't check that, and assumed that these were only included
> by other *.c files in libgit2 itself. I.e. it would compile xdiff for
> its own use, but then expose its own API.
> 
> Anyway, if it is directly included in some user-exposed *.h file then
> yes, you don't want to overwrite their "malloc", but that's a small
> matter of doing an "#undef malloc" at the end (which as the below
> linked-to patch shows we'd support by having macros like
> SHA1DC_CUSTOM_TRAILING_INCLUDE_UBC_CHECK_C).
> 
> Aside from what I'm proposing here doing such "undef at the end" seems
> like a good idea in any case, because if there is any macro leakage here
> you're e.g. re-defining "XDL_BUG" and other things that aren't clearly
> in the libgit2 namespace now, no?
> 
>>> Now, I think that's obviously worth adjusting, e.g. the series I've got
>>> here could make this easier for libgit2 by including st_mult() at least,
>>> I'm not sure what you want to do about regexec_buf().
>>> For the monkeypatching you do now of creating a "git-xdiff.h" I
>>> think it
>>> would be very good to get a patch like what I managed to get
>>> sha1collisiondetection.git to accept for our own use-case, which allows
>>> us to use their upstream code as-is from a submodule:
>>> 	https://github.com/cr-marcstevens/sha1collisiondetection/commit/b45fcef
>>
>> Thanks for the link, That's a really good example where all the
>> identifiers are namespaced and the public include file does not
>> pollute the code that is including it. If you come up with something
>> like that where the client code can set up same #defines for malloc,
>> calloc, realloc and free that would be a good way forward.
> 
> I don't need to come up with that, you've got the linker to do that.
> 
> I.e. for almost any "normal" use of xdiff you'd simply compile it with
> its reference to malloc(), and then you either link that library that
> already links to libc into your program.
> 
> So if you use a custom malloc the xdiff code might still use libc
> malloc, or you'd link the two together and link the resulting program
> with your own custom malloc.
> 
> As explained in the previous & linked-to threads this is how almost
> everyone would include & use such a library, and indeed that's how git
> itself can use malloc() and free() in its sources, but have options to
> link to libc malloc, nedmalloc etc.
> 
> But instead of using the linker to resolve "malloc" to git__malloc you'd
> like to effectively include the upstream *.[ch] files directly, and
> pretend as though the upstream source used git__malloc() or whatever to
> begin with.
> 
> I don't really understand why you can't just do that at the linker
> level, especially as doing so guards you better against namespace
> pollution. But whatever, the suggestion(s) above assume you've got a
> good reason, but show that we don't need to prefix every standard
> function just to accommodate that.
> 
> Instead we can just provide a knob to include a header of yours after
> all others have been included (which the libgit2 monkeypatches to xdiff
> already include), and have that header define "malloc" as "git__malloc",
> "BUG" as "GIT_ASSERT" etc.
> 
> And yes, if you're in turn providing an interface where others will then
> include your header that's doing that you'll need to apply some
> namespacing paranoia, namely to "#undef malloc" etc.
> 
> But none of that requires git to carry prefixed versions of standard
> functions you'd wish to replace, which the patches here show.
> 
>> I do not think we should require projects to be defining there own
>> versions of [C]ALLOC_*() and BUG() just to use xdiff.
> 
> No, I don't think so either. I.e. the idea with these patches is that we
> could bundle up xdiff/* and git-shared-util.h into one distributed
> libgit, which down the road we could even roll independent releases for
> (as upstream seems to have forever gone away).
> 
> Whereas the proposals coming out of libgit2[1] for generalizing xdiff/
> for general use seem to stop just short of the specific hacks we need to
> get it running for libgit2, but e.g. leave "xdl_malloc" defined as
> "xmalloc".
> 
> That isn't a standard library function, and therefore the first thing
> you'd need to do when using it as a library is to find out how git.git
> uses that, and copy/paste it or define your own replacement.
> 
> Whereas I think it should be the other way around, we should e.g. ship a
> shimmy BUG() that calls fprintf(stderr, ...) and abort(), but for
> advanced users such as libgit2 guard those with "ifndef" or whatever, so
> you can have it call GIT_ASSERT() and the like instead.
> 
> 1. https://lore.kernel.org/git/20220209013354.GB7@abe733c6e288/