[RFC] time: Create a endian_t.h headerfile

Message ID 20190919221829.6137-1-alistair.francis@wdc.com
State New
Headers show
Series
  • [RFC] time: Create a endian_t.h headerfile
Related show

Commit Message

Alistair Francis Sept. 19, 2019, 10:18 p.m.
To allow struct __timespec64 and struct timespec to be the same on a
32-bit architecture with a 64-bit time_t we need to add padding to the
struct timespec. This padding requires knowledge of the machines
endianess, which requires us exporting the endianness in
time/bits/types/struct_timespec.h.

To reduce the number of macros that we are exporting let's split out an
endian_t.h header file that can be used.

2019-09-19  Alistair Francis  <alistair.francis@wdc.com>

	* bits/endian.h: Create a endian_t.h headerfile.
	* string/endian.h: Create a endian_t.h headerfile.
	* sysdeps/aarch64/bits/endian.h: Allow including from
	endian_t.h.
	* sysdeps/alpha/bits/endian.h: Likewise.
	* sysdeps/arm/bits/endian.h: Likewise.
	* sysdeps/csky/bits/endian.h: Likewise.
	* sysdeps/hppa/bits/endian.h: Likewise.
	* sysdeps/m68k/bits/endian.h: Likewise.
	* sysdeps/microblaze/bits/endian.h: Likewise.
	* sysdeps/mips/bits/endian.h: Likewise.
	* sysdeps/nios2/bits/endian.h: Likewise.
	* sysdeps/powerpc/bits/endian.h: Likewise.
	* sysdeps/riscv/bits/endian.h: Likewise.
	* sysdeps/s390/bits/endian.h: Likewise.
	* sysdeps/sh/bits/endian.h: Likewise.
	* sysdeps/sparc/bits/endian.h: Likewise.
	* sysdeps/unix/sysv/linux/ia64/bits/endian.h: Likewise.
	* sysdeps/x86/bits/endian.h: Likewise.
	* time/bits/types/endian_t.h: New file.
---
This is based on the feedback from:
 https://sourceware.org/ml/libc-alpha/2019-09/msg00272.html

 bits/endian.h                              |  2 +-
 string/endian.h                            | 16 +---------
 sysdeps/aarch64/bits/endian.h              |  2 +-
 sysdeps/alpha/bits/endian.h                |  2 +-
 sysdeps/arm/bits/endian.h                  |  2 +-
 sysdeps/csky/bits/endian.h                 |  2 +-
 sysdeps/hppa/bits/endian.h                 |  2 +-
 sysdeps/m68k/bits/endian.h                 |  2 +-
 sysdeps/microblaze/bits/endian.h           |  2 +-
 sysdeps/mips/bits/endian.h                 |  2 +-
 sysdeps/nios2/bits/endian.h                |  2 +-
 sysdeps/powerpc/bits/endian.h              |  2 +-
 sysdeps/riscv/bits/endian.h                |  2 +-
 sysdeps/s390/bits/endian.h                 |  2 +-
 sysdeps/sh/bits/endian.h                   |  2 +-
 sysdeps/sparc/bits/endian.h                |  2 +-
 sysdeps/unix/sysv/linux/ia64/bits/endian.h |  2 +-
 sysdeps/x86/bits/endian.h                  |  2 +-
 time/bits/types/endian_t.h                 | 36 ++++++++++++++++++++++
 19 files changed, 54 insertions(+), 32 deletions(-)
 create mode 100644 time/bits/types/endian_t.h

-- 
2.23.0

Comments

Joseph Myers Sept. 19, 2019, 11:34 p.m. | #1
On Thu, 19 Sep 2019, Alistair Francis wrote:

> To allow struct __timespec64 and struct timespec to be the same on a

> 32-bit architecture with a 64-bit time_t we need to add padding to the

> struct timespec. This padding requires knowledge of the machines

> endianess, which requires us exporting the endianness in

