Host endian independence

Host endian independence

[Damien Zammit]

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

Hi there,

Given that GCC is smart enough to optimize bitshifts of bytes to casts
followed by bswaps if necessary on machines that have such instructions,
I propose to remove most (if not all) dependence on BYTE_ORDER in glibc
so that the same code can be written for any host - this will fix some
edge cases for middle endian machines and make the code easier to read.

There does not seem to be any performance hit (at least on x86
achitectures) by making the code host endian independent by choosing a
particular endian for streams and interpreting all streams in the
correct endian using bitshifts when streams are 4-byte aligned.
(0x11 << 8) | 0x22 = 0x1122 on any host endian machine.

Please see attached patches for proof of concept, which remove
dependence on BYTE_ORDER for two subfolders of glibc.

If this is a desirable change, please let me know and I can continue
this work.

Thanks,
Damien Zammit

[-- Attachment #2: 0001-include-endian-helpers.h-Add-endian-specific-stream-.patch --]
[-- Type: text/x-patch, Size: 4975 bytes --]

From bad1f44dd866f0dbc18df00aac2e22e9e216455a Mon Sep 17 00:00:00 2001
From: Damien Zammit <damien@zamaudio.com>
Date: Thu, 22 Aug 2019 20:10:19 +1000
Subject: [PATCH 1/3] include/endian-helpers.h: Add endian specific stream
 handling helpers

---
 include/endian-helpers.h | 193 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 193 insertions(+)
 create mode 100644 include/endian-helpers.h

diff --git a/include/endian-helpers.h b/include/endian-helpers.h
new file mode 100644
index 00000000..c421cae7
--- /dev/null
+++ b/include/endian-helpers.h
@@ -0,0 +1,193 @@
+#ifndef ENDIAN_HELPERS_H_
+#define ENDIAN_HELPERS_H_
+#include <inttypes.h>
+#include <stddef.h>
+
+/* LE routines */
+
+static inline uint16_t
+__attribute ((always_inline))
+read_le16(void *src)
+{
+	uint8_t *buf = (uint8_t *)src;
+	return ((uint16_t)(buf[1]) << 8) | (uint16_t)(buf[0]);
+}
+
+static inline void
+__attribute ((always_inline))
+write_le16(void *dest, uint16_t val)
+{
+	uint8_t *buf = (uint8_t *)dest;
+	buf[0] = (uint8_t)val;
+	buf[1] = (uint8_t)(val >> 8);
+}
+
+static inline uint32_t
+__attribute ((always_inline))
+read_le32(void *src)
+{
+	uint8_t *buf = (uint8_t *)src;
+	return ((uint32_t)(buf[3]) << 24) |
+		((uint32_t)(buf[2]) << 16) |
+		((uint32_t)(buf[1]) << 8) |
+		(uint32_t)(buf[0]);
+}
+
+static inline void
+__attribute ((always_inline))
+write_le32(void *dest, uint32_t val)
+{
+	uint8_t *buf = (uint8_t *)dest;
+	buf[0] = (uint8_t)val;
+	buf[1] = (uint8_t)(val >> 8);
+	buf[2] = (uint8_t)(val >> 16);
+	buf[3] = (uint8_t)(val >> 24);
+}
+
+static inline uint64_t
+__attribute ((always_inline))
+read_le64(void *src)
+{
+	uint8_t *buf = (uint8_t *)src;
+	return ((uint64_t)(buf[7]) << 56) |
+		((uint64_t)(buf[6]) << 48) |
+		((uint64_t)(buf[5]) << 40) |
+		((uint64_t)(buf[4]) << 32) |
+		((uint64_t)(buf[3]) << 24) |
+		((uint64_t)(buf[2]) << 16) |
+		((uint64_t)(buf[1]) << 8) |
+		(uint64_t)(buf[0]);
+}
+
+static inline void
+__attribute ((always_inline))
+write_le64(void *dest, uint64_t val)
+{
+	uint8_t *buf = (uint8_t *)dest;
+	buf[0] = (uint8_t)val;
+	buf[1] = (uint8_t)(val >> 8);
+	buf[2] = (uint8_t)(val >> 16);
+	buf[3] = (uint8_t)(val >> 24);
+	buf[4] = (uint8_t)(val >> 32);
+	buf[5] = (uint8_t)(val >> 40);
+	buf[6] = (uint8_t)(val >> 48);
+	buf[7] = (uint8_t)(val >> 56);
+}
+
+static inline uintptr_t
+__attribute ((always_inline))
+read_leptr(void *src)
+{
+	if (sizeof (size_t) == 8) {
+		return (uintptr_t)read_le64 (src);
+	} else if (sizeof (size_t) == 4) {
+		return (uintptr_t)read_le32 (src);
+	}
+	return (uintptr_t)0;
+}
+
+static inline void
+__attribute ((always_inline))
+write_leptr(void *dest, uintptr_t ptr)
+{
+	if (sizeof (size_t) == 8) {
+		write_le64 (dest, (uint64_t)ptr);
+	} else if (sizeof (size_t) == 4) {
+		write_le32 (dest, (uint32_t)ptr);
+	}
+}
+
+/* BE routines */
+
+static inline uint16_t
+__attribute ((always_inline))
+read_be16(void *src)
+{
+	uint8_t *buf = (uint8_t *)src;
+	return ((uint16_t)(buf[0]) << 8) | (uint16_t)(buf[1]);
+}
+
+static inline void
+__attribute ((always_inline))
+write_be16(void *dest, uint16_t val)
+{
+	uint8_t *buf = (uint8_t *)dest;
+	buf[0] = (uint8_t)(val >> 8);
+	buf[1] = (uint8_t)val;
+}
+
+static inline uint32_t
+__attribute ((always_inline))
+read_be32(void *src)
+{
+	uint8_t *buf = (uint8_t *)src;
+	return ((uint32_t)(buf[0]) << 24) |
+		((uint32_t)(buf[1]) << 16) |
+		((uint32_t)(buf[2]) << 8) |
+		(uint32_t)(buf[3]);
+}
+
+static inline void
+__attribute ((always_inline))
+write_be32(void *dest, uint32_t val)
+{
+	uint8_t *buf = (uint8_t *)dest;
+	buf[0] = (uint8_t)(val >> 24);
+	buf[1] = (uint8_t)(val >> 16);
+	buf[2] = (uint8_t)(val >> 8);
+	buf[3] = (uint8_t)val;
+}
+
+static inline uint64_t
+__attribute ((always_inline))
+read_be64(void *src)
+{
+	uint8_t *buf = (uint8_t *)src;
+	return ((uint64_t)(buf[0]) << 56) |
+		((uint64_t)(buf[1]) << 48) |
+		((uint64_t)(buf[2]) << 40) |
+		((uint64_t)(buf[3]) << 32) |
+		((uint64_t)(buf[4]) << 24) |
+		((uint64_t)(buf[5]) << 16) |
+		((uint64_t)(buf[6]) << 8) |
+		(uint64_t)(buf[7]);
+}
+
+static inline void
+__attribute ((always_inline))
+write_be64(void *dest, uint64_t val)
+{
+	uint8_t *buf = (uint8_t *)dest;
+	buf[0] = (uint8_t)(val >> 56);
+	buf[1] = (uint8_t)(val >> 48);
+	buf[2] = (uint8_t)(val >> 40);
+	buf[3] = (uint8_t)(val >> 32);
+	buf[4] = (uint8_t)(val >> 24);
+	buf[5] = (uint8_t)(val >> 16);
+	buf[6] = (uint8_t)(val >> 8);
+	buf[7] = (uint8_t)val;
+}
+
+static inline uintptr_t
+__attribute ((always_inline))
+read_beptr(void *src)
+{
+	if (sizeof (size_t) == 8) {
+		return (uintptr_t)read_be64 (src);
+	} else if (sizeof (size_t) == 4) {
+		return (uintptr_t)read_be32 (src);
+	}
+	return (uintptr_t)0;
+}
+
+static inline void
+__attribute ((always_inline))
+write_beptr(void *dest, uintptr_t ptr)
+{
+	if (sizeof (size_t) == 8) {
+		write_be64 (dest, (uint64_t)ptr);
+	} else if (sizeof (size_t) == 4) {
+		write_be32 (dest, (uint32_t)ptr);
+	}
+}
+#endif
-- 
2.13.1


[-- Attachment #3: 0002-time-tzfile.c-Remove-dependence-on-BYTE_ORDER.patch --]
[-- Type: text/x-patch, Size: 4081 bytes --]

From 2ef3824d2561987f08db5f3ef9be8b1660a86cd1 Mon Sep 17 00:00:00 2001
From: Damien Zammit <damien@zamaudio.com>
Date: Thu, 22 Aug 2019 20:11:46 +1000
Subject: [PATCH 2/3] time/tzfile.c: Remove dependence on BYTE_ORDER

---
 time/tzfile.c | 63 +++++++++++++----------------------------------------------
 1 file changed, 14 insertions(+), 49 deletions(-)

diff --git a/time/tzfile.c b/time/tzfile.c
index a7d05e2d..f55bed98 100644
--- a/time/tzfile.c
+++ b/time/tzfile.c
@@ -26,6 +26,7 @@
 #include <sys/stat.h>
 #include <stdint.h>
 #include <alloc_buffer.h>
+#include <endian-helpers.h>
 
 #include <timezone/tzfile.h>
 
@@ -61,42 +62,6 @@ static size_t num_leaps;
 static struct leap *leaps;
 static char *tzspec;
 
-#include <endian.h>
-#include <byteswap.h>
-
-/* Decode the four bytes at PTR as a signed integer in network byte order.  */
-static inline int
-__attribute ((always_inline))
-decode (const void *ptr)
-{
-  if (BYTE_ORDER == BIG_ENDIAN && sizeof (int) == 4)
-    return *(const int *) ptr;
-  if (sizeof (int) == 4)
-    return bswap_32 (*(const int *) ptr);
-
-  const unsigned char *p = ptr;
-  int result = *p & (1 << (CHAR_BIT - 1)) ? ~0 : 0;
-
-  result = (result << 8) | *p++;
-  result = (result << 8) | *p++;
-  result = (result << 8) | *p++;
-  result = (result << 8) | *p++;
-
-  return result;
-}
-
-
-static inline int64_t
-__attribute ((always_inline))
-decode64 (const void *ptr)
-{
-  if ((BYTE_ORDER == BIG_ENDIAN))
-    return *(const int64_t *) ptr;
-
-  return bswap_64 (*(const int64_t *) ptr);
-}
-
-
 void
 __tzfile_read (const char *file, size_t extra, char **extrap)
 {
@@ -184,12 +149,12 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
       || memcmp (tzhead.tzh_magic, TZ_MAGIC, sizeof (tzhead.tzh_magic)) != 0)
     goto lose;
 
-  num_transitions = (size_t) decode (tzhead.tzh_timecnt);
-  num_types = (size_t) decode (tzhead.tzh_typecnt);
-  chars = (size_t) decode (tzhead.tzh_charcnt);
-  num_leaps = (size_t) decode (tzhead.tzh_leapcnt);
-  num_isstd = (size_t) decode (tzhead.tzh_ttisstdcnt);
-  num_isgmt = (size_t) decode (tzhead.tzh_ttisgmtcnt);
+  num_transitions = (size_t) read_be32 (tzhead.tzh_timecnt);
+  num_types = (size_t) read_be32 (tzhead.tzh_typecnt);
+  chars = (size_t) read_be32 (tzhead.tzh_charcnt);
+  num_leaps = (size_t) read_be32 (tzhead.tzh_leapcnt);
+  num_isstd = (size_t) read_be32 (tzhead.tzh_ttisstdcnt);
+  num_isgmt = (size_t) read_be32 (tzhead.tzh_ttisgmtcnt);
 
   if (__glibc_unlikely (num_isstd > num_types || num_isgmt > num_types))
     goto lose;
@@ -315,14 +280,14 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
 	 processed.  */
       i = num_transitions;
       while (i-- > 0)
-	transitions[i] = decode ((char *) transitions + i * 4);
+	transitions[i] = read_be32 ((char *) transitions + i * 4);
     }
-  else if (BYTE_ORDER != BIG_ENDIAN)
+  else
     {
       /* Decode the transition times, stored as 8-byte integers in
 	 network (big-endian) byte order.  */
       for (i = 0; i < num_transitions; ++i)
-	transitions[i] = decode64 ((char *) transitions + i * 8);
+	transitions[i] = read_be64 ((char *) transitions + i * 8);
     }
 
   for (i = 0; i < num_types; ++i)
@@ -342,7 +307,7 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
 	/* Bogus index in data file.  */
 	goto lose;
       types[i].idx = c;
-      types[i].offset = decode (x);
+      types[i].offset = read_be32 (x);
     }
 
   if (__glibc_unlikely (__fread_unlocked (zone_names, 1, chars, f) != chars))
