I've applied the two attached patches. The first uses inline functions
for byteswap.h and improves the configure check.
I now see what Paul was saying about floating-point arguments [1].
With the following program:
===============================================
#define bswap_16(x) ((((x) & 0x00FF) << 8) | \
(((x) & 0xFF00) >> 8))
int
main (void)
{
return bswap_16 (0.0);
}
===============================================
$ gcc main.c
main.c: In function ‘main’:
main.c:2:28: error: invalid operands to binary & (have ‘double’ and ‘int’)
2 | #define bswap_16(x) ((((x) & 0x00FF) << 8) | \
| ^
main.c:7:10: note: in expansion of macro ‘bswap_16’
7 | return bswap_16 (0.0);
| ^~~~~~~~
main.c:3:28: error: invalid operands to binary & (have ‘double’ and ‘int’)
3 | (((x) & 0xFF00) >> 8))
| ^
main.c:7:10: note: in expansion of macro ‘bswap_16’
7 | return bswap_16 (0.0);
|
Using the functions from glibc's byteswap.h instead of that macro:
$ gcc main.c
$ ./a.out
$ echo $?
0
Doesn't seem like something that is done often but better to behave
more like glibc in my opinion. Using inline functions also avoids
evaluating the argument multiple times like I mentioned previously.
This matches glibc's behavior too.
I'm pretty sure the checks I added to byteswap.m4 should be good
enough. If not the test cases I added in patch 2 should catch it at
least.
[1] https://lists.gnu.org/archive/html/bug-gnulib/2024-05/msg00188.html
CollinFrom 0858943ba21449b1f8fd0c0ed8c2342cdac3c374 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Thu, 16 May 2024 20:37:14 -0700
Subject: [PATCH 1/2] byteswap: Use inline functions instead of macros.
* lib/byteswap.c: New file.
* lib/byteswap.in.h (bswap_16, bswap_32, bswap_64): Use inline functions
instead of macros.
* m4/byteswap.m4 (gl_BYTESWAP): Check that bswap functions can be used
on double values.
* modules/byteswap (Files): Add lib/byteswap.c.
(Depends-on): Add extern-inline and stdint as conditional dependencies.
(Makefile.am): Add lib/byteswap.c to lib_SOURCES.
---
ChangeLog | 12 ++++++++
lib/byteswap.c | 21 +++++++++++++
lib/byteswap.in.h | 76 +++++++++++++++++++++++++++++++++++++----------
m4/byteswap.m4 | 27 ++++++++++++++---
modules/byteswap | 4 +++
5 files changed, 121 insertions(+), 19 deletions(-)
create mode 100644 lib/byteswap.c
diff --git a/ChangeLog b/ChangeLog
index ba52fbcdf1..ddfe701dd4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2024-05-16 Collin Funk <collin.fu...@gmail.com>
+
+ byteswap: Use inline functions instead of macros.
+ * lib/byteswap.c: New file.
+ * lib/byteswap.in.h (bswap_16, bswap_32, bswap_64): Use inline functions
+ instead of macros.
+ * m4/byteswap.m4 (gl_BYTESWAP): Check that bswap functions can be used
+ on double values.
+ * modules/byteswap (Files): Add lib/byteswap.c.
+ (Depends-on): Add extern-inline and stdint as conditional dependencies.
+ (Makefile.am): Add lib/byteswap.c to lib_SOURCES.
+
2024-05-16 Collin Funk <collin.fu...@gmail.com>
gnulib-tool.py: Fix return value when exiting with Ctrl-C.
diff --git a/lib/byteswap.c b/lib/byteswap.c
new file mode 100644
index 0000000000..6e3dad74ea
--- /dev/null
+++ b/lib/byteswap.c
@@ -0,0 +1,21 @@
+/* Inline functions for <byteswap.h>.
+
+ Copyright 2024 Free Software Foundation, Inc.
+
+ This file is free software: you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as
+ published by the Free Software Foundation; either version 2.1 of the
+ License, or (at your option) any later version.
+
+ This file is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with this program. If not, see <https://www.gnu.org/licenses/>. */
+
+#include <config.h>
+
+#define _GL_BYTESWAP_INLINE _GL_EXTERN_INLINE
+#include <byteswap.h>
diff --git a/lib/byteswap.in.h b/lib/byteswap.in.h
index 8e49efad05..cfa3f04031 100644
--- a/lib/byteswap.in.h
+++ b/lib/byteswap.in.h
@@ -16,29 +16,75 @@
along with this program. If not, see <https://www.gnu.org/licenses/>. */
#ifndef _GL_BYTESWAP_H
-#define _GL_BYTESWAP_H
+#define _GL_BYTESWAP_H 1
+
+/* This file uses _GL_INLINE. */
+#if !_GL_CONFIG_H_INCLUDED
+ #error "Please include config.h first."
+#endif
+
+#include <stdint.h>
+
+_GL_INLINE_HEADER_BEGIN
+#ifndef _GL_BYTESWAP_INLINE
+# define _GL_BYTESWAP_INLINE _GL_INLINE
+#endif
+
+#ifdef __cplusplus
+extern "C" {
+#endif
/* Given an unsigned 16-bit argument X, return the value corresponding to
X with reversed byte order. */
-#define bswap_16(x) ((((x) & 0x00FF) << 8) | \
- (((x) & 0xFF00) >> 8))
+_GL_BYTESWAP_INLINE uint16_t
+bswap_16 (uint16_t x)
+{
+#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 8)) \
+ || (defined __has_builtin && __has_builtin (__builtin_bswap16))
+ return __builtin_bswap16 (x);
+#else
+ return (((((x) >> 8) & 0xff) | (((x) & 0xff) << 8)));
+#endif
+}
/* Given an unsigned 32-bit argument X, return the value corresponding to
X with reversed byte order. */
-#define bswap_32(x) ((((x) & 0x000000FF) << 24) | \
- (((x) & 0x0000FF00) << 8) | \
- (((x) & 0x00FF0000) >> 8) | \
- (((x) & 0xFF000000) >> 24))
+_GL_BYTESWAP_INLINE uint32_t
+bswap_32 (uint32_t x)
+{
+#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)) \
+ || (defined __has_builtin && __has_builtin (__builtin_bswap32))
+ return __builtin_bswap32 (x);
+#else
+ return ((((x) & 0xff000000u) >> 24) | (((x) & 0x00ff0000u) >> 8)
+ | (((x) & 0x0000ff00u) << 8) | (((x) & 0x000000ffu) << 24));
+#endif
+}
/* Given an unsigned 64-bit argument X, return the value corresponding to
X with reversed byte order. */
-#define bswap_64(x) ((((x) & 0x00000000000000FFULL) << 56) | \
- (((x) & 0x000000000000FF00ULL) << 40) | \
- (((x) & 0x0000000000FF0000ULL) << 24) | \
- (((x) & 0x00000000FF000000ULL) << 8) | \
- (((x) & 0x000000FF00000000ULL) >> 8) | \
- (((x) & 0x0000FF0000000000ULL) >> 24) | \
- (((x) & 0x00FF000000000000ULL) >> 40) | \
- (((x) & 0xFF00000000000000ULL) >> 56))
+_GL_BYTESWAP_INLINE uint64_t
+bswap_64 (uint64_t x)
+{
+#if (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)) \
+ || (defined __has_builtin && __has_builtin (__builtin_bswap64))
+ return __builtin_bswap64 (x);
+#else
+ return ((((x) & 0xff00000000000000ull) >> 56)
+ | (((x) & 0x00ff000000000000ull) >> 40)
+ | (((x) & 0x0000ff0000000000ull) >> 24)
+ | (((x) & 0x000000ff00000000ull) >> 8)
+ | (((x) & 0x00000000ff000000ull) << 8)
+ | (((x) & 0x0000000000ff0000ull) << 24)
+ | (((x) & 0x000000000000ff00ull) << 40)
+ | (((x) & 0x00000000000000ffull) << 56));
+#endif
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+_GL_INLINE_HEADER_END
#endif /* _GL_BYTESWAP_H */
diff --git a/m4/byteswap.m4 b/m4/byteswap.m4
index 0c76fe9312..3f5ef45cfe 100644
--- a/m4/byteswap.m4
+++ b/m4/byteswap.m4
@@ -1,5 +1,5 @@
# byteswap.m4
-# serial 5
+# serial 6
dnl Copyright (C) 2005, 2007, 2009-2024 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
@@ -10,9 +10,28 @@
AC_DEFUN([gl_BYTESWAP],
[
dnl Prerequisites of lib/byteswap.in.h.
- AC_CHECK_HEADERS([byteswap.h], [
+ AC_CHECK_HEADERS_ONCE([byteswap.h])
+ if test $ac_cv_header_byteswap_h = yes; then
+ AC_CACHE_CHECK([for working bswap_16, bswap_32, bswap_64],
+ [gl_cv_header_working_byteswap_h],
+ [gl_cv_header_working_byteswap_h=no
+ AC_COMPILE_IFELSE(
+ [AC_LANG_PROGRAM(
+ [[#include <byteswap.h>
+ ]],
+ [[int value_16 = bswap_16 (0.0);
+ int value_32 = bswap_32 (0.0);
+ int value_64 = bswap_64 (0.0);
+ return !(value_16 + value_32 + value_64);
+ ]])
+ ],
+ [gl_cv_header_working_byteswap_h=yes],
+ [gl_cv_header_working_byteswap_h=no])
+ ])
+ fi
+ if test $gl_cv_header_working_byteswap_h = yes; then
GL_GENERATE_BYTESWAP_H=false
- ], [
+ else
GL_GENERATE_BYTESWAP_H=true
- ])
+ fi
])
diff --git a/modules/byteswap b/modules/byteswap
index 68fe6b78ae..82fe424a61 100644
--- a/modules/byteswap
+++ b/modules/byteswap
@@ -3,10 +3,13 @@ Swap bytes of 16, 32 and 64 bit values.
Files:
lib/byteswap.in.h
+lib/byteswap.c
m4/byteswap.m4
Depends-on:
gen-header
+extern-inline [$GL_GENERATE_BYTESWAP_H]
+stdint [$GL_GENERATE_BYTESWAP_H]
configure.ac:
gl_BYTESWAP
@@ -23,6 +26,7 @@ byteswap.h: byteswap.in.h $(top_builddir)/config.status
@NMD@ $(AM_V_GEN)$(MKDIR_P) '%reldir%'
$(gl_V_at)$(SED_HEADER_TO_AT_t) $(srcdir)/byteswap.in.h
$(AM_V_at)mv $@-t $@
+lib_SOURCES += byteswap.c
else
byteswap.h: $(top_builddir)/config.status
rm -f $@
--
2.45.0
From c26bdc4c466515ee07ec5fa33b97628ea87e6cf9 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Thu, 16 May 2024 20:49:56 -0700
Subject: [PATCH 2/2] byteswap tests: Strengthen tests.
* modules/byteswap-tests (Depends-on): Add stdint.
* tests/test-byteswap.c (test_bswap_constant, test_bswap_eval_once)
(test_bswap_double): New functions.
(main): Use them.
---
ChangeLog | 8 ++++++++
modules/byteswap-tests | 1 +
tests/test-byteswap.c | 44 +++++++++++++++++++++++++++++++++++++++---
3 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index ddfe701dd4..6436ddfb81 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-05-16 Collin Funk <collin.fu...@gmail.com>
+
+ byteswap tests: Strengthen tests.
+ * modules/byteswap-tests (Depends-on): Add stdint.
+ * tests/test-byteswap.c (test_bswap_constant, test_bswap_eval_once)
+ (test_bswap_double): New functions.
+ (main): Use them.
+
2024-05-16 Collin Funk <collin.fu...@gmail.com>
byteswap: Use inline functions instead of macros.
diff --git a/modules/byteswap-tests b/modules/byteswap-tests
index be7b65947e..64baf0e404 100644
--- a/modules/byteswap-tests
+++ b/modules/byteswap-tests
@@ -3,6 +3,7 @@ tests/test-byteswap.c
tests/macros.h
Depends-on:
+stdint
configure.ac:
diff --git a/tests/test-byteswap.c b/tests/test-byteswap.c
index 85f7fd4193..60b3ea31e2 100644
--- a/tests/test-byteswap.c
+++ b/tests/test-byteswap.c
@@ -19,14 +19,52 @@
#include <config.h>
#include <byteswap.h>
+#include <stdint.h>
#include "macros.h"
+/* Test bswap functions with constant values. */
+static void
+test_bswap_constant (void)
+{
+ ASSERT (bswap_16 (UINT16_C (0x1234)) == UINT16_C (0x3412));
+ ASSERT (bswap_32 (UINT32_C (0x12345678)) == UINT32_C (0x78563412));
+ ASSERT (bswap_64 (UINT64_C (0x1234567890ABCDEF))
+ == UINT64_C (0xEFCDAB9078563412));
+}
+
+/* Test that the bswap functions evaluate their arguments once. */
+static void
+test_bswap_eval_once (void)
+{
+ uint16_t value_1 = 0;
+ uint32_t value_2 = 0;
+ uint64_t value_3 = 0;
+
+ ASSERT (bswap_16 (value_1++) == 0);
+ ASSERT (bswap_32 (value_2++) == 0);
+ ASSERT (bswap_64 (value_3++) == 0);
+
+ ASSERT (value_1 == 1);
+ ASSERT (value_2 == 1);
+ ASSERT (value_3 == 1);
+}
+
+/* Test that the bswap functions accept floating-point arguments. */
+static void
+test_bswap_double (void)
+{
+ ASSERT (bswap_16 (0.0) == 0);
+ ASSERT (bswap_32 (0.0) == 0);
+ ASSERT (bswap_64 (0.0) == 0);
+}
+
int
-main ()
+main (void)
{
- ASSERT (bswap_16 (0xABCD) == 0xCDAB);
- ASSERT (bswap_32 (0xDEADBEEF) == 0xEFBEADDE);
+ test_bswap_constant ();
+ test_bswap_eval_once ();
+ test_bswap_double ();
return 0;
}
--
2.45.0
