On 5/13/24 10:40 AM, Paul Eggert wrote:
> Unfortunately this patch suffers from the problem we discussed earlier, e.g.,
> the substitute be16toh (n++) has undefined behavior but it should add 1 to n
> and otherwise act as if be16toh (n) was called.
[...]
> These problems arise because Gnulib byteswap.h's macros can evaluate their
> arguments more than once, and are type-generic in a way that is incompatible
> with what POSIX wants for endian.h functions.
I think that this should be addressed in byteswap.h as well. Since
glibc defines them as inline functions. How about the two attached
patches. The version numbers for __builtin_checks are copied from my
/bits/byteswap.h.
> Also, there's a bizarre compatibility issue, in that some floating-point args
> have well-defined behavior in the POSIX spec, e.g., be16toh (0.0) yields 0,
> but this implementation rejects these calls. Although this is lower priority
> it's easy to fix so we might as well do it.
Ah, my floating point knowledge is not so great...
> To fix this, please use _GL_INLINE and implement with inline functions. And
> add please add test cases to catch these issues.
If we can ensure byteswap.h functions are defined as functions,
wouldn't it make sense to just define these as macros to them?
Collin
From f4a34789c8f55161c1cab343a97511206761ff47 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Mon, 13 May 2024 15:05:33 -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
so arguments are evaluated once.
* modules/byteswap (Files): Add lib/byteswap.c.
(Depends-on): Add extern-inline. Add stdint as a conditional dependency.
(Makefile.am): Add lib/byteswap.c to lib_SOURCES.
---
ChangeLog | 10 +++++++
lib/byteswap.c | 21 ++++++++++++++
lib/byteswap.in.h | 74 ++++++++++++++++++++++++++++++++++++++---------
modules/byteswap | 4 +++
4 files changed, 95 insertions(+), 14 deletions(-)
create mode 100644 lib/byteswap.c
diff --git a/ChangeLog b/ChangeLog
index 09cdfb44f9..5a9354da16 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2024-05-13 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
+ so arguments are evaluated once.
+ * modules/byteswap (Files): Add lib/byteswap.c.
+ (Depends-on): Add extern-inline. Add stdint as a conditional dependency.
+ (Makefile.am): Add lib/byteswap.c to lib_SOURCES.
+
2024-05-13 Bruno Haible <br...@clisp.org>
doc: Document our conventions for *.m4 files.
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..24070f3240 100644
--- a/lib/byteswap.in.h
+++ b/lib/byteswap.in.h
@@ -18,27 +18,73 @@
#ifndef _GL_BYTESWAP_H
#define _GL_BYTESWAP_H
+/* 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 (defined __GNUC__ && (__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 (defined __GNUC__ && (__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 (defined __GNUC__ && (__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/modules/byteswap b/modules/byteswap
index 68fe6b78ae..3d836bf8ce 100644
--- a/modules/byteswap
+++ b/modules/byteswap
@@ -2,10 +2,13 @@ Description:
Swap bytes of 16, 32 and 64 bit values.
Files:
+lib/byteswap.c
lib/byteswap.in.h
m4/byteswap.m4
Depends-on:
+stdint [$GL_GENERATE_BYTESWAP_H]
+extern-inline
gen-header
configure.ac:
@@ -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 8f9cfcc1097b6faea843bf62865dece0885c88a6 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Mon, 13 May 2024 15:30:29 -0700
Subject: [PATCH 2/2] byteswap tests: Strengthen tests.
* modules/byteswap-tests: Add stdint to dependencies.
* tests/test-byteswap.c (test_bswap_constant, test_bswap_eval_once): New
functions.
(main): Use them.
---
ChangeLog | 8 ++++++++
modules/byteswap-tests | 1 +
tests/test-byteswap.c | 34 +++++++++++++++++++++++++++++++---
3 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 5a9354da16..1f0f0d9820 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2024-05-13 Collin Funk <collin.fu...@gmail.com>
+
+ byteswap tests: Strengthen tests.
+ * modules/byteswap-tests: Add stdint to dependencies.
+ * tests/test-byteswap.c (test_bswap_constant, test_bswap_eval_once): New
+ functions.
+ (main): Use them.
+
2024-05-13 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..6cbe76d15a 100644
--- a/tests/test-byteswap.c
+++ b/tests/test-byteswap.c
@@ -19,14 +19,42 @@
#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);
+}
+
int
-main ()
+main (void)
{
- ASSERT (bswap_16 (0xABCD) == 0xCDAB);
- ASSERT (bswap_32 (0xDEADBEEF) == 0xEFBEADDE);
+ test_bswap_constant ();
+ test_bswap_eval_once ();
return 0;
}
--
2.45.0
