On Wed, Jul 13 2022, Phillip Wood wrote:

Hi Ævar

On 11/07/2022 11:48, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Jul 11 2022, Phillip Wood wrote:

quoted

Hi Ævar

On 08/07/2022 15:20, Ævar Arnfjörð Bjarmason wrote:

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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/ (local)