Re: [PATCH] byteswap: port better to limited platforms

[Bruno Haible]

Paul Eggert wrote:
> > $ gcc -Wall -S foo.c
> > foo.c:1: warning: integer constant is too large for ‘long’ type
> 
> I don't see how that would produce incorrect code for byteswap.h.

Indeed, gcc 4.4.7 warns but produces a long long value (no truncation).

> The longer story is C89 lacked 'long long'. Also, in C89 on a 32-bit int 
> platform the decimal integer literal 2147483648 had type 'unsigned int' 
> because 2147483648 fits in 'unsigned int' but not plain 'int'. Although 
> this misfeature was fixed in C99 it was present in many compilers for 
> some time after that. (It still seems to be present in MS Visual Studio! 
> See 
> .)
> 
> To help write portable code, GCC formerly warned about integer constants 
> that might not be portable to C89, but it then went ahead and produced 
> correct code.

Thanks for explaining. I remember that at least in the preprocessor,
truncation did occur. I don't remember when in the object code the
compilers (other than MSVC) stopped doing the truncation.

> The need for these warnings went away long ago (except perhaps for 
> Microsoft...) but they can be annoying so I installed the attached patch 
> to pacify older GCCs.

Thanks!

Bruno

Re: [PATCH] byteswap: port better to limited platforms

[Paul Eggert]

On 2024-05-17 16:51, Bruno Haible wrote:

-- foo.c --
unsigned long long x = 0xff00;
---

$ gcc -Wall -S foo.c
foo.c:1: warning: integer constant is too large for ‘long’ type


I don't see how that would produce incorrect code for byteswap.h.

If memory serves, GCC formerly warned about integer constants that had a 
"surprising" type, for a definition of "surprising" that was too 
generous so that there were too many false alarms.


The longer story is C89 lacked 'long long'. Also, in C89 on a 32-bit int 
platform the decimal integer literal 2147483648 had type 'unsigned int' 
because 2147483648 fits in 'unsigned int' but not plain 'int'. Although 
this misfeature was fixed in C99 it was present in many compilers for 
some time after that. (It still seems to be present in MS Visual Studio! 
See 
.)


To help write portable code, GCC formerly warned about integer constants 
that might not be portable to C89, but it then went ahead and produced 
correct code.


The need for these warnings went away long ago (except perhaps for 
Microsoft...) but they can be annoying so I installed the attached patch 
to pacify older GCCs.



PS. We could simplify the code by removing all uses of __builtin_bswap64 
etc. Although these uses are present only to improve performance, they 
do not improve performance with GCC 14 x86-64 with -O2.From 221d026428e20d18faf96b0e38b25f4d89d32805 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sat, 18 May 2024 07:48:47 -0700
Subject: [PATCH] byteswap: pacify GCC 4.4.7 and older

Problem reported by Bruno Haible in:
https://lists.gnu.org/r/bug-gnulib/2024-05/msg00277.html
* lib/byteswap.in.h (bswap_16, bswap_32, bswap_64):
Compute the mask rather than using long constants like
0xff00 that may generate bogus warnings.
---
 ChangeLog |  9 +
 lib/byteswap.in.h | 31 +--
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 29adf56ab7..79f813e1b2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2024-05-18  Paul Eggert  
+
+	byteswap: pacify GCC 4.4.7 and older
+	Problem reported by Bruno Haible in:
+	https://lists.gnu.org/r/bug-gnulib/2024-05/msg00277.html
+	* lib/byteswap.in.h (bswap_16, bswap_32, bswap_64):
+	Compute the mask rather than using long constants like
+	0xff00 that may generate bogus warnings.
+
 2024-05-18  Bruno Haible  
 
 	endian tests: Verify that it can be used from C++.
diff --git a/lib/byteswap.in.h b/lib/byteswap.in.h
index 0e760005c2..4be335d915 100644
--- a/lib/byteswap.in.h
+++ b/lib/byteswap.in.h
@@ -62,8 +62,9 @@ bswap_16 (uint_least16_t x)
 #ifdef _GL_BYTESWAP_HAS_BUILTIN_BSWAP16
   return __builtin_bswap16 (x);
 #else
-  return (  (x & 0xff00) >> 8
-  | (x & 0x00ff) << 8);
+  uint_fast16_t mask = 0xff;
+  return (  (x & mask << 8 * 1) >> 8 * 1
+  | (x & mask << 8 * 0) << 8 * 1);
 #endif
 }
 
@@ -75,10 +76,11 @@ bswap_32 (uint_least32_t x)
 #ifdef _GL_BYTESWAP_HAS_BUILTIN_BSWAP32
   return __builtin_bswap32 (x);
 #else
