On 4/3/21 11:17 PM, Marc Nieper-Wißkirchen wrote:
Does the comparison make any sense, by the way?
Yes, although it's needed only on unusual (and these days perhaps theoretical?) platforms where SIZE_MAX < PTRDIFF_MAX.
I hadn't noticed the issue, as the projects I contribute to (coreutils, etc.) compile with -Wno-sign-compare because gcc -Wsign-compare has too many false alarms.
I prefer to avoid casts merely to pacify GCC (as casts are too error-prone), so I installed the attached. I hope it works for you. (If not, perhaps you can use -Wno-sign-compare too....)
This underscores the fact that the xalloc module should use idx_t instead of size_t pretty much everywhere. If xrealloc's size arg were of idx_t we wouldn't need any of this hacking. I realize that replacing size_t with idx_t is an incompatible change to xalloc's API, but it's time callers started using signed instead of unsigned byte counts as that helps avoid and/or catch integer-overflow errors better. I'll add that to my list of things to do for Gnulib.
From 06931a1f5fc04cf4e3585408fa10f79a745b3099 Mon Sep 17 00:00:00 2001 From: Paul Eggert <egg...@cs.ucla.edu> Date: Mon, 5 Apr 2021 20:08:27 -0700 Subject: [PATCH] xalloc: try to pacify gcc -Wsign-compare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem reported by Marc Nieper-Wißkirchen in: https://lists.gnu.org/r/bug-gnulib/2021-04/msg00034.html * lib/xmalloc.c (xpalloc): For odd platforms where SIZE_MAX < IDX_MAX, use a tricky destination for INT_MULTIPLY_WRAPV instead of an explicit comparison to SIZE_MAX. This should be more likely to pacify gcc -Wsign-compare. --- ChangeLog | 10 ++++++++++ lib/xmalloc.c | 13 +++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 81d3aab73..f3cca1ab2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2021-04-05 Paul Eggert <egg...@cs.ucla.edu> + + xalloc: try to pacify gcc -Wsign-compare + Problem reported by Marc Nieper-Wißkirchen in: + https://lists.gnu.org/r/bug-gnulib/2021-04/msg00034.html + * lib/xmalloc.c (xpalloc): For odd platforms where SIZE_MAX < IDX_MAX, + use a tricky destination for INT_MULTIPLY_WRAPV instead of an + explicit comparison to SIZE_MAX. This should be more likely to + pacify gcc -Wsign-compare. + 2021-04-05 Marc Nieper-Wißkirchen <m...@nieper-wisskirchen.de> hamt: Fix coding errors. diff --git a/lib/xmalloc.c b/lib/xmalloc.c index faeccacc9..4a6589571 100644 --- a/lib/xmalloc.c +++ b/lib/xmalloc.c @@ -122,14 +122,23 @@ xpalloc (void *pa, idx_t *nitems, idx_t nitems_incr_min, Adjust the growth according to three constraints: NITEMS_INCR_MIN, NITEMS_MAX, and what the C language can represent safely. */ - idx_t n, nbytes; + idx_t n; if (INT_ADD_WRAPV (n0, n0 >> 1, &n)) n = IDX_MAX; if (0 <= nitems_max && nitems_max < n) n = nitems_max; + /* NBYTES is of a type suitable for holding the count of bytes in an object. + This is typically idx_t, but it should be size_t on (theoretical?) + platforms where SIZE_MAX < IDX_MAX so xpalloc does not pass + values greater than SIZE_MAX to xrealloc. */ +#if IDX_MAX <= SIZE_MAX + idx_t nbytes; +#else + size_t nbytes; +#endif idx_t adjusted_nbytes - = ((INT_MULTIPLY_WRAPV (n, item_size, &nbytes) || SIZE_MAX < nbytes) + = (INT_MULTIPLY_WRAPV (n, item_size, &nbytes) ? MIN (IDX_MAX, SIZE_MAX) : nbytes < DEFAULT_MXFAST ? DEFAULT_MXFAST : 0); if (adjusted_nbytes) -- 2.27.0
