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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 13, 2022, 13:21 UTC
Message-ID: <24148494-72ab-23f8-58b5-c8371c44d167@gmail.com>
In-Reply-To: <220713.86edyp46ld.gmgdl@evledraar.gmail.com>

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

Show 9 quoted lines

> 
> On Wed, Jul 13 2022, Phillip Wood wrote:
>>
>> 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.

I'm surprised you think it is complex, the implementation of XDL_ALLOC_GROW() and its helper is pretty short.

> 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]).

I think the different structural approach stems in part from the subtle differences between ALLOC_GROW() and XDL_ALLOC_GROW(). Once one appreciates that the latter needs to free the original allocation on overflow and allocation failure and work with code that uses long rather than size_t then there is not much code left to be shared between them.

> 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.

I don't think xmalloc() is marked as NORETURN in wrapper.h so the compiler would need somehow determine that from looking at the implementation in wrapper.c but even if it is using LTO I'm not sure it'll have that information when creating xdiff/lib.a

Show 6 quoted lines

> 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.

I didn't set out to fix that issue per-se, I only realized it existed when I reviewed this series.

>     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?

As I've said before xdiff does not want to die on overflow so it cannot use st_mult(). Also you cannot assume that you're dealing with size_t when you do the overflow check in alloc_nr() so I think it is a tricky problem to solve.

>   * 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.

It is not just that - there are plenty of differences that stem from returning an error rather that dying that reduce the amount of common code. Having a separate definition was also driven by a desire to keep the xdiff code self contained.

This will likely be the last message from me in this thread for a while 
- I'm going offline later next week and want to make sure I have time to 
review Stolee's rebase patches before then.

Best Wishes

Phillip