> time/bits/types/struct_timespec.h.

> 

> To reduce the number of macros that we are exporting let's split out an

> endian_t.h header file that can be used.


An installed header has to be appropriately listed in "headers" in some 
directory's Makefile to cause it to be installed; otherwise you get code 
that builds file in the glibc source tree but breaks with an installed 
compiler and headers.

The bits/types/ namespace is *only* for headers defining a single type 
(that is, bits/types/endian_t.h would be a header that defines the 
endian_t typedef and nothing else).  So it's not appropriate for this 
header.

You can't use "#include <time/...>" in an installed header because that 
would also break when using an installed compiler and headers.  #includes 
in installed headers need to use the <bits/*> path the included header 
would have when installed.  Then you add an include/bits/ wrapper in the 
glibc source tree to allow the uninstalled header to be found when 
building glibc.  (And time/ is not the right location for a header about 
endianness.  Either string/, or use the top-level bits/ directory and so 
avoid the need for any wrapper at all.)

If anything, I think the cleanest naming would be to have <bits/endian.h> 
be the architecture-independent header that defines these three macros and 
includes <bits/endian-arch.h>, where the existing <bits/endian.h> headers 
that define __BYTE_ORDER all get renamed to <bits/endian-arch.h>.  So 
you'd need to add bits/endian-arch.h to the "headers" setting in 
string/Makefile.

-- 
Joseph S. Myers
joseph@codesourcery.com
Zack Weinberg Sept. 20, 2019, 3:20 p.m. | #2
On Thu, Sep 19, 2019 at 7:34 PM Joseph Myers <joseph@codesourcery.com> wrote:
>

> If anything, I think the cleanest naming would be to have <bits/endian.h>

> be the architecture-independent header that defines these three macros and

> includes <bits/endian-arch.h>, where the existing <bits/endian.h> headers

> that define __BYTE_ORDER all get renamed to <bits/endian-arch.h>.  So

> you'd need to add bits/endian-arch.h to the "headers" setting in

> string/Makefile.


I believe I wrote that patch already, in fact, as part of my
installed-headers cleanups series:
https://sourceware.org/ml/libc-alpha/2019-06/msg00783.html  Unlike a
lot of those patches, it should be low-risk for application breakage
and applicable independently.

zw
Joseph Myers Sept. 20, 2019, 4:51 p.m. | #3
On Fri, 20 Sep 2019, Zack Weinberg wrote:

> On Thu, Sep 19, 2019 at 7:34 PM Joseph Myers <joseph@codesourcery.com> wrote:

> >

> > If anything, I think the cleanest naming would be to have <bits/endian.h>

> > be the architecture-independent header that defines these three macros and

> > includes <bits/endian-arch.h>, where the existing <bits/endian.h> headers

> > that define __BYTE_ORDER all get renamed to <bits/endian-arch.h>.  So

> > you'd need to add bits/endian-arch.h to the "headers" setting in

> > string/Makefile.

> 

> I believe I wrote that patch already, in fact, as part of my

> installed-headers cleanups series:

> https://sourceware.org/ml/libc-alpha/2019-06/msg00783.html  Unlike a

> lot of those patches, it should be low-risk for application breakage

> and applicable independently.


Yes, that's the sort of thing I'd expect (and I had a comment on it in 
<https://sourceware.org/ml/libc-alpha/2019-07/msg00561.html>).  (This is 
not a review.  When retesting this patch it would be necessary to check if 
any new includes of <endian.h> have been added to installed headers that 
need updating as well.)

I should explicitly note that, while it's easy to think of possible 
followup cleanups in this area, it's best *not* to combine such cleanups 
with this patch in order to keep it of a reasonable size and complexity.

-- 
Joseph S. Myers
joseph@codesourcery.com
Alistair Francis Sept. 20, 2019, 9:05 p.m. | #4
On Fri, Sep 20, 2019 at 9:51 AM Joseph Myers <joseph@codesourcery.com> wrote:
>

> On Fri, 20 Sep 2019, Zack Weinberg wrote:

>

> > On Thu, Sep 19, 2019 at 7:34 PM Joseph Myers <joseph@codesourcery.com> wrote:

> > >

> > > If anything, I think the cleanest naming would be to have <bits/endian.h>

> > > be the architecture-independent header that defines these three macros and

> > > includes <bits/endian-arch.h>, where the existing <bits/endian.h> headers

> > > that define __BYTE_ORDER all get renamed to <bits/endian-arch.h>.  So

> > > you'd need to add bits/endian-arch.h to the "headers" setting in

> > > string/Makefile.

> >

> > I believe I wrote that patch already, in fact, as part of my

> > installed-headers cleanups series:

> > https://sourceware.org/ml/libc-alpha/2019-06/msg00783.html  Unlike a

> > lot of those patches, it should be low-risk for application breakage

> > and applicable independently.


That patch doesn't apply cleanly. What happened to this series? Should
I wait for it to go in or should I look at manually applying this
patch.

>

> Yes, that's the sort of thing I'd expect (and I had a comment on it in

> <https://sourceware.org/ml/libc-alpha/2019-07/msg00561.html>).  (This is

> not a review.  When retesting this patch it would be necessary to check if

> any new includes of <endian.h> have been added to installed headers that

> need updating as well.)

>

> I should explicitly note that, while it's easy to think of possible

> followup cleanups in this area, it's best *not* to combine such cleanups

> with this patch in order to keep it of a reasonable size and complexity.


Ok

Alistair

>

> --

> Joseph S. Myers

> joseph@codesourcery.com
Alistair Francis Sept. 20, 2019, 9:51 p.m. | #5
On Fri, Sep 20, 2019 at 2:05 PM Alistair Francis <alistair23@gmail.com> wrote:
>

> On Fri, Sep 20, 2019 at 9:51 AM Joseph Myers <joseph@codesourcery.com> wrote:

> >

> > On Fri, 20 Sep 2019, Zack Weinberg wrote:

> >

> > > On Thu, Sep 19, 2019 at 7:34 PM Joseph Myers <joseph@codesourcery.com> wrote:

> > > >

> > > > If anything, I think the cleanest naming would be to have <bits/endian.h>

> > > > be the architecture-independent header that defines these three macros and

> > > > includes <bits/endian-arch.h>, where the existing <bits/endian.h> headers

> > > > that define __BYTE_ORDER all get renamed to <bits/endian-arch.h>.  So

> > > > you'd need to add bits/endian-arch.h to the "headers" setting in

> > > > string/Makefile.

> > >

> > > I believe I wrote that patch already, in fact, as part of my

> > > installed-headers cleanups series:

> > > https://sourceware.org/ml/libc-alpha/2019-06/msg00783.html  Unlike a

> > > lot of those patches, it should be low-risk for application breakage

> > > and applicable independently.

>

> That patch doesn't apply cleanly. What happened to this series? Should

> I wait for it to go in or should I look at manually applying this

> patch.


It didn't turn out to be too bad to manually apply the patch.

I think I have something that is all working now. I haven't had a
chance to test it though.

Alistair

>

> >

> > Yes, that's the sort of thing I'd expect (and I had a comment on it in

> > <https://sourceware.org/ml/libc-alpha/2019-07/msg00561.html>).  (This is

> > not a review.  When retesting this patch it would be necessary to check if

> > any new includes of <endian.h> have been added to installed headers that

> > need updating as well.)

> >

> > I should explicitly note that, while it's easy to think of possible

> > followup cleanups in this area, it's best *not* to combine such cleanups

> > with this patch in order to keep it of a reasonable size and complexity.

>

> Ok

>

> Alistair

>

> >

> > --

> > Joseph S. Myers

> > joseph@codesourcery.com

Patch

diff --git a/bits/endian.h b/bits/endian.h
index 45afd4ae477..29838739049 100644
--- a/bits/endian.h
+++ b/bits/endian.h
@@ -6,7 +6,7 @@ 
    So if cross-compiling to a machine with a different byte order,
    the bits/endian.h file for that machine must exist.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/string/endian.h b/string/endian.h
index 17e4e893fb5..e23484c1f0f 100644
--- a/string/endian.h
+++ b/string/endian.h
@@ -19,21 +19,7 @@ 
 #define	_ENDIAN_H	1
 
 #include <features.h>
-
-/* Definitions for byte order, according to significance of bytes,
-   from low addresses to high addresses.  The value is what you get by
-   putting '4' in the most significant byte, '3' in the second most
-   significant byte, '2' in the second least significant byte, and '1'
-   in the least significant byte, and then writing down one digit for
-   each byte, starting with the byte at the lowest address at the left,
-   and proceeding to the byte with the highest address at the right.  */
-
-#define	__LITTLE_ENDIAN	1234
-#define	__BIG_ENDIAN	4321
-#define	__PDP_ENDIAN	3412
-
-/* This file defines `__BYTE_ORDER' for the particular machine.  */
-#include <bits/endian.h>
+#include <time/bits/types/endian_t.h>
 
 /* Some machines may need to use a different endianness for floating point
    values.  */