@@ -355,13 +320,13 @@ __tzfile_read (const char *file, size_t extra, char **extrap)
 			    != trans_width, 0))
 	goto lose;
       if (trans_width == 4)
-	leaps[i].transition = decode (x);
+	leaps[i].transition = read_be32 (x);
       else
-	leaps[i].transition = decode64 (x);
+	leaps[i].transition = read_be64 (x);
 
       if (__glibc_unlikely (__fread_unlocked (x, 1, 4, f) != 4))
 	goto lose;
-      leaps[i].change = (long int) decode (x);
+      leaps[i].change = (long int) read_be32 (x);
     }
 
   for (i = 0; i < num_isstd; ++i)
-- 
2.13.1


[-- Attachment #4: 0003-catgets-Remove-dependence-on-BYTE_ORDER.patch --]
[-- Type: text/x-patch, Size: 6006 bytes --]

From 32a3f800b3977bb7c0826e231c89c8a242cc5e69 Mon Sep 17 00:00:00 2001
From: Damien Zammit <damien@zamaudio.com>
Date: Thu, 22 Aug 2019 20:15:51 +1000
Subject: [PATCH 3/3] catgets: Remove dependence on BYTE_ORDER

---
 catgets/gencat.c       | 42 +++++++++++++++++++-----------------------
 catgets/open_catalog.c | 31 ++++++++++++-------------------
 2 files changed, 31 insertions(+), 42 deletions(-)

diff --git a/catgets/gencat.c b/catgets/gencat.c
index f0d47ae0..e4b73e5d 100644
--- a/catgets/gencat.c
+++ b/catgets/gencat.c
@@ -22,7 +22,7 @@
 #include <argp.h>
 #include <assert.h>
 #include <ctype.h>
-#include <endian.h>
+#include <endian-helpers.h>
 #include <errno.h>
 #include <error.h>
 #include <fcntl.h>
@@ -44,10 +44,6 @@
 
 #include "catgetsinfo.h"
 
-
-#define SWAPU32(w) \
-  (((w) << 24) | (((w) & 0xff00) << 8) | (((w) >> 8) & 0xff00) | ((w) >> 24))
-
 struct message_list
 {
   int number;
@@ -852,7 +848,8 @@ write_out (struct catalog *catalog, const char *output_name,
   struct obstack string_pool;
   const char *strings;
   size_t strings_size;
-  uint32_t *array1, *array2;
+  uint32_t *array1;
+  uint8_t buf[8];
   size_t cnt;
   int fd;
 
@@ -932,8 +929,6 @@ write_out (struct catalog *catalog, const char *output_name,
   array1 =
     (uint32_t *) alloca (best_size * best_depth * sizeof (uint32_t) * 3);
   memset (array1, '\0', best_size * best_depth * sizeof (uint32_t) * 3);
-  array2
-    = (uint32_t *) alloca (best_size * best_depth * sizeof (uint32_t) * 3);
   obstack_init (&string_pool);
 
   set_run = catalog->all_sets;
@@ -969,10 +964,6 @@ write_out (struct catalog *catalog, const char *output_name,
   strings_size = obstack_object_size (&string_pool);
   strings = obstack_finish (&string_pool);
 
-  /* Compute ARRAY2 by changing the byte order.  */
-  for (cnt = 0; cnt < best_size * best_depth * 3; ++cnt)
-    array2[cnt] = SWAPU32 (array1[cnt]);
-
   /* Now we can write out the whole data.  */
   if (strcmp (output_name, "-") == 0
       || strcmp (output_name, "/dev/stdout") == 0)
@@ -986,19 +977,24 @@ write_out (struct catalog *catalog, const char *output_name,
     }
 
   /* Write out header.  */
-  write (fd, &obj, sizeof (obj));
+  write_le32 (buf, obj.magic);
+  write (fd, buf, sizeof (uint32_t));
+  write_le32 (buf, obj.plane_size);
+  write (fd, buf, sizeof (uint32_t));
+  write_le32 (buf, obj.plane_depth);
+  write (fd, buf, sizeof (uint32_t));
+  /* Nothing to write for obj.name_ptr - it is 0 bytes itself */
 
   /* We always write out the little endian version of the index
-     arrays.  */
-#if __BYTE_ORDER == __LITTLE_ENDIAN
-  write (fd, array1, best_size * best_depth * sizeof (uint32_t) * 3);
-  write (fd, array2, best_size * best_depth * sizeof (uint32_t) * 3);
-#elif __BYTE_ORDER == __BIG_ENDIAN
-  write (fd, array2, best_size * best_depth * sizeof (uint32_t) * 3);
-  write (fd, array1, best_size * best_depth * sizeof (uint32_t) * 3);
-#else
-# error Cannot handle __BYTE_ORDER byte order
-#endif
+     array (LE then BE) */
+  for (cnt = 0; cnt < best_size * best_depth * 3; ++cnt) {
+    write_le32 (buf, array1[cnt]);
+    write (fd, buf, sizeof (uint32_t));
+  }
+  for (cnt = 0; cnt < best_size * best_depth * 3; ++cnt) {
+    write_be32 (buf, array1[cnt]);
+    write (fd, buf, sizeof (uint32_t));
+  }
 
   /* Finally write the strings.  */
   write (fd, strings, strings_size);
diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c
index 9aa6a09f..5c124073 100644
--- a/catgets/open_catalog.c
+++ b/catgets/open_catalog.c
@@ -16,8 +16,7 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <byteswap.h>
-#include <endian.h>
+#include <endian-helpers.h>
 #include <errno.h>
 #include <fcntl.h>
 #include <string.h>
@@ -32,9 +31,6 @@
 #include <not-cancel.h>
 
 
-#define SWAPU32(w) bswap_32 (w)
-
-
 int
 __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
 		__nl_catd catalog)
@@ -255,11 +251,11 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
     }
 
   /* Determine whether the file is a catalog file and if yes whether
-     it is written using the correct byte order.  Else we have to swap
-     the values.  */
-  if (__glibc_likely (catalog->file_ptr->magic == CATGETS_MAGIC))
+     it is written using the correct byte order.  Else we have to read
+     the values in opposite endian.  */
+  if (__glibc_likely (read_le32 (&catalog->file_ptr->magic) == CATGETS_MAGIC))
     swapping = 0;
-  else if (catalog->file_ptr->magic == SWAPU32 (CATGETS_MAGIC))
+  else if (read_be32 (&catalog->file_ptr->magic) == CATGETS_MAGIC)
     swapping = 1;
   else
     {
@@ -275,23 +271,20 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var,
       goto close_unlock_return;
     }
 
-#define SWAP(x) (swapping ? SWAPU32 (x) : (x))
+#define READ32(x) (swapping ? read_be32 (x) : read_le32 (x))
 
   /* Get dimensions of the used hashing table.  */
-  catalog->plane_size = SWAP (catalog->file_ptr->plane_size);
-  catalog->plane_depth = SWAP (catalog->file_ptr->plane_depth);
+  catalog->plane_size = READ32 (&catalog->file_ptr->plane_size);
+  catalog->plane_depth = READ32 (&catalog->file_ptr->plane_depth);
 
   /* The file contains two versions of the pointer tables.  Pick the
      right one for the local byte order.  */
-#if __BYTE_ORDER == __LITTLE_ENDIAN
-  catalog->name_ptr = &catalog->file_ptr->name_ptr[0];
-#elif __BYTE_ORDER == __BIG_ENDIAN
-  catalog->name_ptr = &catalog->file_ptr->name_ptr[catalog->plane_size
+  if (__glibc_unlikely (swapping))
+    catalog->name_ptr = &catalog->file_ptr->name_ptr[catalog->plane_size
 						  * catalog->plane_depth
 						  * 3];
-#else
-# error Cannot handle __BYTE_ORDER byte order
-#endif
+  else
+    catalog->name_ptr = &catalog->file_ptr->name_ptr[0];
 
   /* The rest of the file contains all the strings.  They are
      addressed relative to the position of the first string.  */
-- 
2.13.1

Re: Host endian independence

[Joseph Myers]

On Sat, 24 Aug 2019, Damien Zammit wrote:

> Please see attached patches for proof of concept, which remove
> dependence on BYTE_ORDER for two subfolders of glibc.

I don't think these changes are appropriate.

I think we should make more use of the *existing* byte-swap interfaces, 
such as be32toh and be64toh in <endian.h>, rather than inventing new ones.  
By using those interfaces, tzfile.c, for example, could lose some of its 
existing endian checks (that would be a very small local change to the 
implementations of the decode and decode64 functions, larger changes are 
not needed and make the code less clean because the logical information 
that certain data is stored in the files in big-endian format is best kept 
local to the implementations of those two functions, rather than 
hardcoding that information in lots of places with read_be32 and read_be64 
names).  (I'm not convinced that any changes in this area beyond very 
minimal use of bswap_32 would improve the catgets code.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Host endian independence

[Damien Zammit]

Hi Joseph,

Thanks for your reply.

My goal is to introduce the endian-helpers and remove all code relating to byte-swapping,
then the existing byte-swapping interfaces may not be needed and could potentially be removed.

On 28/8/19 1:54 am, Joseph Myers wrote:
> I don't think these changes are appropriate.
> 
> I think we should make more use of the *existing* byte-swap interfaces, 
> such as be32toh and be64toh in <endian.h>, rather than inventing new ones.

Firstly, endian-helpers.h is not a byte-swapping interface, it is a collection of functions
that read/write streams in desired endian, which is something currently missing from glibc.  
I can see that your preference is to reuse existing byte-swap interfaces.
However, I am suggesting that byte-swapping, in general, is unnecessary and a kludge.
It also makes it very confusing for someone reading the code which endian a stream is
stored in when you are swapping based on the host machine's byte order.
Knowing the endianness of a stream in advance gives you the ability to write
one parsing function for each integer type within a stream that works on any host
and the compiler can optimize it.  The endianness of the stream can (and should only)
be handled at the interface where the stream is being converted from bytes to integer types
and vice versa, rather than having structs containing conditionally swapped bytes.

> By using those interfaces, tzfile.c, for example, could lose some of its 
> existing endian checks (that would be a very small local change to the 
> implementations of the decode and decode64 functions, larger changes are 
> not needed and make the code less clean because the logical information 
> that certain data is stored in the files in big-endian format is best kept 
> local to the implementations of those two functions, rather than 
> hardcoding that information in lots of places with read_be32 and read_be64 
> names).

If a stream coming from a file is stored in big endian, why not be explicit in
the naming of functions used to decode it so that it is clear which endian it is?

> ... (I'm not convinced that any changes in this area beyond very 
> minimal use of bswap_32 would improve the catgets code.)

I mostly agree, but I picked off two directories that were easy targets for my
host endian independent demonstration.  The purpose was to keep the code mostly the same,
*except* you can see there is now no more dependence on host BYTE_ORDER at all in the code.

--
Damien Zammit

Re: Host endian independence

[Joseph Myers]

On Thu, 29 Aug 2019, Damien Zammit wrote:

> Firstly, endian-helpers.h is not a byte-swapping interface, it is a 
> collection of functions that read/write streams in desired endian, which 
> is something currently missing from glibc.

That reads like simply a personal preference for arranging things 
differently, which isn't a good basis for changing how things are done 
away from very well established and widely used Unix style (functions such 
as htonl; interfaces such as htobe32 are simply a more modern version of 
that, avoiding the historical "network" and "long" concepts implicit in 
the older names).  Avoiding such a header at all means we can have a much 
smaller patch that genuinely improves consistency (reduces the number of 
explicit tests of endianness in glibc) rather than changing things for one 
person's individual taste and means we don't need to go through all the 
many deviations in that header from GNU coding style.

> I can see that your preference is to reuse existing byte-swap 
> interfaces. However, I am suggesting that byte-swapping, in general, is 
> unnecessary and a kludge.

glibc isn't application code.  It's system-level C, written for a 
particular profile of common-usage architectures and ABIs.  In 
system-level C, expressing things in terms of converting between host byte 
order and a particular byte-order for an external interface is entirely 
appropriate.  And interfaces such as htobe32 and be32toh make clear 
exactly what conversions are taking place.

Furthermore, the implementations of those interfaces use __builtin_bswap*, 
which are exactly the appropriate idioms for such byte-swapping in GNU C.

A plausible alternative would be to use the scalar_storage_order attribute 
(added to GCC in 2015, so available in all GCC versions now supported for 
building glibc) - subject to checking how well it is optimized compared to 
the present code (e.g. if a field gets used multiple times, does the 
compiler optimize that to only doing a single endian conversion on it?); 
that could eliminate the need for any explicit endian conversions at all 
in some places.  But that would require careful consideration to gain 
consensus before actually introducing any uses into glibc.

> > By using those interfaces, tzfile.c, for example, could lose some of its 
> > existing endian checks (that would be a very small local change to the 
> > implementations of the decode and decode64 functions, larger changes are 
> > not needed and make the code less clean because the logical information 
> > that certain data is stored in the files in big-endian format is best kept 
> > local to the implementations of those two functions, rather than 
> > hardcoding that information in lots of places with read_be32 and read_be64 
> > names).
> 
> If a stream coming from a file is stored in big endian, why not be 
> explicit in the naming of functions used to decode it so that it is 
> clear which endian it is?

General principles of encapsulation of information in one place.  There is 
one piece of information "tzfile format uses big-endian" that is most 
cleanly kept in one place (so avoiding typos from a single place 
accidentally using *_le32 instead of *_be32, for example).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Host endian independence

[Yann Droneaud]

Hi,

Le jeudi 29 août 2019 à 14:45 +1000, Damien Zammit a écrit :
> 
> Thanks for your reply.
> 
> My goal is to introduce the endian-helpers and remove all code relating to byte-swapping,
> then the existing byte-swapping interfaces may not be needed and could potentially be removed.
> 
> On 28/8/19 1:54 am, Joseph Myers wrote:
> > I don't think these changes are appropriate.
> > 
> > I think we should make more use of the *existing* byte-swap interfaces, 
> > such as be32toh and be64toh in <endian.h>, rather than inventing new ones.
> 
> Firstly, endian-helpers.h is not a byte-swapping interface, it is a collection of functions
> that read/write streams in desired endian, which is something currently missing from glibc.  
> I can see that your preference is to reuse existing byte-swap interfaces.
> However, I am suggesting that byte-swapping, in general, is unnecessary and a kludge.
> It also makes it very confusing for someone reading the code which endian a stream is
> stored in when you are swapping based on the host machine's byte order.
> Knowing the endianness of a stream in advance gives you the ability to write
> one parsing function for each integer type within a stream that works on any host
> and the compiler can optimize it.  The endianness of the stream can (and should only)
> be handled at the interface where the stream is being converted from bytes to integer types
> and vice versa, rather than having structs containing conditionally swapped bytes.
> 
> > By using those interfaces, tzfile.c, for example, could lose some of its 
> > existing endian checks (that would be a very small local change to the 
> > implementations of the decode and decode64 functions, larger changes are 
> > not needed and make the code less clean because the logical information 
> > that certain data is stored in the files in big-endian format is best kept 
> > local to the implementations of those two functions, rather than 
> > hardcoding that information in lots of places with read_be32 and read_be64 
> > names).
> 
> If a stream coming from a file is stored in big endian, why not be explicit in
> the naming of functions used to decode it so that it is clear which endian it is?
> 
> > ... (I'm not convinced that any changes in this area beyond very 
> > minimal use of bswap_32 would improve the catgets code.)
> 
> I mostly agree, but I picked off two directories that were easy targets for my
> host endian independent demonstration.  The purpose was to keep the code mostly the same,
> *except* you can see there is now no more dependence on host BYTE_ORDER at all in the code.
> 

This remind me of the Rob Pike article "The byte order fallacy"
https://commandcenter.blogspot.com/2012/04/byte-order-fallacy.html

Regards.

-- 
Yann Droneaud
OPTEYA

Re: Host endian independence

[Florian Weimer]

* Joseph Myers:

> On Sat, 24 Aug 2019, Damien Zammit wrote:
>
>> Please see attached patches for proof of concept, which remove
>> dependence on BYTE_ORDER for two subfolders of glibc.
>
> I don't think these changes are appropriate.
>
> I think we should make more use of the *existing* byte-swap interfaces, 
> such as be32toh and be64toh in <endian.h>, rather than inventing new ones.  
> By using those interfaces, tzfile.c, for example, could lose some of its 
> existing endian checks (that would be a very small local change to the 
> implementations of the decode and decode64 functions, larger changes are 
> not needed and make the code less clean because the logical information 
> that certain data is stored in the files in big-endian format is best kept 
> local to the implementations of those two functions, rather than 
> hardcoding that information in lots of places with read_be32 and read_be64 
> names).  (I'm not convinced that any changes in this area beyond very 
> minimal use of bswap_32 would improve the catgets code.)

The problem with the existing interfaces is that that they make it hard
to take alignment issues into account.  See bug 20243.  When used for
parsing packet buffers, they also tend to introduce aliasing violations.

Nowadays, compilers should optimize multiple byte-wise reads into wide
load, possibly followed by a byte swap instruction, on targets that do
not require strict alignment.  (Or a cross-endian load if the target
supports that.)  In the past, it was really necessary for good
performance to have properly aligned buffers and parse them with an
aliasing violation, but that may no longer be necessary.

Obviously, packet parsing and generation should be done with a
higher-level abstraction and not these read/write functions, to enforce
error checking, but these functions could serve as a building block for
that.

Thanks,
Florian

Re: Host endian independence

[Joseph Myers]

On Fri, 30 Aug 2019, Florian Weimer wrote:

> The problem with the existing interfaces is that that they make it hard
> to take alignment issues into account.  See bug 20243.  When used for
> parsing packet buffers, they also tend to introduce aliasing violations.

That seems like a matter of declaring your structure appropriately (with 
the right alignment if e.g. it's being allocated on the stack, or with the 
packed attribute if it might genuinely be at arbitrary alignments; with 
the may_alias attribute if it's necessary for it to alias something with a 
different effective type).

The scalar_storage_order attribute could certainly be added to an 
appropriately declared structure to move the endianness information 
entirely into how the structure is defined and out of the code using the 
structure.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: Host endian independence

[Damien Zammit]

On 31/8/19 2:22 am, Joseph Myers wrote:
> The scalar_storage_order attribute could certainly be added to an 
> appropriately declared structure to move the endianness information 
> entirely into how the structure is defined and out of the code using the 
> structure.

I was not aware of this attribute until now, but looks like the best approach to me.
I might put together a new proof of concept and send in a new set of patches for review.
I'm not expecting it to be merged as is, as you said, Joseph, it would need 
to be carefully considered and tested - I think it would be good to have something to test.
Does libc-alpha have a mailing list convention for sending patches in that are WIP?

Florian Weimer wrote:
> The problem with the existing interfaces is that that they make it hard
> to take alignment issues into account.  See bug 20243.  When used for
> parsing packet buffers, they also tend to introduce aliasing violations.

I had a quick look at bug 20243, would it also be fixed by using the "packed" attribute and
Joseph's idea of putting the endianness info in the definition of the structs with the
"scalar_storage_order" attribute?

Florian Weimer wrote:
> Obviously, packet parsing and generation should be done with a
> higher-level abstraction and not these read/write functions, to enforce
> error checking, but these functions could serve as a building block for
> that.

Neither code like endian-helpers.h nor byte-swapping interfaces seem needed if we use this
attribute and packed structs.  Endianness issues can be solved cleanly and most byte-swapping
in the code can be removed.
Florian, what do you think of using a "scalar_storage_order" and "packed" approach?

Thanks for all the ideas and great answers.

--
Damien Zammit
damien AT zamaudio.com

Re: Host endian independence

[Florian Weimer]

* Damien Zammit:

> On 31/8/19 2:22 am, Joseph Myers wrote:
>> The scalar_storage_order attribute could certainly be added to an 
>> appropriately declared structure to move the endianness information 
>> entirely into how the structure is defined and out of the code using the 
>> structure.
>
> I was not aware of this attribute until now, but looks like the best
> approach to me.  I might put together a new proof of concept and send
> in a new set of patches for review.

You need to be careful where you use this features.  Some shared code
cannot use features which are GNU C only (not even the C++ frontend
supports it).  And it's always a bit risky to be an early adopter of
such compiler extensions.

> Florian Weimer wrote:
>> The problem with the existing interfaces is that that they make it hard
>> to take alignment issues into account.  See bug 20243.  When used for
>> parsing packet buffers, they also tend to introduce aliasing violations.
>
> I had a quick look at bug 20243, would it also be fixed by using the
> "packed" attribute and Joseph's idea of putting the endianness info in
> the definition of the structs with the "scalar_storage_order"
> attribute?

Note that either case will need a new struct definition.

Thanks,
Florian

Re: Host endian independence

[Florian Weimer]

* Joseph Myers:

> On Fri, 30 Aug 2019, Florian Weimer wrote:
>
>> The problem with the existing interfaces is that that they make it hard
>> to take alignment issues into account.  See bug 20243.  When used for
>> parsing packet buffers, they also tend to introduce aliasing violations.
>
> That seems like a matter of declaring your structure appropriately (with 
> the right alignment if e.g. it's being allocated on the stack, or with the 
> packed attribute if it might genuinely be at arbitrary alignments; with 
> the may_alias attribute if it's necessary for it to alias something with a 
> different effective type).

To some degree, yes, but I don't think the GCC internal type system
supports member alignment (and storage order, for the pragma), so you
need to be extremely careful when building abstractions around that.  It
doesn't help that all the common targets (from my perspective) do not
have alignment traps, so bugs are difficult to spot.

Thanks,
Florian