Re: [PATCH 5/7] xdiff: use GALLOC_GROW(), not XDL_ALLOC_GROW()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 13 2022, Phillip Wood wrote:

> Hi Ævar
>
> On 11/07/2022 11:48, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Jul 11 2022, Phillip Wood wrote:
>> 
>>> Hi Ævar
>>>
>>> On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote:
>>>> Replace the recently introduced XDL_ALLOC_GROW() with invocations of
>>>> the GALLOC_GROW() from git-shared-util.h.
>>>> As this change shows the macro + function indirection of
>>>> XDL_ALLOC_GROW() is something we needed only because the two callsites
>>>> we used it in wanted to use it as an expression, and we thus had to
>>>> pass the "sizeof" down.
>>>> Let's just check the value afterwards instead, which allows us to
>>>> use
>>>> the shared macro, we can also remove xdl_reallo(), this was its last
>>>> user.
>>>
>>> I don't think this expression->statement change is an
>>> improvement.
>> I think the use-as-statement is prettier too, but I think the
>> uglyness
>> of having to pass down the sizeof() & re-implementing the macro version
>> of the alloc-or-die variant outweights that.
>
> I think this is partly a choice between prioritizing ease of
> implementation or ease of use for callers.
>
>>> This change also removes the overflow checks that are
>>> present in XDL_ALLOC_GROW()[...]
>> We end up calling st_mult(), which does that overflow check. Do you
>> mean
>> that the POC shimmy layer I showed in another reply for libgit2 doesn't
>> have an st_mult() that detects overflows?
>
> I was referring to
>
> #define alloc_nr(x) (((x)+16)*3/2)
>
> in cache.h. XDL_ALLOC_GROW() detects overflows when growing the number
> of items as well as when calculating the number of bytes to allocate.
>
>> That's true, but as noted downthread of that we can & could ship that as
>> part of the shimmy layer, but that's unrelated to this change.
>> In your pre-image you use LONG_MAX instead of UINTMAX_MAX & I don't
>> see
>> (but maybe I haven't looked at it carefully enough) how it does the same
>> dying on overflows. Doesn't it just fall back to LONG_MAX?
>
> It does not die on overflow as we want to return errors rather than
> die in the xdiff code. It uses long to match the existing code.
>
>> Part of this is that it's not clear to me from your commit(s) why you
>> need to rewrite alloc_nr() and rewrite (or drop?) st_mult().
>
> So that we don't die on overflow and so that the xdiff code is self
> contained.
>
> I'm a bit disappointed that this patch seems to have been written
> without really taking the time to understand exactly what the code it
> is replacing is doing.

Well, there's a lot to understand :) So it's also an implicit comment on
the complexity of your series.

In case it wasn't clear the main thrust of what I've been commenting on
here is asking why what you have here needs to *structurally* look the
way it does, i.e. why you think the malloc() & free() naming can't
resolve to libgit2's wrappers (per the thread ending at [1]).

And, if we can't end up with an xdiff/* in our tree that doesn't have
return value checking flying in the face of xmalloc's NORETURN()
behavior on allocation failures.

But yes, the suggested replacement isn't behaving exactly as yours does,
but I think that's just an implementation detail as far as the stuctural
questions above go. I.e.:

 * You're trying to fix a long-standing overflow issue in alloc_nr(),
   but in such a way that only leaves xdiff/* with the fix.

   Can't we address that seperately (e.g. turning alloc_nr into a static
   inline function using the st_* helper), and then make xdiff and
   cache.h use that new shared helper?

 * You seem to be set on the idea that you absolutely must be rewriting
   large parts of the existing allocation macro, because you'd really
   like to use it as an expression v.s. a statement.

   I really disagree with that trade-off, i.e. the whole endeavor of
   duplicating the implementation in ways that are mostly the same but
   not quite (e.g. that alloc_grow case) doesn't seem worth it
   v.s. sharing the behavior.

   But likewise it seems to be an implementation detail of your series
   that we can't have both, i.e. if you're set on using these as an
   expression factoring the shared behavior in cache.h out into some
   static inline functions, then calling those from both variants.

1. https://lore.kernel.org/git/220711.865yk47x54.gmgdl@evledraar.gmail.com/