diff --git a/sysdeps/aarch64/bits/endian.h b/sysdeps/aarch64/bits/endian.h
index 03801d312a7..26ab0f556e4 100644
--- a/sysdeps/aarch64/bits/endian.h
+++ b/sysdeps/aarch64/bits/endian.h
@@ -16,7 +16,7 @@ 
    License along with the GNU C Library.  If not, see
    <https://www.gnu.org/licenses/>.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/alpha/bits/endian.h b/sysdeps/alpha/bits/endian.h
index 8a16e14e24e..df2e6380726 100644
--- a/sysdeps/alpha/bits/endian.h
+++ b/sysdeps/alpha/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* Alpha is little-endian.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/arm/bits/endian.h b/sysdeps/arm/bits/endian.h
index f49f6ab1c9b..f3d17a449a3 100644
--- a/sysdeps/arm/bits/endian.h
+++ b/sysdeps/arm/bits/endian.h
@@ -1,4 +1,4 @@ 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/csky/bits/endian.h b/sysdeps/csky/bits/endian.h
index 51df38d8f91..bfd4eaedcd0 100644
--- a/sysdeps/csky/bits/endian.h
+++ b/sysdeps/csky/bits/endian.h
@@ -1,4 +1,4 @@ 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/hppa/bits/endian.h b/sysdeps/hppa/bits/endian.h
index 585db0c0faa..c1c6b23aff4 100644
--- a/sysdeps/hppa/bits/endian.h
+++ b/sysdeps/hppa/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* hppa1.1 big-endian.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/m68k/bits/endian.h b/sysdeps/m68k/bits/endian.h
index bf4ecb60a4c..f3edd102a28 100644
--- a/sysdeps/m68k/bits/endian.h
+++ b/sysdeps/m68k/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* m68k is big-endian.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/microblaze/bits/endian.h b/sysdeps/microblaze/bits/endian.h
index 6e961a55474..6e19d2c5dff 100644
--- a/sysdeps/microblaze/bits/endian.h
+++ b/sysdeps/microblaze/bits/endian.h
@@ -16,7 +16,7 @@ 
    License along with the GNU C Library.  If not, see
    <https://www.gnu.org/licenses/>.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/mips/bits/endian.h b/sysdeps/mips/bits/endian.h
