New function xpalloc in module xalloc

New function xpalloc in module xalloc

[Marc Nieper-Wißkirchen]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

GCC prints the following warning when compiling the new code:

lib/xmalloc.c: In function 'xpalloc':
lib/xmalloc.c:132:64: warning: comparison of integer expressions of
different signedness: 'long unsigned int' and 'idx_t' {aka 'long int'}
[-Wsign-compare]
  132 |     = ((INT_MULTIPLY_WRAPV (n, item_size, &nbytes) || SIZE_MAX <
nbytes)

If I understand the error message correctly, it is because 'nbytes' is a
signed type while SIZE_MAX is unsigned.

Does the comparison make any sense, by the way?

Thanks for investigating,

Marc

[-- Attachment #2: Type: text/html, Size: 1323 bytes --]

Re: New function xpalloc in module xalloc

[Jeffrey Walton]

On Sun, Apr 4, 2021 at 2:17 AM Marc Nieper-Wißkirchen
<marc.nieper+gnu@gmail.com> wrote:
>
> GCC prints the following warning when compiling the new code:
>
> lib/xmalloc.c: In function 'xpalloc':
> lib/xmalloc.c:132:64: warning: comparison of integer expressions of different signedness: 'long unsigned int' and 'idx_t' {aka 'long int'} [-Wsign-compare]
>   132 |     = ((INT_MULTIPLY_WRAPV (n, item_size, &nbytes) || SIZE_MAX < nbytes)
>
> If I understand the error message correctly, it is because 'nbytes' is a signed type while SIZE_MAX is unsigned.
>
> Does the comparison make any sense, by the way?

Only for positive integers.

If idx_t is negative, like -1, then -1 will be promoted to an unsigned
type and then -1 > 0.

Jeff

Re: New function xpalloc in module xalloc

[Marc Nieper-Wißkirchen]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

SIZE_MAX could be defined as -1 promoted to an unsigned type, meaning that
the unsigned comparison would always yield false. Or am I missing something?

In any case, if the promotion is what is intended, 'nbytes' should be
probably replaced with '(size_t) nbytes' to silence the warning and to make
it explicit.


Am So., 4. Apr. 2021 um 08:24 Uhr schrieb Jeffrey Walton <noloader@gmail.com
>:

> On Sun, Apr 4, 2021 at 2:17 AM Marc Nieper-Wißkirchen
> <marc.nieper+gnu@gmail.com> wrote:
> >
> > GCC prints the following warning when compiling the new code:
> >
> > lib/xmalloc.c: In function 'xpalloc':
> > lib/xmalloc.c:132:64: warning: comparison of integer expressions of
> different signedness: 'long unsigned int' and 'idx_t' {aka 'long int'}
> [-Wsign-compare]
> >   132 |     = ((INT_MULTIPLY_WRAPV (n, item_size, &nbytes) || SIZE_MAX <
> nbytes)
> >
> > If I understand the error message correctly, it is because 'nbytes' is a
> signed type while SIZE_MAX is unsigned.
> >
> > Does the comparison make any sense, by the way?
>
> Only for positive integers.
>
> If idx_t is negative, like -1, then -1 will be promoted to an unsigned
> type and then -1 > 0.
>
> Jeff
>

[-- Attachment #2: Type: text/html, Size: 1934 bytes --]

Re: New function xpalloc in module xalloc

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

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.

[-- Attachment #2: 0001-xalloc-try-to-pacify-gcc-Wsign-compare.patch --]
[-- Type: text/x-patch, Size: 2582 bytes --]

From 06931a1f5fc04cf4e3585408fa10f79a745b3099 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@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  <eggert@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  <marc@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

Re: New function xpalloc in module xalloc

[Marc Nieper-Wißkirchen]

[-- Attachment #1: Type: text/plain, Size: 1898 bytes --]

Hi Paul,

Am Di., 6. Apr. 2021 um 05:19 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:

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

Ah, okay. I didn't think of this possibility.


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

The fix works for me. Thank you very much! IMO, it's much better than
asking to compile with "-Wno-sign-compare' because this can (like type
casts) silence other, non-false positive warnings.

Speaking of type casts, I don't think they would have been bad here because
they would document exactly what was going on here. By writing

SIZE_MAX < (uintmax_t) nbytes

the otherwise implicit type conversion would have been made explicit and
using 'uintmax_t' also documents that it is expected that the width of
'nbytes' can be greater than the width of 'size_t'.


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

The philosophy about idx_t could be worth an entry in the manual.

Marc

[-- Attachment #2: Type: text/html, Size: 3434 bytes --]

Re: New function xpalloc in module xalloc

[Paul Eggert]

On 4/5/21 11:48 PM, Marc Nieper-Wißkirchen wrote:
> SIZE_MAX < (uintmax_t) nbytes

Eeuuw! That's a cure worse than the disease. Among other things, there 
is no guarantee that PTRDIFF_MAX <= UINTMAX_MAX so in theory the 
comparison could go completely awry with a sufficiently-large NBYTES.

"Don't use casts." Words to live by.

Re: New function xpalloc in module xalloc

[Marc Nieper-Wißkirchen]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

Am Di., 6. Apr. 2021 um 21:05 Uhr schrieb Paul Eggert <eggert@cs.ucla.edu>:

> On 4/5/21 11:48 PM, Marc Nieper-Wißkirchen wrote:
> > SIZE_MAX < (uintmax_t) nbytes
>
> Eeuuw! That's a cure worse than the disease. Among other things, there
> is no guarantee that PTRDIFF_MAX <= UINTMAX_MAX so in theory the
> comparison could go completely awry with a sufficiently-large NBYTES.
>

I checked the ISO C18 standard before I wrote this. :) In 6.2.5 it says
that for every signed type there is an unsigned type of the same width.
Given the definitions of INTMAX_MAX and UINTMAX_MAX, I then concluded

PTRDIFF_MAX <= INTMAX_MAX <= UINTMAX_MAX.

Where is the flaw in my reasoning?

[-- Attachment #2: Type: text/html, Size: 1404 bytes --]

Re: New function xpalloc in module xalloc

[Paul Eggert]

On 4/6/21 12:23 PM, Marc Nieper-Wißkirchen wrote:
> Where is the flaw in my reasoning?

Oh, you're right; any nonnegative signed integer value will fit into 
uintmax_t. (Perhaps this wasn't always true in older standards, but it's 
true of the recent C standard.)

So that cast should work. Still, I wouldn't use a cast; I'd assign the 
integer to an uintmax_t local value, and use that.

And anyway, for this particular case widening to uintmax_t is not 
necessary; it complicates the code and can make it less efficient on 
some platforms by forcing an unnecessary conversion at runtime. So let's 
not do that.