-  return (  (x & 0xff00) >> 24
-  | (x & 0x00ff) >>  8
-  | (x & 0xff00) <<  8
-  | (x & 0x00ff) << 24);
+  uint_fast32_t mask = 0xff;
+  return (  (x & mask << 8 * 3) >> 8 * 3
+  | (x & mask << 8 * 2) >> 8 * 1
+  | (x & mask << 8 * 1) << 8 * 1
+  | (x & mask << 8 * 0) << 8 * 3);
 #endif
 }
 
@@ -91,14 +93,15 @@ bswap_64 (uint_least64_t x)
 # ifdef _GL_BYTESWAP_HAS_BUILTIN_BSWAP64
   return __builtin_bswap64 (x);
 # else
-  return (  (x & 0xff00) >> 56
-  | (x & 0x00ff) >> 40
-  | (x & 0xff00) >> 24
-  | (x & 0x00ff) >>  8
-  | (x & 0xff00) <<  8
-  | (x & 0x00ff) << 24
-  | (x & 0xff00) << 40
-  | (x & 0x00ff) << 56);
+  uint_fast64_t mask = 0xff;
+  return (  (x & mask << 8 * 7) >> 8 * 7
+  | (x & mask << 8 * 6) >> 8 * 5
+  | (x & mask << 8 * 5) >> 8 * 3
+  | (x & mask << 8 * 4) >> 8 * 1
+  | (x & mask << 8 * 3) << 8 * 1
+  | (x & mask << 8 * 2) << 8 * 3
+  | (x & mask << 8 * 1) << 8 * 5
+  | (x & mask << 8 * 0) << 8 * 7);
 # endif
 }
 #endif
-- 
2.40.1

Re: [PATCH] byteswap: port better to limited platforms

[Collin Funk]

On 5/17/24 3:49 PM, Paul Eggert wrote:
> POSIX does not require uint64_t, and the C standard
> does not require uint16_t or uint32_t either, so port
> to platforms that lack these types.  The POSIX limitation
> is the only significant one in practice.  I ran into this
> issue when updating Emacs, which still ports to platforms
> lacking 64-bit types.

Oops, sorry about that. I know Emacs uses this module but I didn't
realize it supported platforms without 64-bit types.

> Also, redo to avoid unnecssary parentheses, as these are now
> functions not macros.

Thanks. I started removing parentheses but it messed with Emacs
indentation. I guess c-mode probably only cares about the outer
parentheses now that I think about it.

Collin

Re: [PATCH] byteswap: port better to limited platforms

[Collin Funk]

On 5/17/24 4:51 PM, Bruno Haible wrote:
> I think this produces wrong code with gcc 4.4.7 and older. See:
> 
> -- foo.c --
> unsigned long long x = 0xff00;
> ---
> 
> $ gcc -Wall -S foo.c
> foo.c:1: warning: integer constant is too large for ‘long’ type

I think I've seen warnings like this before with newer GCC versions
but I am not sure if correct code was produced or not.

I have always disliked the need for suffixes after integer constants
in C (U, ULL, ULL, etc.).

If stdint correctly defines UINT16_C, UINT32_C, and UINT64_C then I
would just use those.

POSIX states [1]:

The macro INTN_C(value) shall expand to an integer constant
expression corresponding to the type int_least N _t. The macro
UINTN_C(value) shall expand to an integer constant expression
corresponding to the type uint_least N _t. For example, if
uint_least64_t is a name for the type unsigned long long, then
UINT64_C(0x123) might expand to the integer constant 0x123ULL.

I guess the regular U, UL, ULL suffix would work by themselves or a
cast if preferred.

Collin

[1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdint.h.html

Re: [PATCH] byteswap: port better to limited platforms

[Bruno Haible]

Paul Eggert wrote:
> -#else
> -  return x) & 0xff00ull) >> 56)
> -  | (((x) & 0x00ffull) >> 40)
> -  | (((x) & 0xff00ull) >> 24)
> -  | (((x) & 0x00ffull) >> 8)
> -  | (((x) & 0xff00ull) << 8)
> -  | (((x) & 0x00ffull) << 24)
> -  | (((x) & 0xff00ull) << 40)
> -  | (((x) & 0x00ffull) << 56));
> -#endif
> +# else
> +  return (  (x & 0xff00) >> 56
> +  | (x & 0x00ff) >> 40
> +  | (x & 0xff00) >> 24
> +  | (x & 0x00ff) >>  8
> +  | (x & 0xff00) <<  8
> +  | (x & 0x00ff) << 24
> +  | (x & 0xff00) << 40
> +  | (x & 0x00ff) << 56);
> +# endif
>  }
> +#endif

I think this produces wrong code with gcc 4.4.7 and older. See:

-- foo.c --
unsigned long long x = 0xff00;
---

$ gcc -Wall -S foo.c
foo.c:1: warning: integer constant is too large for ‘long’ type

Bruno