index 126059799d7..12b14bc5aae 100644
--- a/sysdeps/mips/bits/endian.h
+++ b/sysdeps/mips/bits/endian.h
@@ -3,7 +3,7 @@ 
    want to be able to share the installed header files between
    both, so we define __BYTE_ORDER based on GCC's predefines.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/nios2/bits/endian.h b/sysdeps/nios2/bits/endian.h
index 164f9e4d785..f83a74d0d8a 100644
--- a/sysdeps/nios2/bits/endian.h
+++ b/sysdeps/nios2/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* The Nios II architecture has selectable endianness.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/powerpc/bits/endian.h b/sysdeps/powerpc/bits/endian.h
index c82f7ad4de6..8463d17fd66 100644
--- a/sysdeps/powerpc/bits/endian.h
+++ b/sysdeps/powerpc/bits/endian.h
@@ -17,7 +17,7 @@ 
 
 /* PowerPC can be little or big endian.  Hopefully gcc will know...  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/riscv/bits/endian.h b/sysdeps/riscv/bits/endian.h
index 4aaf559d4f4..c07ea5f748f 100644
--- a/sysdeps/riscv/bits/endian.h
+++ b/sysdeps/riscv/bits/endian.h
@@ -1,4 +1,4 @@ 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/s390/bits/endian.h b/sysdeps/s390/bits/endian.h
index ac27f0119a8..a2b93390d5a 100644
--- a/sysdeps/s390/bits/endian.h
+++ b/sysdeps/s390/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* s390 is big-endian */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/sh/bits/endian.h b/sysdeps/sh/bits/endian.h
index 1fef1ff938f..46f8fd3d3ec 100644
--- a/sysdeps/sh/bits/endian.h
+++ b/sysdeps/sh/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* SH is bi-endian but with a big-endian FPU.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/sparc/bits/endian.h b/sysdeps/sparc/bits/endian.h
index 8acfdf5df6f..60394ea43aa 100644
--- a/sysdeps/sparc/bits/endian.h
+++ b/sysdeps/sparc/bits/endian.h
@@ -1,7 +1,7 @@ 
 /* Sparc is big-endian, but v9 supports endian conversion on loads/stores
    and GCC supports such a mode.  Be prepared.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/unix/sysv/linux/ia64/bits/endian.h b/sysdeps/unix/sysv/linux/ia64/bits/endian.h
index 98a5e239916..8bb227ecd53 100644
--- a/sysdeps/unix/sysv/linux/ia64/bits/endian.h
+++ b/sysdeps/unix/sysv/linux/ia64/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* Linux/ia64 is little-endian.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/sysdeps/x86/bits/endian.h b/sysdeps/x86/bits/endian.h
index 5a56c726f71..c159872ae64 100644
--- a/sysdeps/x86/bits/endian.h
+++ b/sysdeps/x86/bits/endian.h
@@ -1,6 +1,6 @@ 
 /* i386/x86_64 are little-endian.  */
 
