| Message ID | patch-5.7-3665576576f-20220708T140354Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xdiff: use standard alloc macros, share them via git-shared-util.h | expand |
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. This change also removes the overflow checks that are present in XDL_ALLOC_GROW() and fails to free the old allocation when realloc() fails. It is not a like for like replacement. Best Wishes Phillip > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > xdiff/xdiff.h | 1 - > xdiff/xmacros.h | 11 ----------- > xdiff/xprepare.c | 8 +++++--- > xdiff/xutils.c | 17 ----------------- > xdiff/xutils.h | 2 -- > 5 files changed, 5 insertions(+), 34 deletions(-) > > diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h > index 72e25a9ffa5..832cf9d977e 100644 > --- a/xdiff/xdiff.h > +++ b/xdiff/xdiff.h > @@ -121,7 +121,6 @@ typedef struct s_bdiffparam { > > #define xdl_malloc(x) xmalloc(x) > #define xdl_free(ptr) free(ptr) > -#define xdl_realloc(ptr,x) xrealloc(ptr,x) > > void *xdl_mmfile_first(mmfile_t *mmf, long *size); > long xdl_mmfile_size(mmfile_t *mmf); > diff --git a/xdiff/xmacros.h b/xdiff/xmacros.h > index 75506bdf17e..6a6b3057375 100644 > --- a/xdiff/xmacros.h > +++ b/xdiff/xmacros.h > @@ -48,15 +48,4 @@ do { \ > (v) = (unsigned long) __p[0] | ((unsigned long) __p[1]) << 8 | \ > ((unsigned long) __p[2]) << 16 | ((unsigned long) __p[3]) << 24; \ > } while (0) > - > -/* > - * Ensure array p can accommodate at least nr elements, growing the > - * array and updating alloc (which is the number of allocated > - * elements) as necessary. Frees p and returns -1 on failure, returns > - * 0 on success > - */ > -#define XDL_ALLOC_GROW(p, nr, alloc) \ > - (-!((nr) <= (alloc) || \ > - ((p) = xdl_alloc_grow_helper((p), (nr), &(alloc), sizeof(*(p)))))) > - > #endif /* #if !defined(XMACROS_H) */ > diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c > index d6cbee32a2a..4182d9e1c0a 100644 > --- a/xdiff/xprepare.c > +++ b/xdiff/xprepare.c > @@ -128,8 +128,9 @@ static int xdl_classify_record(unsigned int pass, xdlclassifier_t *cf, xrecord_t > return -1; > } > rcrec->idx = cf->count++; > - if (XDL_ALLOC_GROW(cf->rcrecs, cf->count, cf->alloc)) > - return -1; > + GALLOC_GROW(cf->rcrecs, cf->count, cf->alloc); > + if (!cf->rcrecs) > + return -1; > cf->rcrecs[rcrec->idx] = rcrec; > rcrec->line = line; > rcrec->size = rec->size; > @@ -187,7 +188,8 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_ > for (top = blk + bsize; cur < top; ) { > prev = cur; > hav = xdl_hash_record(&cur, top, xpp->flags); > - if (XDL_ALLOC_GROW(recs, nrec + 1, narec)) > + GALLOC_GROW(recs, nrec + 1, narec); > + if (!recs) > goto abort; > if (!(crec = xdl_cha_alloc(&xdf->rcha))) > goto abort; > diff --git a/xdiff/xutils.c b/xdiff/xutils.c > index a6f10353cff..c0cd5338c4e 100644 > --- a/xdiff/xutils.c > +++ b/xdiff/xutils.c > @@ -436,20 +436,3 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp, > > return status; > } > - > -void* xdl_alloc_grow_helper(void *p, long nr, long *alloc, size_t size) > -{ > - void *tmp = NULL; > - size_t n = ((LONG_MAX - 16) / 2 >= *alloc) ? 2 * *alloc + 16 : LONG_MAX; > - if (nr > n) > - n = nr; > - if (SIZE_MAX / size >= n) > - tmp = xdl_realloc(p, n * size); > - if (tmp) { > - *alloc = n; > - } else { > - xdl_free(p); > - *alloc = 0; > - } > - return tmp; > -} > diff --git a/xdiff/xutils.h b/xdiff/xutils.h > index fd0bba94e8b..7ae3f897bef 100644 > --- a/xdiff/xutils.h > +++ b/xdiff/xutils.h > @@ -42,7 +42,5 @@ int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2, > int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp, > int line1, int count1, int line2, int count2); > > -/* Do not call this function, use XDL_ALLOC_GROW instead */ > -void* xdl_alloc_grow_helper(void* p, long nr, long* alloc, size_t size); > > #endif /* #if !defined(XUTILS_H) */
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. > 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? 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? 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(). > and fails to free the old allocation when > realloc() fails. It is not a like for like replacement. Yes, we should have a free() there. Wel spotted. But again, doing that as part of the "gently" branch seems preferrable to have duplicate versions for expression (non-fatal) v.s. statement (fatal) variants.
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. Best Wishes Phillip >> and fails to free the old allocation when >> realloc() fails. It is not a like for like replacement. > > Yes, we should have a free() there. Wel spotted. But again, doing that > as part of the "gently" branch seems preferrable to have duplicate > versions for expression (non-fatal) v.s. statement (fatal) variants.
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/
On 13/07/2022 11:48, Ævar Arnfjörð Bjarmason wrote: > > 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 > 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
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h index 72e25a9ffa5..832cf9d977e 100644 --- a/xdiff/xdiff.h +++ b/xdiff/xdiff.h @@ -121,7 +121,6 @@ typedef struct s_bdiffparam { #define xdl_malloc(x) xmalloc(x) #define xdl_free(ptr) free(ptr) -#define xdl_realloc(ptr,x) xrealloc(ptr,x) void *xdl_mmfile_first(mmfile_t *mmf, long *size); long xdl_mmfile_size(mmfile_t *mmf); diff --git a/xdiff/xmacros.h b/xdiff/xmacros.h index 75506bdf17e..6a6b3057375 100644 --- a/xdiff/xmacros.h +++ b/xdiff/xmacros.h @@ -48,15 +48,4 @@ do { \ (v) = (unsigned long) __p[0] | ((unsigned long) __p[1]) << 8 | \ ((unsigned long) __p[2]) << 16 | ((unsigned long) __p[3]) << 24; \ } while (0) - -/* - * Ensure array p can accommodate at least nr elements, growing the - * array and updating alloc (which is the number of allocated - * elements) as necessary. Frees p and returns -1 on failure, returns - * 0 on success - */ -#define XDL_ALLOC_GROW(p, nr, alloc) \ - (-!((nr) <= (alloc) || \ - ((p) = xdl_alloc_grow_helper((p), (nr), &(alloc), sizeof(*(p)))))) - #endif /* #if !defined(XMACROS_H) */ diff --git a/xdiff/xprepare.c b/xdiff/xprepare.c index d6cbee32a2a..4182d9e1c0a 100644 --- a/xdiff/xprepare.c +++ b/xdiff/xprepare.c @@ -128,8 +128,9 @@ static int xdl_classify_record(unsigned int pass, xdlclassifier_t *cf, xrecord_t return -1; } rcrec->idx = cf->count++; - if (XDL_ALLOC_GROW(cf->rcrecs, cf->count, cf->alloc)) - return -1; + GALLOC_GROW(cf->rcrecs, cf->count, cf->alloc); + if (!cf->rcrecs) + return -1; cf->rcrecs[rcrec->idx] = rcrec; rcrec->line = line; rcrec->size = rec->size; @@ -187,7 +188,8 @@ static int xdl_prepare_ctx(unsigned int pass, mmfile_t *mf, long narec, xpparam_ for (top = blk + bsize; cur < top; ) { prev = cur; hav = xdl_hash_record(&cur, top, xpp->flags); - if (XDL_ALLOC_GROW(recs, nrec + 1, narec)) + GALLOC_GROW(recs, nrec + 1, narec); + if (!recs) goto abort; if (!(crec = xdl_cha_alloc(&xdf->rcha))) goto abort; diff --git a/xdiff/xutils.c b/xdiff/xutils.c index a6f10353cff..c0cd5338c4e 100644 --- a/xdiff/xutils.c +++ b/xdiff/xutils.c @@ -436,20 +436,3 @@ int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp, return status; } - -void* xdl_alloc_grow_helper(void *p, long nr, long *alloc, size_t size) -{ - void *tmp = NULL; - size_t n = ((LONG_MAX - 16) / 2 >= *alloc) ? 2 * *alloc + 16 : LONG_MAX; - if (nr > n) - n = nr; - if (SIZE_MAX / size >= n) - tmp = xdl_realloc(p, n * size); - if (tmp) { - *alloc = n; - } else { - xdl_free(p); - *alloc = 0; - } - return tmp; -} diff --git a/xdiff/xutils.h b/xdiff/xutils.h index fd0bba94e8b..7ae3f897bef 100644 --- a/xdiff/xutils.h +++ b/xdiff/xutils.h @@ -42,7 +42,5 @@ int xdl_emit_hunk_hdr(long s1, long c1, long s2, long c2, int xdl_fall_back_diff(xdfenv_t *diff_env, xpparam_t const *xpp, int line1, int count1, int line2, int count2); -/* Do not call this function, use XDL_ALLOC_GROW instead */ -void* xdl_alloc_grow_helper(void* p, long nr, long* alloc, size_t size); #endif /* #if !defined(XUTILS_H) */
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. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- xdiff/xdiff.h | 1 - xdiff/xmacros.h | 11 ----------- xdiff/xprepare.c | 8 +++++--- xdiff/xutils.c | 17 ----------------- xdiff/xutils.h | 2 -- 5 files changed, 5 insertions(+), 34 deletions(-)