Re: [PATCH 3/3] xdiff: introduce XDL_ALLOC_GROW()

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

On 07/07/2022 12:17, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jul 06 2022, Phillip Wood wrote:
> 
>> Hi Ævar
>>
>> On 30/06/2022 14:25, Ævar Arnfjörð Bjarmason wrote:
>>> On Thu, Jun 30 2022, Phillip Wood wrote:
>>>
>>>> On 30/06/2022 13:03, Phillip Wood wrote:
>>>>> On 30/06/2022 11:54, Ævar Arnfjörð Bjarmason wrote:
>>>>>>
>>>>>> On Wed, Jun 29 2022, Phillip Wood via GitGitGadget wrote:
>>>>>>
>>>>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>>>
>>>>>>> Add a helper to grow an array. This is analogous to ALLOC_GROW() in
>>>>>>> the rest of the codebase but returns −1 on allocation failure to
>>>>>>> accommodate other users of libxdiff such as libgit2.
>>>>>>
>>>>>> Urm, does it? I just skimmed this, so maybe I missed something, but I
>>>>>> don't see where you changed the definition of xdl_malloc(),
>>>>>> xdl_realloc() etc.
>>>>
>>>> Oh I think I might have misunderstood your question. For git.git it
>>>> will still die() but for other users that arrange for xdl_realloc() to
>>>> return NULL on failure it will return -1. The same applies to the
>>>> comments in the previous two patches about XDL_[CM]ALLOC_ARRAY()
>>>> returning NULL on allocation failure.
>>> Yes, I meant that the "but returns −1 on allocation failure to
>>> accommodate other users of libxdiff such as libgit2" is really more of
>>> a:
>>> 	...but *allows for* dropping in another xmalloc(), xrealloc()
>>> 	etc. implementation that doesn't die on failure.
>>> So I think the rest of my upthread question still stands, i.e.:
>>> 	"So if that's the plan why would we need an XDL_ALLOC_ARRAY(),
>>> 	can't you just check that it [I mean ALLOC_ARRAY()] returns
>>> 	non-NULL?"
>>> I.e. if the plan is to replace the underlying x*() functions with
>>> non-fatal variants can't you use ALLOC_ARRAY() instead? I haven't tried
>>> that, but I don't see a reason we couldn't do that in principle...
>>
>> As the cover letter says, the aim of this series is not to replace
>> xmalloc() etc with non-fatal variants, it is just to make the xdiff
>> code more readable.
> 
> I don't think it's more readable to carry code in-tree that's
> unreachable except when combined with code out-of-tree. I.e. this series
> leaves us with the equivalent of:
> 
> 	ptr = xmalloc(...);
>          if (!ptr)
> 		/* unreachable in git.git ... */
> 
> I don't think it's more readable to have code that rather trivial
> analysis will show goes against the "__attribute__((noreturn))" we're
> placing on our die() function.

We're already in this situation. The code in xdiff is written to handle 
allocation failures and we use an allocation function that dies instead. 
This patch series does nothing to alter that situation.

> Which is what I'm pointing out with "running with scissors". I.e. I'm
> fully on-board with the end goal, but that can be accomplished in a way
> that doesn't confuse humans & analyzers alike.
> 
>> (One can already use a non-fatal allocation
>> function for xdl_malloc()) [...]
> 
> That just seems inviting a segfault or undefined/untested behavior
> (whether in the sense of "undefined by C" or "untested by git.git's
> codebase logic"). Everything around xmalloc() now assumes "never returns
> NULL", and you want to:
> 
>   * Make it return NULL when combined with out-of-tree-code

No I do not want to alter the behavior of xmalloc() at all, that is why 
this series does not alter the behavior of xmalloc()

>   * Maintain the code in git.git, where it never returns NULL, but in a
>     way where we won't have bugs when combined with a new macro that
>     behaves differently, in a way we never even test ourselves.

That describes the current situation with xdiff, this series does not 
alter that.

> Isn't that correct, or am I missing something?

You should note that libgit2 uses malloc() as it's default allocator, 
seeming without issue.

>> I don't think that using ALLOC_ARRAY() in
>> xdiff is helpful for other users as they would have to define their
>> own array allocation macros, rather than just providing their own
>> allocation functions. I would like to reduce the friction others have
>> upstreaming xdiff patches to us, not increase it.
> 
> Yes, I'm totally on-board with reducing the friction in others using
> xdiff, and would like to see more of that sort of out-of-tree use in
> general (although for things outside of xdiff GPL v.s. LGPL concerns
> come into play).
> 
> I'd even like for us to explicitly make that much easier. I.e. if you
> want to use xdiff now you search for it, and find the at this point
> unmaintained upstream, and if you find that git has a "newer" version
> you'll have some trouble extracting it already.
> 
> After this series you'll need to start writing & maintaining your own
> non-trivial alloc wrapper logic if you're doing that. If you get it
> subtly wrong you'll have a buggy xdiff, and most likely users will just
> copy/paste the git.git version from our git-compat-util.h & cache.h,
> which is rather silly.

This series does not alter what wrappers you need to write whereas your 
suggestion of using ALLOC_ARRAY() would force more work on potential 
xdiff users (though below I think you're suggesting we provide them in a 
separate header so they can be reused more easily).

> Which is why I'm saying we could/should do this in a much easier way,
> i.e.:
> 
>   * Factor out the now only-fatal-on-NULL ALLOC_GROW() such that we can
>     have a non-fatal version (ALLOC_GROW) and a non-fatal one.
> 
>     I don't know what we'd call it. we usually have X*() meaning "fatal",
>     but these are fatal by default, maybe G* for gently? So
>     GALLOC_GROW().  Urgh, anyway, continuing with that ugly name...

Further proof that naming is hard...

>   * Have xdiff/ use that GALLOC_GROW() & malloc(), not ALLOC_GROW() &
>     xmalloc(), as we really want to have the appropriate code flow
>     analysis etc. spot for us that we should handle NULL returns,
>     otherwise combining this code with libgit2 will be buggy/broken.
> 
> This makes it much easier for libgit2 to use this, as it won't need to
> do anything special at all. Since our GALLOC_GROW() will eventualy use
> malloc() instead of xmalloc() you don't need to define anything that
> re-implements the GALLOC_GROW() or whatever other non-fatal versions of
> our only-fatal helpers we have.
> 
> This assumes that we'd move these macros out of git-compat-util.h and a
> new git-exernal-compat.h, or that instead of *just* copying the xdiff/
> directory your import script would need to run some small bit of cc -E
> and or perl/sed to one-off extract the smell bits of
> git-exernal-compat.h or cache.h that we need.

I think there is an argument that we should change our xdiff wrapper to 
use malloc() rather than xmalloc() so we're able to test the error 
handling. That then begs the question as to how we actually get the 
allocation functions to fail when they're being tested. I also think 
that is an orthogonal change that could happen with or without this 
patch series.

Best Wishes

Phillip