-#ifndef _ENDIAN_H
+#ifndef _ENDIAN_T_H
 # error "Never use <bits/endian.h> directly; include <endian.h> instead."
 #endif
 
diff --git a/time/bits/types/endian_t.h b/time/bits/types/endian_t.h
new file mode 100644
index 00000000000..d68f5ba2f8b
--- /dev/null
+++ b/time/bits/types/endian_t.h
@@ -0,0 +1,36 @@ 
+/* Copyright (C) 1992-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library 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.
+
+   The GNU C Library 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 the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef	_ENDIAN_T_H
+#define	_ENDIAN_T_H	1
+
+/* Definitions for byte order, according to significance of bytes,
+   from low addresses to high addresses.  The value is what you get by
+   putting '4' in the most significant byte, '3' in the second most
+   significant byte, '2' in the second least significant byte, and '1'
+   in the least significant byte, and then writing down one digit for
+   each byte, starting with the byte at the lowest address at the left,
+   and proceeding to the byte with the highest address at the right.  */
+
+#define	__LITTLE_ENDIAN	1234
+#define	__BIG_ENDIAN	4321
+#define	__PDP_ENDIAN	3412
+
+/* This file defines `__BYTE_ORDER' for the particular machine.  */
+#include <bits/endian.h>
+
+#endif