[PATCH] sys/types.h: Define new types: [s]nseconds_t

[PATCH] sys/types.h: Define new types: [s]nseconds_t

[Alejandro Colomar]

For symmetry with the existing [s]useconds_t types.

The timespec(3) structure uses long for tv_nsec,
except if __x86_64__ && __ILP32__,
in which case it uses long long.
Let's define a stable type that can be relied upon by users,
for example for creating pointers.

Cc: наб <nabijaczleweli@nabijaczleweli.xyz>
Cc: Jakub Wilk <jwilk@jwilk.net>
Cc: Zack Weinberg <zackw@panix.com>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 posix/sys/types.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/posix/sys/types.h b/posix/sys/types.h
index 477a45f4af..147e2d3500 100644
--- a/posix/sys/types.h
+++ b/posix/sys/types.h
@@ -140,6 +140,14 @@ typedef __suseconds_t suseconds_t;
 # endif
 #endif
 
+#if defined __x86_64__ && defined __ILP32__
+typedef unsigned long long nseconds_t;
+typedef long long snseconds_t;
+#else
+typedef unsigned long nseconds_t;
+typedef long snseconds_t;
+#endif
+
 #define	__need_size_t
 #include <stddef.h>
 
-- 
2.34.1

Re: [PATCH] sys/types.h: Define new types: [s]nseconds_t

[Andreas Schwab]

On Dez 07 2021, Alejandro Colomar via Libc-alpha wrote:

> diff --git a/posix/sys/types.h b/posix/sys/types.h
> index 477a45f4af..147e2d3500 100644
> --- a/posix/sys/types.h
> +++ b/posix/sys/types.h
> @@ -140,6 +140,14 @@ typedef __suseconds_t suseconds_t;
>  # endif
>  #endif
>  
> +#if defined __x86_64__ && defined __ILP32__
> +typedef unsigned long long nseconds_t;
> +typedef long long snseconds_t;
> +#else
> +typedef unsigned long nseconds_t;
> +typedef long snseconds_t;
> +#endif

This needs to be defined in terms of a type defined in a bits header.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] sys/types.h: Define new types: [s]nseconds_t

[наб]

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

On Tue, Dec 07, 2021 at 12:19:58PM +0100, Alejandro Colomar wrote:
> For symmetry with the existing [s]useconds_t types.
If you'd hold on to your horses, like, until the next WG14 meeting,
turns out I know a guy who knows a guy, but my guy is the WG14 project
editor, and I'm like, a day's away from getting an N number and turns
out getting your "C defect" paper through is pretty easy if you don't
have to sit in an NB meeting yourself.
(Granted, this wouldn't have to be a paper at all if this were C++,
 but this is pretty good as-is.)

You can find my current draft:
  https://srhtcdn.githack.com/~nabijaczleweli/wg14/blob/868d3e096db0fe4378d77cd67215bed56045196d/N%3F%3F%3F%3F.pdf
accepting this would mean just a
  typedef decltype(struct timespec::tv_nsec) nsec_t;
to make the implementation C2X conformant again.

Best,

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] sys/types.h: Define new types: [s]nseconds_t

[Alejandro Colomar (man-pages)]

Hi наб,

On 12/7/21 14:17, наб wrote:
> On Tue, Dec 07, 2021 at 12:19:58PM +0100, Alejandro Colomar wrote:
>> For symmetry with the existing [s]useconds_t types.
> If you'd hold on to your horses, like, until the next WG14 meeting,
> turns out I know a guy who knows a guy, but my guy is the WG14 project
> editor, and I'm like, a day's away from getting an N number and turns
> out getting your "C defect" paper through is pretty easy if you don't
> have to sit in an NB meeting yourself.
> (Granted, this wouldn't have to be a paper at all if this were C++,
>   but this is pretty good as-is.)

Sounds great! :)

I'll continue with my glibc patch as a draft, to learn how glibc types 
are defined (there's a lot of magic I still need to learn), but will 
wait for the results of your C2X proposal before merging this to glibc.

As Zack suggested, an unsigned version is probably unnecessary, so I'll 
use nsec_t for the patches, as you used in your draft.  I like short 
names, and also signed types.

> 
> You can find my current draft:
>    https://srhtcdn.githack.com/~nabijaczleweli/wg14/blob/868d3e096db0fe4378d77cd67215bed56045196d/N%3F%3F%3F%3F.pdf

It looks good.

Would you mind sharing the source code of your pdf?  I'm curious to 
learn to write those papers.

Also, please CC me when you have any news on that :)

> accepting this would mean just a
>    typedef decltype(struct timespec::tv_nsec) nsec_t;
> to make the implementation C2X conformant again.

Hmm, not sure if it would be better for glibc to define nsec_t in terms 
of timespec, or the other way around.  I had in mind something like the 
following:

Here would go many ifdefs:
typedef /* probably */ long nsec_t;


And this would be clean:
struct timespec {
     ...
     nsec_t tv_nsec;
};


Thanks,
Alex

-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

Re: [PATCH] sys/types.h: Define new types: [s]nseconds_t

[наб]

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

On Tue, Dec 07, 2021 at 07:20:07PM +0100, Alejandro Colomar (man-pages) wrote:
> As Zack suggested, an unsigned version is probably unnecessary, so I'll use
> nsec_t for the patches, as you used in your draft.  I like short names, and
> also signed types.
Great, this, if lands, will give glibc free compatibility with C2X.

> On 12/7/21 14:17, наб wrote:
> > You can find my current draft:
> >    https://srhtcdn.githack.com/~nabijaczleweli/wg14/blob/868d3e096db0fe4378d77cd67215bed56045196d/N%3F%3F%3F%3F.pdf
> It looks good.
Thanks :0

> Would you mind sharing the source code of your pdf?  I'm curious to learn to
> write those papers.
https://git.sr.ht/~nabijaczleweli/wg14/tree, but note that this is /far/
from a "good" or "normal" set-up ‒ most people use TeX,
and are proficient at it because they have a collegial education;
I started using groff -mom yesterday at 3am because I needed to make a
PDF and I'd remembered reading it was the strongest successor to -ms.

FTR: The PDF linked above was made at revision ee254af.

> Also, please CC me when you have any news on that :)
Will do.

> > accepting this would mean just a
> >    typedef decltype(struct timespec::tv_nsec) nsec_t;
> > to make the implementation C2X conformant again.
> Hmm, not sure if it would be better for glibc to define nsec_t in terms of
> timespec, or the other way around.  I had in mind something like the
> following:
I just meant that as an abstract short-hand form,
decomposing to roughly what you've out-lined below, yes.

Best,
наб

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] sys/types.h: Define new types: [s]nseconds_t

[Joseph Myers]

On Tue, 7 Dec 2021, наб via Libc-alpha wrote:

> If you'd hold on to your horses, like, until the next WG14 meeting,

I suspect we already have enough papers for the entire agenda of the next 
(two-week) meeting (indeed, it seems plausible new feature consideration 
will spill over to the July meeting and a C23 schedule delay will be 
needed).

> turns out I know a guy who knows a guy, but my guy is the WG14 project
> editor, and I'm like, a day's away from getting an N number and turns
> out getting your "C defect" paper through is pretty easy if you don't
> have to sit in an NB meeting yourself.

I think this is more like a new feature than a defect (and one that 
definitely needs to involve the Austin Group liaison, given that struct 
timespec is a feature C got from POSIX, and maybe the C++ liaison - C++ 
liaison tends to work quite efficiently if you get papers on the agendas 
for the liaison group meetings, but Austin Group liaison may be slower).  
Note also that the next version of POSIX is aligning with C17, not C23.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] sys/types.h: Define new types: [s]nseconds_t

[Zack Weinberg]

On Tue, Dec 7, 2021, at 6:19 AM, Alejandro Colomar wrote:
> For symmetry with the existing [s]useconds_t types.
>
> The timespec(3) structure uses long for tv_nsec,
> except if __x86_64__ && __ILP32__,
> in which case it uses long long.
> Let's define a stable type that can be relied upon by users,
> for example for creating pointers.

I endorse the _concept_ of this change, regardless of whether the POSIX and C committees like it.  That said, there are some technical issues with the patch as is:

> +#if defined __x86_64__ && defined __ILP32__
> +typedef unsigned long long nseconds_t;
> +typedef long long snseconds_t;
> +#else
> +typedef unsigned long nseconds_t;
> +typedef long snseconds_t;
> +#endif

As Andreas pointed out, this belongs in bits/types.h, defining __nseconds_t and __snseconds_t.  Also, the choice of underlying type should be controlled by a new macro defined by bits/wordsize.h, so that we don't have x86-specific ifdeffage in bits/types.h.

There should then be headers <bits/types/nseconds_t.h> and <bits/types/snseconds_t.h> that define the user-namespace typedefs in terms of the __-prefixed types.  <bits/types/struct_timespec.h> should include <bits/types/snseconds_t.h> and use __snseconds_t (*not* snseconds_t) in the definition of struct timespec; <sys/types.h> should include both headers.

See if you can clean up the ifdef mess in the definition of struct timespec at the same time.  It appears to me, just looking at that, that this is *not* just an issue for x86-64 with the x32 ABI.

Tangentially, should we actually have nseconds_t if nothing's going to use it?

zw

Re: [PATCH] sys/types.h: Define new types: [s]nseconds_t

[Alejandro Colomar (man-pages)]

Hi Zack,

On 12/7/21 16:50, Zack Weinberg wrote:
> On Tue, Dec 7, 2021, at 6:19 AM, Alejandro Colomar wrote:
>> For symmetry with the existing [s]useconds_t types.
>>
>> The timespec(3) structure uses long for tv_nsec,
>> except if __x86_64__ && __ILP32__,
>> in which case it uses long long.
>> Let's define a stable type that can be relied upon by users,
>> for example for creating pointers.
> 
> I endorse the _concept_ of this change, regardless of whether the POSIX and C committees like it.  That said, there are some technical issues with the patch as is:

:)

> 
>> +#if defined __x86_64__ && defined __ILP32__
>> +typedef unsigned long long nseconds_t;
>> +typedef long long snseconds_t;
>> +#else
>> +typedef unsigned long nseconds_t;
>> +typedef long snseconds_t;
>> +#endif
> 
> As Andreas pointed out, this belongs in bits/types.h, defining __nseconds_t and __snseconds_t.  Also, the choice of underlying type should be controlled by a new macro defined by bits/wordsize.h, so that we don't have x86-specific ifdeffage in bits/types.h.
> 
> There should then be headers <bits/types/nseconds_t.h> and <bits/types/snseconds_t.h> that define the user-namespace typedefs in terms of the __-prefixed types.  <bits/types/struct_timespec.h> should include <bits/types/snseconds_t.h> and use __snseconds_t (*not* snseconds_t) in the definition of struct timespec; <sys/types.h> should include both headers.
> 
> See if you can clean up the ifdef mess in the definition of struct timespec at the same time.  It appears to me, just looking at that, that this is *not* just an issue for x86-64 with the x32 ABI.

Thanks for the directions.  This way it'll be much easier.  I'll try to 
have a look at that mess ;)

> 
> Tangentially, should we actually have nseconds_t if nothing's going to use it?
> 

Hmmm, I think we can live with a single ns type.  I like signed types, 
so if nothing is using the unsigned version, we can live without it. 
I'll use the nsec_t name, as proposed by наб, for the moment.

Cheers,
Alex


-- 
Alejandro Colomar
Linux man-pages comaintainer; https://www.kernel.org/doc/man-pages/
http://www.alejandro-colomar.es/

[RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Alejandro Colomar]

Based on the existing suseconds_t type.

The timespec(3) structure uses long for tv_nsec,
except if __x86_64__ && __ILP32__,
in which case it uses long long.
Let's define a stable type that can be relied upon by users,
for example for creating pointers.

Link: linux-man <https://lore.kernel.org/linux-man/ec1dcc655184f6cdaae40ff8b7970b750434e4ef.1638123425.git.nabijaczleweli@nabijaczleweli.xyz/T/>
Link: glibc <https://sourceware.org/pipermail/libc-alpha/2021-December/133702.html>
Cc: наб <nabijaczleweli@nabijaczleweli.xyz>
Cc: Jakub Wilk <jwilk@jwilk.net>
Cc: Zack Weinberg <zackw@panix.com>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Rich Felker <dalias@libc.org>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---

Hi,

For this RFC I used snseconds_t for simplicity writing the patch,
and it helps reviewing it too I think,
but it can easily be changed later at once.

I'd like some feedback on naming the type nsec_t vs snseconds_t.
snseconds_t is more similar to the existing suseconds_t type.
However, since there's no unsigned version,
having the 's' prefix is slightly weird (not that much,
but nsec_t is shorter, and still very readable, in line with C tradition).
And if the prefix wasn't there,
it would be even worse, since it would create confusion.
nsec_t is similar to tv_nsec.

 bits/typesizes.h                                 | 1 +
 posix/bits/types.h                               | 1 +
 posix/sys/types.h                                | 5 +++++
 sysdeps/mach/hurd/bits/typesizes.h               | 1 +
 sysdeps/unix/sysv/linux/alpha/bits/typesizes.h   | 1 +
 sysdeps/unix/sysv/linux/generic/bits/typesizes.h | 1 +
 sysdeps/unix/sysv/linux/s390/bits/typesizes.h    | 1 +
 sysdeps/unix/sysv/linux/sparc/bits/typesizes.h   | 1 +
 sysdeps/unix/sysv/linux/x86/bits/typesizes.h     | 1 +
 9 files changed, 13 insertions(+)

diff --git a/bits/typesizes.h b/bits/typesizes.h
index 63564b2fa1..85c093fabb 100644
--- a/bits/typesizes.h
+++ b/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/posix/bits/types.h b/posix/bits/types.h
index 2dc63de5c2..c4107f1458 100644
--- a/posix/bits/types.h
+++ b/posix/bits/types.h
@@ -161,6 +161,7 @@ __STD_TYPE __TIME_T_TYPE __time_t;	/* Seconds since the Epoch.  */
 __STD_TYPE __USECONDS_T_TYPE __useconds_t; /* Count of microseconds.  */
 __STD_TYPE __SUSECONDS_T_TYPE __suseconds_t; /* Signed count of microseconds.  */
 __STD_TYPE __SUSECONDS64_T_TYPE __suseconds64_t;
+__STD_TYPE __SNSECONDS_T_TYPE __snseconds_t; /* Signed count of nanoseconds.  */
 
 __STD_TYPE __DADDR_T_TYPE __daddr_t;	/* The type of a disk address.  */
 __STD_TYPE __KEY_T_TYPE __key_t;	/* Type of an IPC key.  */
diff --git a/posix/sys/types.h b/posix/sys/types.h
index 477a45f4af..e8fe2d1ba6 100644
--- a/posix/sys/types.h
+++ b/posix/sys/types.h
@@ -140,6 +140,11 @@ typedef __suseconds_t suseconds_t;
 # endif
 #endif
 
+#ifndef __snseconds_t_defined
+typedef __snseconds_t snseconds_t;
+# define __snseconds_t_defined
+#endif
+
 #define	__need_size_t
 #include <stddef.h>
 
diff --git a/sysdeps/mach/hurd/bits/typesizes.h b/sysdeps/mach/hurd/bits/typesizes.h
index 36adbe09c4..558f9ee597 100644
--- a/sysdeps/mach/hurd/bits/typesizes.h
+++ b/sysdeps/mach/hurd/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h b/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h
index 9bdc925168..d17131cf25 100644
--- a/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h
@@ -50,6 +50,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__S64_TYPE
 #define __SUSECONDS64_T_TYPE	__S64_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/generic/bits/typesizes.h b/sysdeps/unix/sysv/linux/generic/bits/typesizes.h
index c658dfdf60..bb5276b103 100644
--- a/sysdeps/unix/sysv/linux/generic/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/generic/bits/typesizes.h
@@ -64,6 +64,7 @@
 #define __CLOCK_T_TYPE		__SLONGWORD_TYPE
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/s390/bits/typesizes.h b/sysdeps/unix/sysv/linux/s390/bits/typesizes.h
index 82e77e1e03..c2ba225750 100644
--- a/sysdeps/unix/sysv/linux/s390/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/s390/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h b/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h
index 7aaca9757d..cebc057109 100644
--- a/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__S32_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/x86/bits/typesizes.h b/sysdeps/unix/sysv/linux/x86/bits/typesizes.h
index 060af056f8..e668755fca 100644
--- a/sysdeps/unix/sysv/linux/x86/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/x86/bits/typesizes.h
@@ -65,6 +65,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SYSCALL_SLONG_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SYSCALL_SLONG_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
-- 
2.34.1

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Joseph Myers]

On Tue, 7 Dec 2021, Alejandro Colomar via Libc-alpha wrote:

> For this RFC I used snseconds_t for simplicity writing the patch,
> and it helps reviewing it too I think,
> but it can easily be changed later at once.
> 
> I'd like some feedback on naming the type nsec_t vs snseconds_t.

My suggestion would be to do things *without* having such a user-visible 
name at all, just the __*_t name (which arguably makes sense as an 
internal cleanup anyway, given that we need to handle the peculiarities of 
the x32 kernel ABI).  The choice of user-visible name, if any, could be 
considered separately and in conjunction with WG14 / WG21 / Austin Group 
consideration of the issue.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Paul Eggert]

On 12/7/21 15:56, Joseph Myers wrote:

> My suggestion would be to do things *without* having such a user-visible
> name at all, just the __*_t name (which arguably makes sense as an
> internal cleanup anyway, given that we need to handle the peculiarities of
> the x32 kernel ABI).

I also like this suggestion.

I've written quite a bit of user code that uses struct timespec, and 
I've never felt a need for a user-visible name for tv_nsec's type.

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Rich Felker]

On Tue, Dec 07, 2021 at 10:48:14PM +0100, Alejandro Colomar wrote:
> Based on the existing suseconds_t type.
> 
> The timespec(3) structure uses long for tv_nsec,
> except if __x86_64__ && __ILP32__,
> in which case it uses long long.
> Let's define a stable type that can be relied upon by users,
> for example for creating pointers.

This is a non-starter because the relevant standards already require
the tv_nsec member to have type long. x32 just needs to be fixed to
match the requirement. This is really easy to do; just change the
definition to have the appropriate padding. Don't make a huge mess for
the sake of a barely-used target that just had a messy but fixable
bug.

Rich

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Zack Weinberg]

On Tue, Dec 7, 2021, at 7:29 PM, Rich Felker wrote:
> On Tue, Dec 07, 2021 at 10:48:14PM +0100, Alejandro Colomar wrote:
>> Based on the existing suseconds_t type.
>> 
>> The timespec(3) structure uses long for tv_nsec,
>> except if __x86_64__ && __ILP32__,
>> in which case it uses long long.
>> Let's define a stable type that can be relied upon by users,
>> for example for creating pointers.
>
> This is a non-starter because the relevant standards already require
> the tv_nsec member to have type long.

That requirement is a defect in the standards, and I see no reason why this particular defect should be granted 'we're stuck with this' status.

> x32 just needs to be fixed to match the requirement.

This doesn't just affect x32, it affects every ABI with 32-bit long and 64-bit time_t.  Look at the ifdef mess in glibc bits/types/struct_timespec.h if you don't believe me.

zw

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Rich Felker]

On Tue, Dec 07, 2021 at 09:26:59PM -0500, Zack Weinberg wrote:
> On Tue, Dec 7, 2021, at 7:29 PM, Rich Felker wrote:
> > On Tue, Dec 07, 2021 at 10:48:14PM +0100, Alejandro Colomar wrote:
> >> Based on the existing suseconds_t type.
> >> 
> >> The timespec(3) structure uses long for tv_nsec,
> >> except if __x86_64__ && __ILP32__,
> >> in which case it uses long long.
> >> Let's define a stable type that can be relied upon by users,
> >> for example for creating pointers.
> >
> > This is a non-starter because the relevant standards already require
> > the tv_nsec member to have type long.
> 
> That requirement is a defect in the standards, and I see no reason
> why this particular defect should be granted 'we're stuck with this'
> status.

When the standard codifies existing *universal* practice without
introducing a gratuitous typedef, then the Linux kernel folks fail to
read it and make a weird new ABI that breaks it and the glibc folks
copy that non-critically (rather than doing the obvious padding
thing), it's not a defect in the standard. It's a defect in the review
process on the Linux and glibc side.

> > x32 just needs to be fixed to match the requirement.
> 
> This doesn't just affect x32, it affects every ABI with 32-bit long
> and 64-bit time_t. Look at the ifdef mess in glibc
> bits/types/struct_timespec.h if you don't believe me.

They all got it right -- tv_nsec has type long (32-bit). There is
nothing affected by the issue described here except x32, and it's
easily fixed. We've always had it right on musl x32 too.

Rich

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Zack Weinberg]

On Tue, Dec 7, 2021, at 10:05 PM, Rich Felker wrote:
> On Tue, Dec 07, 2021 at 09:26:59PM -0500, Zack Weinberg wrote:
>> On Tue, Dec 7, 2021, at 7:29 PM, Rich Felker wrote:
>> >
>> > This is a non-starter because the relevant standards already require
>> > the tv_nsec member to have type long.
>> 
>> That requirement is a defect in the standards, and I see no reason
>> why this particular defect should be granted 'we're stuck with this'
>> status.
>
> When the standard codifies existing *universal* practice without
> introducing a gratuitous typedef

Universal practice is not necessarily correct.  It was an error to define a structure type that's passed across the user/kernel boundary, with a field whose type isn't a typedef name.  It has *always* been an error.  And I have yet to see a compelling reason why the error should not be corrected.

>> > x32 just needs to be fixed to match the requirement.
>> 
>> This doesn't just affect x32, it affects every ABI with 32-bit long
>> and 64-bit time_t. Look at the ifdef mess in glibc
>> bits/types/struct_timespec.h if you don't believe me.
>
> They all got it right -- tv_nsec has type long (32-bit). There is
> nothing affected by the issue described here except x32, and it's
> easily fixed. We've always had it right on musl x32 too.

I looked through the source code to musl and I could not find the definition of struct timespec.  It appears that it's supposed to be defined by <bits/alltypes.h> but that's a generated file and the only piece of the generator I could find (tools/mkalltypes.sed) doesn't seem to have code to emit a definition of struct timespec, but it's not in arch/*/bits/alltypes.h.in either.  Where should I be looking?

Having said that, I don't actually _care_ whether the spec bug can be papered over with sufficient cleverness in the user-side definition of struct timespec.  It is still a bug in the spec.

zw

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Rich Felker]

On Wed, Dec 08, 2021 at 09:34:39AM -0500, Zack Weinberg wrote:
> On Tue, Dec 7, 2021, at 10:05 PM, Rich Felker wrote:
> > On Tue, Dec 07, 2021 at 09:26:59PM -0500, Zack Weinberg wrote:
> >> On Tue, Dec 7, 2021, at 7:29 PM, Rich Felker wrote:
> >> >
> >> > This is a non-starter because the relevant standards already require
> >> > the tv_nsec member to have type long.
> >> 
> >> That requirement is a defect in the standards, and I see no reason
> >> why this particular defect should be granted 'we're stuck with this'
> >> status.
> >
> > When the standard codifies existing *universal* practice without
> > introducing a gratuitous typedef
> 
> Universal practice is not necessarily correct. It was an error to
> define a structure type that's passed across the user/kernel
> boundary, with a field whose type isn't a typedef name. It has
> *always* been an error. And I have yet to see a compelling reason
> why the error should not be corrected.

This is your ideology. I don't agree with it, and I hope others won't
too. Stop treating it as Truth folks are supposed to automatically
agree with you on.

> >> > x32 just needs to be fixed to match the requirement.
> >> 
> >> This doesn't just affect x32, it affects every ABI with 32-bit long
> >> and 64-bit time_t. Look at the ifdef mess in glibc
> >> bits/types/struct_timespec.h if you don't believe me.
> >
> > They all got it right -- tv_nsec has type long (32-bit). There is
> > nothing affected by the issue described here except x32, and it's
> > easily fixed. We've always had it right on musl x32 too.
> 
> I looked through the source code to musl and I could not find the
> definition of struct timespec. It appears that it's supposed to be
> defined by <bits/alltypes.h> but that's a generated file and the
> only piece of the generator I could find (tools/mkalltypes.sed)
> doesn't seem to have code to emit a definition of struct timespec,
> but it's not in arch/*/bits/alltypes.h.in either. Where should I be
> looking?

include/alltypes.h.in is also used, and contains:

STRUCT timespec { time_t tv_sec; int :8*(sizeof(time_t)-sizeof(long))*(__BYTE_ORDER==4321); long tv_nsec; int :8*(sizeof(time_t)-sizeof(long))*(__BYTE_ORDER!=4321); };

which is just time_t tv_sec and long tv_nsec surrounded by the
appropriate anonymous padding for endianness and sizeof(long).

The same could be achieved writing it out explicitly for the different
options but we wanted it to be automatically consistent using a single
definition.

> Having said that, I don't actually _care_ whether the spec bug can
> be papered over with sufficient cleverness in the user-side
> definition of struct timespec. It is still a bug in the spec.

No "cleverness" is needed here. Nothing at all would be needed if the
kernel weren't trying to be "clever" and match the layout between
32-bit and 64-bit archs; since it is, you just need padding in the
right place to match that. You're making a huge meal over something
that is incredibly simple.

Rich

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Zack Weinberg]

On Wed, Dec 8, 2021, at 12:38 PM, Rich Felker wrote:
> On Wed, Dec 08, 2021 at 09:34:39AM -0500, Zack Weinberg wrote:
>> Universal practice is not necessarily correct. It was an error to
>> define a structure type that's passed across the user/kernel
>> boundary, with a field whose type isn't a typedef name. It has
>> *always* been an error. And I have yet to see a compelling reason
>> why the error should not be corrected.
>
> This is your ideology. I don't agree with it, and I hope others won't
> too. Stop treating it as Truth folks are supposed to automatically
> agree with you on.

We have objective evidence that using a bare fundamental type for this field causes problems.  You want to convince me that its type should not be changed, you should be offering actual _reasons_ to not change it, not just insisting that it's fine as is.

Actual reasons, in my book, would include things like a real-world program that fails to compile in a way that cannot be fixed with local patches, or -- much more seriously -- a real-world program that recompiles fine but then produces incorrect output when run.

>> I looked through the source code to musl and I could not find the
>> definition of struct timespec
...
>
> include/alltypes.h.in is also used, and contains:
>
> STRUCT timespec { time_t tv_sec; int 
> :8*(sizeof(time_t)-sizeof(long))*(__BYTE_ORDER==4321); long tv_nsec; 
> int :8*(sizeof(time_t)-sizeof(long))*(__BYTE_ORDER!=4321); };
>
> which is just time_t tv_sec and long tv_nsec surrounded by the
> appropriate anonymous padding for endianness and sizeof(long).

Ah, I see how it works now, thanks.

zw

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Adhemerval Zanella]

On 08/12/2021 11:34, Zack Weinberg via Libc-alpha wrote:
> On Tue, Dec 7, 2021, at 10:05 PM, Rich Felker wrote:
>> On Tue, Dec 07, 2021 at 09:26:59PM -0500, Zack Weinberg wrote:
>>> On Tue, Dec 7, 2021, at 7:29 PM, Rich Felker wrote:
>>>>
>>>> This is a non-starter because the relevant standards already require
>>>> the tv_nsec member to have type long.
>>>
>>> That requirement is a defect in the standards, and I see no reason
>>> why this particular defect should be granted 'we're stuck with this'
>>> status.
>>
>> When the standard codifies existing *universal* practice without
>> introducing a gratuitous typedef
> 
> Universal practice is not necessarily correct.  It was an error to define a structure type that's passed across the user/kernel boundary, with a field whose type isn't a typedef name.  It has *always* been an error.  And I have yet to see a compelling reason why the error should not be corrected.
> 
>>>> x32 just needs to be fixed to match the requirement.
>>>
>>> This doesn't just affect x32, it affects every ABI with 32-bit long
>>> and 64-bit time_t. Look at the ifdef mess in glibc
>>> bits/types/struct_timespec.h if you don't believe me.
>>
>> They all got it right -- tv_nsec has type long (32-bit). There is
>> nothing affected by the issue described here except x32, and it's
>> easily fixed. We've always had it right on musl x32 too.
> 
> I looked through the source code to musl and I could not find the definition of struct timespec.  It appears that it's supposed to be defined by <bits/alltypes.h> but that's a generated file and the only piece of the generator I could find (tools/mkalltypes.sed) doesn't seem to have code to emit a definition of struct timespec, but it's not in arch/*/bits/alltypes.h.in either.  Where should I be looking?
> 
> Having said that, I don't actually _care_ whether the spec bug can be papered over with sufficient cleverness in the user-side definition of struct timespec.  It is still a bug in the spec.
> 
> zw

So if I understood correctly, this change does not really fix or improve
the current code, but rather adds a potentially type issue (specially
if used along with more 'strong' typed languages like C++) just for the
sake of 'symmetry'?

Also, the C2X draft paper does not seems to address a real issue, since 
it mixes a kernel ABI with libc exported interface.  In fact, I complete
agree with Rich here, it is really unfortunated that glibc x32 exported
a different tv_nsec type (not sure if we can fix it now without breaking
anything).


> This doesn't just affect x32, it affects every ABI with 32-bit long and 64-bit time_t.  Look at the ifdef mess in glibc bits/types/struct_timespec.h if you don't believe me.

The preprocessor mess is mostly because 1. x32 decided to use a 
different type than the spec and 2. we support multiple time_t size
through _TIME_BITS (where old interface did not have the padding).

If weren't bound by both we could use a more simplified way like
musl:

  struct timespec 
  { 
    time_t tv_sec; 
  #if __BYTE_ORDER == __BIG_ENDIAN
    int: 32;           /* Padding.  */
    long int tv_nsec;  /* Nanoseconds.  */
  #else
    long int tv_nsec;  /* Nanoseconds.  */
    int: 32;           /* Padding.  */
  #endif
  };

(since time_t would be always 64-bit).

Re: [RFC v2 1/2] sys/types.h: Define new type: snseconds_t

[Joseph Myers]

On Wed, 8 Dec 2021, Zack Weinberg via Libc-alpha wrote:

> On Tue, Dec 7, 2021, at 10:05 PM, Rich Felker wrote:
> > On Tue, Dec 07, 2021 at 09:26:59PM -0500, Zack Weinberg wrote:
> >> On Tue, Dec 7, 2021, at 7:29 PM, Rich Felker wrote:
> >> >
> >> > This is a non-starter because the relevant standards already require
> >> > the tv_nsec member to have type long.
> >> 
> >> That requirement is a defect in the standards, and I see no reason
> >> why this particular defect should be granted 'we're stuck with this'
> >> status.
> >
> > When the standard codifies existing *universal* practice without
> > introducing a gratuitous typedef
> 
> Universal practice is not necessarily correct.  It was an error to 
> define a structure type that's passed across the user/kernel boundary, 

"user/kernel boundary" is not a concept relevant to the standards; the 
standards deal with the *implementation* as a whole, all parts of which 
are expected to work together to implement features.

It was a defect when older XPG standards specified "long" for some fields 
in struct stat, because having st_blocks (for example) outside the range 
of 32-bit long is useful.  In the case of tv_nsec, all valid values fit 
within the range of 32-bit long and so that argument does not apply.

-- 
Joseph S. Myers
joseph@codesourcery.com

[RFC v2 2/2] sys/types.h: struct timespec: Use snseconds_t for tv_nsec

[Alejandro Colomar]

The timespec(3) structure uses long for tv_nsec,
except if __x86_64__ && __ILP32__,
in which case it uses long long.
Let's use a stable type that can be relied upon by users,
for example for creating pointers.

Link: linux-man <https://lore.kernel.org/linux-man/ec1dcc655184f6cdaae40ff8b7970b750434e4ef.1638123425.git.nabijaczleweli@nabijaczleweli.xyz/T/>
Link: glibc <https://sourceware.org/pipermail/libc-alpha/2021-December/133702.html>
Cc: наб <nabijaczleweli@nabijaczleweli.xyz>
Cc: Jakub Wilk <jwilk@jwilk.net>
Cc: Zack Weinberg <zackw@panix.com>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Rich Felker <dalias@libc.org>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 time/bits/types/struct_timespec.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
index 489e81136d..33dda4db2a 100644
--- a/time/bits/types/struct_timespec.h
+++ b/time/bits/types/struct_timespec.h
@@ -18,14 +18,14 @@ struct timespec
 #if __WORDSIZE == 64 \
   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
   || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
-  __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
+  __snseconds_t tv_nsec;  /* Nanoseconds.  */
 #else
 # if __BYTE_ORDER == __BIG_ENDIAN
-  int: 32;           /* Padding.  */
-  long int tv_nsec;  /* Nanoseconds.  */
+  int: 32;                /* Padding.  */
+  __snseconds_t tv_nsec;  /* Nanoseconds.  */
 # else
-  long int tv_nsec;  /* Nanoseconds.  */
-  int: 32;           /* Padding.  */
+  __snseconds_t tv_nsec;  /* Nanoseconds.  */
+  int: 32;                /* Padding.  */
 # endif
 #endif
 };
-- 
2.34.1

[RFC v3 1/3] bits/types[izes].h: Define new internal type: __snseconds_t

[Alejandro Colomar]

Based on the existing suseconds_t type,
with the difference that __snseconds_t should be long when possible.

The timespec(3) structure uses long for tv_nsec,
except if __x86_64__ && __ILP32__,
in which case it uses long long.

Add __snseconds_t to allow simplifying the definition of struct timespec.

Link: linux-man <https://lore.kernel.org/linux-man/ec1dcc655184f6cdaae40ff8b7970b750434e4ef.1638123425.git.nabijaczleweli@nabijaczleweli.xyz/T/>
Link: glibc <https://sourceware.org/pipermail/libc-alpha/2021-December/133702.html>
Cc: наб <nabijaczleweli@nabijaczleweli.xyz>
Cc: Jakub Wilk <jwilk@jwilk.net>
Cc: Zack Weinberg <zackw@panix.com>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Rich Felker <dalias@libc.org>
Cc: Andreas Schwab <schwab@linux-m68k.org>
Cc: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 bits/typesizes.h                                 | 1 +
 posix/bits/types.h                               | 1 +
 sysdeps/mach/hurd/bits/typesizes.h               | 1 +
 sysdeps/unix/sysv/linux/alpha/bits/typesizes.h   | 1 +
 sysdeps/unix/sysv/linux/generic/bits/typesizes.h | 1 +
 sysdeps/unix/sysv/linux/s390/bits/typesizes.h    | 1 +
 sysdeps/unix/sysv/linux/sparc/bits/typesizes.h   | 1 +
 sysdeps/unix/sysv/linux/x86/bits/typesizes.h     | 1 +
 8 files changed, 8 insertions(+)

diff --git a/bits/typesizes.h b/bits/typesizes.h
index 63564b2fa1..85c093fabb 100644
--- a/bits/typesizes.h
+++ b/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/posix/bits/types.h b/posix/bits/types.h
index 2dc63de5c2..c4107f1458 100644
--- a/posix/bits/types.h
+++ b/posix/bits/types.h
@@ -161,6 +161,7 @@ __STD_TYPE __TIME_T_TYPE __time_t;	/* Seconds since the Epoch.  */
 __STD_TYPE __USECONDS_T_TYPE __useconds_t; /* Count of microseconds.  */
 __STD_TYPE __SUSECONDS_T_TYPE __suseconds_t; /* Signed count of microseconds.  */
 __STD_TYPE __SUSECONDS64_T_TYPE __suseconds64_t;
+__STD_TYPE __SNSECONDS_T_TYPE __snseconds_t; /* Signed count of nanoseconds.  */
 
 __STD_TYPE __DADDR_T_TYPE __daddr_t;	/* The type of a disk address.  */
 __STD_TYPE __KEY_T_TYPE __key_t;	/* Type of an IPC key.  */
diff --git a/sysdeps/mach/hurd/bits/typesizes.h b/sysdeps/mach/hurd/bits/typesizes.h
index 36adbe09c4..558f9ee597 100644
--- a/sysdeps/mach/hurd/bits/typesizes.h
+++ b/sysdeps/mach/hurd/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h b/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h
index 9bdc925168..d17131cf25 100644
--- a/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/alpha/bits/typesizes.h
@@ -50,6 +50,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__S64_TYPE
 #define __SUSECONDS64_T_TYPE	__S64_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/generic/bits/typesizes.h b/sysdeps/unix/sysv/linux/generic/bits/typesizes.h
index c658dfdf60..bb5276b103 100644
--- a/sysdeps/unix/sysv/linux/generic/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/generic/bits/typesizes.h
@@ -64,6 +64,7 @@
 #define __CLOCK_T_TYPE		__SLONGWORD_TYPE
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/s390/bits/typesizes.h b/sysdeps/unix/sysv/linux/s390/bits/typesizes.h
index 82e77e1e03..c2ba225750 100644
--- a/sysdeps/unix/sysv/linux/s390/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/s390/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h b/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h
index 7aaca9757d..cebc057109 100644
--- a/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/sparc/bits/typesizes.h
@@ -51,6 +51,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__S32_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SLONGWORD_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
diff --git a/sysdeps/unix/sysv/linux/x86/bits/typesizes.h b/sysdeps/unix/sysv/linux/x86/bits/typesizes.h
index 060af056f8..e668755fca 100644
--- a/sysdeps/unix/sysv/linux/x86/bits/typesizes.h
+++ b/sysdeps/unix/sysv/linux/x86/bits/typesizes.h
@@ -65,6 +65,7 @@
 #define __USECONDS_T_TYPE	__U32_TYPE
 #define __SUSECONDS_T_TYPE	__SYSCALL_SLONG_TYPE
 #define __SUSECONDS64_T_TYPE	__SQUAD_TYPE
+#define __SNSECONDS_T_TYPE	__SYSCALL_SLONG_TYPE
 #define __DADDR_T_TYPE		__S32_TYPE
 #define __KEY_T_TYPE		__S32_TYPE
 #define __CLOCKID_T_TYPE	__S32_TYPE
-- 
2.34.1

[RFC v3 2/3] sys/types.h: struct timespec: Use __snseconds_t for tv_nsec

[Alejandro Colomar]

The timespec(3) structure uses long for tv_nsec,
except if __x86_64__ && __ILP32__,
in which case it uses long long.

Use __snseconds_t to simplify the definition of struct timespec.

Link: linux-man <https://lore.kernel.org/linux-man/ec1dcc655184f6cdaae40ff8b7970b750434e4ef.1638123425.git.nabijaczleweli@nabijaczleweli.xyz/T/>
Link: glibc <https://sourceware.org/pipermail/libc-alpha/2021-December/133702.html>
Cc: наб <nabijaczleweli@nabijaczleweli.xyz>
Cc: Jakub Wilk <jwilk@jwilk.net>
Cc: Zack Weinberg <zackw@panix.com>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Rich Felker <dalias@libc.org>
Cc: Andreas Schwab <schwab@linux-m68k.org>
Cc: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 time/bits/types/struct_timespec.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
index 489e81136d..cc366590d9 100644
--- a/time/bits/types/struct_timespec.h
+++ b/time/bits/types/struct_timespec.h
@@ -18,14 +18,14 @@ struct timespec
 #if __WORDSIZE == 64 \
   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
   || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
-  __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
+  __snseconds_t tv_nsec;  /* Nanoseconds.  */
 #else
 # if __BYTE_ORDER == __BIG_ENDIAN
-  int: 32;           /* Padding.  */
-  long int tv_nsec;  /* Nanoseconds.  */
+  int: 32;                /* Padding.  */
+  __snseconds_t tv_nsec;  /* Nanoseconds.  */
 # else
-  long int tv_nsec;  /* Nanoseconds.  */
-  int: 32;           /* Padding.  */
+  __snseconds_t tv_nsec;  /* Nanoseconds.  */
+  int: 32;                /* Padding.  */
 # endif
 #endif
 };
-- 
2.34.1

Re: [RFC v3 2/3] sys/types.h: struct timespec: Use __snseconds_t for tv_nsec

[Zack Weinberg]

On Wed, Dec 8, 2021, at 9:47 AM, Alejandro Colomar wrote:
>
> Use __snseconds_t to simplify the definition of struct timespec.
...
>  #if __WORDSIZE == 64 \
>    || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
>    || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
> -  __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
> +  __snseconds_t tv_nsec;  /* Nanoseconds.  */

All my grousing about spec bugs aside, the _point_ of having tv_nsec's type be a typedef is that it ought to be possible to make this preprocessor conditional (and the rest of the cases in the same if-else chain) less hairy.  Can you look into that please?

zw

Re: [RFC v3 2/3] sys/types.h: struct timespec: Use __snseconds_t for tv_nsec

[Alejandro Colomar (man-pages)]

On 12/8/21 15:53, Zack Weinberg wrote:
> On Wed, Dec 8, 2021, at 9:47 AM, Alejandro Colomar wrote:
>>
>> Use __snseconds_t to simplify the definition of struct timespec.
> ...
>>   #if __WORDSIZE == 64 \
>>     || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
>>     || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
>> -  __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
>> +  __snseconds_t tv_nsec;  /* Nanoseconds.  */
> 
> All my grousing about spec bugs aside, the _point_ of having tv_nsec's type be a typedef is that it ought to be possible to make this preprocessor conditional (and the rest of the cases in the same if-else chain) less hairy.  Can you look into that please?

Yes, that's what I'd like to achieve in the end too.  However, I first 
wanted to make sure that I get the definition of snseconds_t right.  I'd 
remove those ifdefs in a 4th patch (definitely in a separate patch from 
this one changing the type), so that each one is clearly understandable.

Can you please confirm if this change by itself is correct for all cases?

Thanks,
Alex


-- 
Alejandro Colomar
Linux man-pages maintainer; https://www.kernel.org/doc/man-pages/

Re: [RFC v3 2/3] sys/types.h: struct timespec: Use __snseconds_t for tv_nsec

[Joseph Myers]

On Wed, 8 Dec 2021, Zack Weinberg wrote:

> On Wed, Dec 8, 2021, at 9:47 AM, Alejandro Colomar wrote:
> >
> > Use __snseconds_t to simplify the definition of struct timespec.
> ...
> >  #if __WORDSIZE == 64 \
> >    || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
> >    || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
> > -  __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
> > +  __snseconds_t tv_nsec;  /* Nanoseconds.  */
> 
> All my grousing about spec bugs aside, the _point_ of having tv_nsec's 
> type be a typedef is that it ought to be possible to make this 
> preprocessor conditional (and the rest of the cases in the same if-else 
> chain) less hairy.  Can you look into that please?

The question about whether there is padding around tv_nsec is a separate 
one to what the type of tv_nsec is.  I don't see how you can avoid the 
conditionals related to padding, which is what these conditionals are 
about.  And the preprocessor doesn't readily lend itself to saying "if 
tv_sec is 64-bit and tv_nsec is 32-bit" to describe when padding is 
involved.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC v3 2/3] sys/types.h: struct timespec: Use __snseconds_t for tv_nsec

[Alejandro Colomar (man-pages)]

On 12/8/21 16:24, Joseph Myers wrote:
> On Wed, 8 Dec 2021, Zack Weinberg wrote:
> 
>> On Wed, Dec 8, 2021, at 9:47 AM, Alejandro Colomar wrote:
>>>
>>> Use __snseconds_t to simplify the definition of struct timespec.
>> ...
>>>   #if __WORDSIZE == 64 \
>>>     || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
>>>     || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
>>> -  __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
>>> +  __snseconds_t tv_nsec;  /* Nanoseconds.  */
>>
>> All my grousing about spec bugs aside, the _point_ of having tv_nsec's
>> type be a typedef is that it ought to be possible to make this
>> preprocessor conditional (and the rest of the cases in the same if-else
>> chain) less hairy.  Can you look into that please?
> 
> The question about whether there is padding around tv_nsec is a separate
> one to what the type of tv_nsec is.  I don't see how you can avoid the
> conditionals related to padding, which is what these conditionals are
> about.  And the preprocessor doesn't readily lend itself to saying "if
> tv_sec is 64-bit and tv_nsec is 32-bit" to describe when padding is
> involved.
> 

How about defining _WIDTH macros for integral types (mirroring C2X 
_WIDTH macros)?

That way we could make use of them to make these ifdefs simpler.

I guess it would be something like

#if __SNSECONDS_WIDTH == 32 && __TIME_WIDTH == 64
...

Would this make sense?

Thanks,
Alex


-- 
Alejandro Colomar
Linux man-pages maintainer; https://www.kernel.org/doc/man-pages/

Re: [RFC v3 2/3] sys/types.h: struct timespec: Use __snseconds_t for tv_nsec

[Zack Weinberg]

On Wed, Dec 8, 2021, at 10:24 AM, Joseph Myers wrote:
> On Wed, 8 Dec 2021, Zack Weinberg wrote:
>> All my grousing about spec bugs aside, the _point_ of having tv_nsec's 
>> type be a typedef is that it ought to be possible to make this 
>> preprocessor conditional (and the rest of the cases in the same if-else 
>> chain) less hairy.  Can you look into that please?
>
> The question about whether there is padding around tv_nsec is a separate 
> one to what the type of tv_nsec is.  I don't see how you can avoid the 
> conditionals related to padding, which is what these conditionals are 
> about.  And the preprocessor doesn't readily lend itself to saying "if 
> tv_sec is 64-bit and tv_nsec is 32-bit" to describe when padding is 
> involved.

I'm hoping we can get to something like this

    struct timespec {
        __time_t tv_sec;
    #if __TIMESPEC_PAD_BEFORE_TV_NSEC
        int : __TIMESPEC_PAD_BEFORE_TV_NSEC;
    #endif
        __snseconds_t tv_nsec;
    };

where __TIMESPEC_PAD_BEFORE_TV_NSEC is defined in some appropriate sysdeps header (bits/wordsize.h is the first thing to come to mind but I'm not sure it's optimal).  Maybe we'd also need __TIMESPEC_PAD_AFTER_TV_NSEC, if the alignment requirement of __time_t is insufficient to make the compiler insert tail padding on some architectures.

zw

Re: [RFC v3 2/3] sys/types.h: struct timespec: Use __snseconds_t for tv_nsec

[Rich Felker]

On Wed, Dec 08, 2021 at 10:59:04AM -0500, Zack Weinberg wrote:
> On Wed, Dec 8, 2021, at 10:24 AM, Joseph Myers wrote:
> > On Wed, 8 Dec 2021, Zack Weinberg wrote:
> >> All my grousing about spec bugs aside, the _point_ of having tv_nsec's 
> >> type be a typedef is that it ought to be possible to make this 
> >> preprocessor conditional (and the rest of the cases in the same if-else 
> >> chain) less hairy.  Can you look into that please?
> >
> > The question about whether there is padding around tv_nsec is a separate 
> > one to what the type of tv_nsec is.  I don't see how you can avoid the 
> > conditionals related to padding, which is what these conditionals are 
> > about.  And the preprocessor doesn't readily lend itself to saying "if 
> > tv_sec is 64-bit and tv_nsec is 32-bit" to describe when padding is 
> > involved.
> 
> I'm hoping we can get to something like this
> 
>     struct timespec {
>         __time_t tv_sec;
>     #if __TIMESPEC_PAD_BEFORE_TV_NSEC
>         int : __TIMESPEC_PAD_BEFORE_TV_NSEC;
>     #endif
>         __snseconds_t tv_nsec;
>     };
> 
> where __TIMESPEC_PAD_BEFORE_TV_NSEC is defined in some appropriate
> sysdeps header (bits/wordsize.h is the first thing to come to mind
> but I'm not sure it's optimal).

As we did in musl, you can write it as an integer constant expression
assuming you have __BYTE_ORDER, in terms of sizeof(time_t) and
sizeof(long). You don't even need a preprocessor conditional around it
because :0 is valid.

> Maybe we'd also need __TIMESPEC_PAD_AFTER_TV_NSEC, if the alignment
> requirement of __time_t is insufficient to make the compiler insert
> tail padding on some architectures.

Yes, you do, for that reason. At least x86 and m68k are affected.
Maybe some other obscure 32-bit archs too.

Rich

[RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Alejandro Colomar]

Use a type that can be relied upon by users,
for example for creating pointers.

It is backwards compatible, as it is defined to be long whenever it can,
and it makes the underlying type opaque,
since the user never had a need to know what it is.

First of all, this simplifies the implementation,
allowing a different underlying type in kernel and in user space.

The user only needs to know that it can hold [0, 999'999'999],
and it's a signed type.
To print it,
casting to long or to intmax_t (or even int when it's 32-bit) should be safe.
Using long was too specific of a contract with programmers.

Using snseconds_t in user code adds extra readability to the code,
since long is both meaningless and also unnecessarily explicit.

Link: linux-man <https://lore.kernel.org/linux-man/ec1dcc655184f6cdaae40ff8b7970b750434e4ef.1638123425.git.nabijaczleweli@nabijaczleweli.xyz/T/>
Link: glibc <https://sourceware.org/pipermail/libc-alpha/2021-December/133702.html>
Cc: наб <nabijaczleweli@nabijaczleweli.xyz>
Cc: Jakub Wilk <jwilk@jwilk.net>
Cc: Zack Weinberg <zackw@panix.com>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Michael Kerrisk (man-pages) <mtk.manpages@gmail.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Joseph Myers <joseph@codesourcery.com>
Cc: Rich Felker <dalias@libc.org>
Cc: Andreas Schwab <schwab@linux-m68k.org>
Cc: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Alejandro Colomar <alx.manpages@gmail.com>
---
 posix/sys/types.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/posix/sys/types.h b/posix/sys/types.h
index 477a45f4af..dae71f92b7 100644
--- a/posix/sys/types.h
+++ b/posix/sys/types.h
@@ -140,6 +140,11 @@ typedef __suseconds_t suseconds_t;
 # endif
 #endif
 
+#ifndef __snseconds_t_defined
+typedef __snseconds_t snseconds_t;
+# define __snseconds_t_defined
+#endif
+
 #define	__need_size_t
 #include <stddef.h>
 
-- 
2.34.1

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Zack Weinberg]

On Wed, Dec 8, 2021, at 9:48 AM, Alejandro Colomar wrote:
> 
> +#ifndef __snseconds_t_defined
> +typedef __snseconds_t snseconds_t;
> +# define __snseconds_t_defined
> +#endif

This is the old __need/__defined convention for not repeating typedefs.  Please use a <bits/types/snseconds_t.h> header instead.  (I really gotta find the time to dust off my patch series that finishes the conversion.)

zw

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Alejandro Colomar (man-pages)]

On 12/8/21 15:55, Zack Weinberg wrote:
>> +#ifndef __snseconds_t_defined
>> +typedef __snseconds_t snseconds_t;
>> +# define __snseconds_t_defined
>> +#endif
> 
> This is the old __need/__defined convention for not repeating typedefs.  Please use a <bits/types/snseconds_t.h> header instead.  (I really gotta find the time to dust off my patch series that finishes the conversion.)
> 

ack

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Paul Eggert]

On 12/8/21 06:48, Alejandro Colomar wrote:
> Use a type that can be relied upon by users,
> for example for creating pointers.

I'm still not seeing the need for a user-visible type "snseconds_t".

In response to the earlier proposal "[RFC v2 1/2] sys/types.h: Define 
new type: snseconds_t", Joseph suggested omitting snseconds_t[1], I 
supported this suggestion[2], and I don't recall seeing any statement 
explaining why this type needs to be user-visible.

This is a separate issue of whether we need a __snseconds_t type at all 
(which is the disagreement that Rich Felker is expressing). Although the 
type __snseconds_t may well be useful for glibc's own purposes, my 
experience is that user code doesn't need a snseconds_t type without the 
leading underscores. Arguably a user-visible snseconds_t type would 
cause more confusion than it'd cure, and so would be a net negative.

[1] https://sourceware.org/pipermail/libc-alpha/2021-December/133782.html
[2] https://sourceware.org/pipermail/libc-alpha/2021-December/133784.html>

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Rich Felker]

On Wed, Dec 08, 2021 at 10:12:44AM -0800, Paul Eggert wrote:
> On 12/8/21 06:48, Alejandro Colomar wrote:
> >Use a type that can be relied upon by users,
> >for example for creating pointers.
> 
> I'm still not seeing the need for a user-visible type "snseconds_t".
> 
> In response to the earlier proposal "[RFC v2 1/2] sys/types.h:
> Define new type: snseconds_t", Joseph suggested omitting
> snseconds_t[1], I supported this suggestion[2], and I don't recall
> seeing any statement explaining why this type needs to be
> user-visible.
> 
> This is a separate issue of whether we need a __snseconds_t type at
> all (which is the disagreement that Rich Felker is expressing).
> Although the type __snseconds_t may well be useful for glibc's own
> purposes, my experience is that user code doesn't need a snseconds_t
> type without the leading underscores. Arguably a user-visible
> snseconds_t type would cause more confusion than it'd cure, and so
> would be a net negative.

In discussing this with others in musl community, one thing that came
up is that it's extremely unlikely WG14 will be happy with this
proposed change, even if the Austin Group were, and since C11 the
definition of timespec is in the domain of WG14. I think that makes
the change a non-starter, or at least a source of a huge gratuitous
conflict that's not guaranteed to turn out in a way anybody likes.

I have no strong position on whether __snseconds_t (as a
glibc-internal thing) would be helpful to improve the x32 mess, but
I'd really like to see x32 fixed to conform to the standard.

For what it's worty: when musl first added x32, we had a lot of nasty
x32-local hacks to work around the fact that the kernel was not
prepared to see junk in the padding bits of tv_nsec. After doing all
the time64 work for 32-bit archs, though, it ended up that there were
clean places to do all the translation needed. It might turn out the
same for glibc. Also, provided you have a post-time64 kernel, my
understanding is that x32 already ignores the padding bits, so when
glibc gets to the point where the minimum supported kernel version
passes that, no action at all would be needed to fix x32 except
changing the public type.

Rich

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Zack Weinberg]

On Wed, Dec 8, 2021, at 1:25 PM, Rich Felker wrote:
> In discussing this with others in musl community, one thing that came
> up is that it's extremely unlikely WG14 will be happy with this
> proposed change, even if the Austin Group were,

What reasons do you have for making this assessment?

> For what it's worty: when musl first added x32, we had a lot of nasty
> x32-local hacks to work around the fact that the kernel was not
> prepared to see junk in the padding bits of tv_nsec. After doing all
> the time64 work for 32-bit archs, though, it ended up that there were
> clean places to do all the translation needed. It might turn out the
> same for glibc.

It is entirely possible that glibc's use of __syscall_slong_t for tv_nsec on x32 (and several other ABIs) is working around a problem that no longer exists, or that could be pushed out of scope by bumping the minimum supported kernel version.  I do not have a functional testbed for any of the affected ABIs.  I wonder if there's anyone reading this who has such a testbed, who could experiment with a change along the lines of

--- struct_timespec.h.old       2021-12-08 15:04:49.000000001 -0500
+++ struct_timespec.h   2021-12-08 15:08:59.000000001 -0500
@@ -15,18 +15,14 @@
 #else
   __time_t tv_sec;              /* Seconds.  */
 #endif
-#if __WORDSIZE == 64 \
-  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
-  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
-  __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
-#else
-# if __BYTE_ORDER == __BIG_ENDIAN
+#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
+  && __BYTE_ORDER == __BIG_ENDIAN
   int: 32;           /* Padding.  */
+#endif
   long int tv_nsec;  /* Nanoseconds.  */
-# else
-  long int tv_nsec;  /* Nanoseconds.  */
+#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
+  && __BYTE_ORDER == __LITTLE_ENDIAN
   int: 32;           /* Padding.  */
-# endif
 #endif
 };

and report back on the minimum kernel version for which this works correctly.

(I would still want the type changed in the standards, but only as futureproofing.)

zw

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Rich Felker]

On Wed, Dec 08, 2021 at 03:10:22PM -0500, Zack Weinberg wrote:
> On Wed, Dec 8, 2021, at 1:25 PM, Rich Felker wrote:
> > In discussing this with others in musl community, one thing that came
> > up is that it's extremely unlikely WG14 will be happy with this
> > proposed change, even if the Austin Group were,
> 
> What reasons do you have for making this assessment?

WG14 is generally very slow-moving, conservative about making changes,
and not agreeable to "please change this because we botched something
in our implementation" or even far more reasonable desires for changes
for the sake of maintaining ABI stability. They're also likely not
agreeable to your view that this morally "should be" a typedef because
of being a user-kernel boundary, because from their perspective (also
mine) the fact that there's a user-kernel boundary here is an
implementation detail not anything fundamental.

> > For what it's worty: when musl first added x32, we had a lot of nasty
> > x32-local hacks to work around the fact that the kernel was not
> > prepared to see junk in the padding bits of tv_nsec. After doing all
> > the time64 work for 32-bit archs, though, it ended up that there were
> > clean places to do all the translation needed. It might turn out the
> > same for glibc.
> 
> It is entirely possible that glibc's use of __syscall_slong_t for
> tv_nsec on x32 (and several other ABIs) is working around a problem
> that no longer exists,

It's not so much "working around a problem" as much as matching wrong
choices made on the kernel side at the time x32 was introduced. Linus
was apparently not aware that the API type was long (in the sense of
the C long type in the userspace ABI) and, as I understand it,
insisted the folks doing x32 use the existing 64-bit struct with
64-bit "kernel long" in it. So that's what happened. It would have
worked, even at the time, to just do the matching padding on the
userspace side if there'd been a tiny x32 shim in the kernel, for
syscalls where userspace passes-in a timespec, to zero the upper bits,
but that wasn't done until much later when it was integrated with the
kernel time64 support for 32-bit archs.

> or that could be pushed out of scope by
> bumping the minimum supported kernel version. I do not have a
> functional testbed for any of the affected ABIs. I wonder if there's
> anyone reading this who has such a testbed, who could experiment
> with a change along the lines of
> 
> --- struct_timespec.h.old       2021-12-08 15:04:49.000000001 -0500
> +++ struct_timespec.h   2021-12-08 15:08:59.000000001 -0500
> @@ -15,18 +15,14 @@
>  #else
>    __time_t tv_sec;              /* Seconds.  */
>  #endif
> -#if __WORDSIZE == 64 \
> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
> -  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
> -  __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
> -#else
> -# if __BYTE_ORDER == __BIG_ENDIAN
> +#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
> +  && __BYTE_ORDER == __BIG_ENDIAN
>    int: 32;           /* Padding.  */
> +#endif
>    long int tv_nsec;  /* Nanoseconds.  */
> -# else
> -  long int tv_nsec;  /* Nanoseconds.  */
> +#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
> +  && __BYTE_ORDER == __LITTLE_ENDIAN
>    int: 32;           /* Padding.  */
> -# endif
>  #endif
>  };
> 
> and report back on the minimum kernel version for which this works correctly.

I'll see if anyone I know has a glibc x32 test environment to do this.

> (I would still want the type changed in the standards, but only as futureproofing.)

https://xkcd.com/927/

Rich

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Adhemerval Zanella]

On 08/12/2021 17:10, Zack Weinberg via Libc-alpha wrote:
> On Wed, Dec 8, 2021, at 1:25 PM, Rich Felker wrote:
>> In discussing this with others in musl community, one thing that came
>> up is that it's extremely unlikely WG14 will be happy with this
>> proposed change, even if the Austin Group were,
> 
> What reasons do you have for making this assessment?
> 
>> For what it's worty: when musl first added x32, we had a lot of nasty
>> x32-local hacks to work around the fact that the kernel was not
>> prepared to see junk in the padding bits of tv_nsec. After doing all
>> the time64 work for 32-bit archs, though, it ended up that there were
>> clean places to do all the translation needed. It might turn out the
>> same for glibc.
> 
> It is entirely possible that glibc's use of __syscall_slong_t for tv_nsec on x32 (and several other ABIs) is working around a problem that no longer exists, or that could be pushed out of scope by bumping the minimum supported kernel version.  I do not have a functional testbed for any of the affected ABIs.  I wonder if there's anyone reading this who has such a testbed, who could experiment with a change along the lines of
> 
> --- struct_timespec.h.old       2021-12-08 15:04:49.000000001 -0500
> +++ struct_timespec.h   2021-12-08 15:08:59.000000001 -0500
> @@ -15,18 +15,14 @@
>  #else
>    __time_t tv_sec;              /* Seconds.  */
>  #endif
> -#if __WORDSIZE == 64 \
> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
> -  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
> -  __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
> -#else
> -# if __BYTE_ORDER == __BIG_ENDIAN
> +#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
> +  && __BYTE_ORDER == __BIG_ENDIAN
>    int: 32;           /* Padding.  */
> +#endif
>    long int tv_nsec;  /* Nanoseconds.  */
> -# else
> -  long int tv_nsec;  /* Nanoseconds.  */
> +#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
> +  && __BYTE_ORDER == __LITTLE_ENDIAN
>    int: 32;           /* Padding.  */
> -# endif
>  #endif
>  };

We can even be more clever and use something similar to what musl does:

diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
index 489e81136d..b962d3f95f 100644
--- a/time/bits/types/struct_timespec.h
+++ b/time/bits/types/struct_timespec.h
@@ -15,19 +15,9 @@ struct timespec
 #else
   __time_t tv_sec;             /* Seconds.  */
 #endif
-#if __WORDSIZE == 64 \
-  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
-  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
-  __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
-#else
-# if __BYTE_ORDER == __BIG_ENDIAN
-  int: 32;           /* Padding.  */
-  long int tv_nsec;  /* Nanoseconds.  */
-# else
-  long int tv_nsec;  /* Nanoseconds.  */
-  int: 32;           /* Padding.  */
-# endif
-#endif
+  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __BIG_ENDIAN);
+  long int tv_nsec;
+  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __LITTLE_ENDIAN);
 };
 
 #endif

> 
> and report back on the minimum kernel version for which this works correctly.

The above does not show any regression on a recent 5.13, but I am not sure which
is the minimal version it does work correctly on x32. 

I don't mind bump the minimal kernel version for x32, however it would be good
to check with x86 maintainers.

> 
> (I would still want the type changed in the standards, but only as futureproofing.)
> 
> zw
> 

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Alejandro Colomar (man-pages)]

Hi Adhemerval, Paul,

On 12/8/21 22:12, Adhemerval Zanella wrote:
> We can even be more clever and use something similar to what musl does:
> 
> diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
> index 489e81136d..b962d3f95f 100644
> --- a/time/bits/types/struct_timespec.h
> +++ b/time/bits/types/struct_timespec.h
> @@ -15,19 +15,9 @@ struct timespec
>   #else
>     __time_t tv_sec;             /* Seconds.  */
>   #endif
> -#if __WORDSIZE == 64 \
> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
> -  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
> -  __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
> -#else
> -# if __BYTE_ORDER == __BIG_ENDIAN
> -  int: 32;           /* Padding.  */
> -  long int tv_nsec;  /* Nanoseconds.  */
> -# else
> -  long int tv_nsec;  /* Nanoseconds.  */
> -  int: 32;           /* Padding.  */
> -# endif
> -#endif
> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __BIG_ENDIAN);
> +  long int tv_nsec;
> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __LITTLE_ENDIAN);
>   };
>   
>   #endif
> 
>>
>> and report back on the minimum kernel version for which this works correctly.
> 
> The above does not show any regression on a recent 5.13, but I am not sure which
> is the minimal version it does work correctly on x32.
> 
> I don't mind bump the minimal kernel version for x32, however it would be good
> to check with x86 maintainers.

If that works, this seems to me a clean workaround to the X32 bug (IMO 
both spec and kernel bug, in others' opinions kernel bug, in others' 
opinions spec bug, but we agree there's a bug).  We simply forget the 
bug ever existed, and hope no one triggers it again in a future 
implementation.  In the manual page we can add a small bug note for past 
implementations, noting that it doesn't apply anymore; instead of a 
prominent note that some implementations are currently buggy, which 
would be much worse.

> 
>>
>> (I would still want the type changed in the standards, but only as futureproofing.)
>>

Regarding this, I completely agree with Zack, that this is a bug spec, 
and since the standards simply wrote in stone existing practice, we can 
only conclude that existing practice wasn't ideal.  Having a contract so 
specific about the type of tv_nsec is unnecessary and in general C has 
moved in the other direction: use int or typedefs almost everywhere; 
rare are the interfaces that use long (and even more rarely or never 
short), and usually justified.  This case is just an accident of history.

I also see the value of keeping some accidents just for the sake of not 
accidentally making bigger accidents, and C has traditionally done this, 
so I could live with 'long' for tv_nsec, even if it is wrong.

In this case, in principle, snseconds_t would be backwards compatible, 
and it would allow future implementations (say 100 years from now?) to 
move to int if needed (maybe for ABI stability, maybe for other future 
reasons that we cannot foresee), or would allow to keep it defined as 
long forever if API stability is god.

But if you disagree with that, the above changes seem reasonable and 
enough to me.

Paul:

On 12/8/21 19:12, Paul Eggert wrote:
 >
 > I'm still not seeing the need for a user-visible type "snseconds_t".
 >

I hope Zack and I made our reasons visible to you in our latest (and 
this) emails.

 > In response to the earlier proposal "[RFC v2 1/2] sys/types.h: Define
 > new type: snseconds_t", Joseph suggested omitting snseconds_t[1], I
 > supported this suggestion[2], and I don't recall seeing any statement
 > explaining why this type needs to be user-visible.

Joseph suggested omitting it, and also suggested that if making it 
visible, do it in a separate patch for a separate discussion, and that's 
exactly what I did.

I enjoy these discussions, even when I know beforehand that my proposed 
changes will be rejected, because I (and also the other part) usually 
learn something, and hopefully fix other (related or unrelated) things 
that help all of us.  In this case we learnt some better code from musl, 
and cleaned up some mess of code (or are in the path to do so).

So please don't feel ignored in your rejection; I took it into account :)

 >
 > This is a separate issue of whether we need a __snseconds_t type at all
 > (which is the disagreement that Rich Felker is expressing). Although the
 > type __snseconds_t may well be useful for glibc's own purposes, my
 > experience is that user code doesn't need a snseconds_t type without the
 > leading underscores. Arguably a user-visible snseconds_t type would
 > cause more confusion than it'd cure, and so would be a net negative.

Yes, and that's why in v3 they are in different patches, although since 
they have some relation, they are still in the same patch set.

Thanks,
Alex


-- 
Alejandro Colomar
Linux man-pages maintainer; https://www.kernel.org/doc/man-pages/

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Adhemerval Zanella]

On 08/12/2021 18:53, Alejandro Colomar (man-pages) wrote:
> Hi Adhemerval, Paul,
> 
> On 12/8/21 22:12, Adhemerval Zanella wrote:
>> We can even be more clever and use something similar to what musl does:
>>
>> diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
>> index 489e81136d..b962d3f95f 100644
>> --- a/time/bits/types/struct_timespec.h
>> +++ b/time/bits/types/struct_timespec.h
>> @@ -15,19 +15,9 @@ struct timespec
>>   #else
>>     __time_t tv_sec;             /* Seconds.  */
>>   #endif
>> -#if __WORDSIZE == 64 \
>> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
>> -  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
>> -  __syscall_slong_t tv_nsec;   /* Nanoseconds.  */
>> -#else
>> -# if __BYTE_ORDER == __BIG_ENDIAN
>> -  int: 32;           /* Padding.  */
>> -  long int tv_nsec;  /* Nanoseconds.  */
>> -# else
>> -  long int tv_nsec;  /* Nanoseconds.  */
>> -  int: 32;           /* Padding.  */
>> -# endif
>> -#endif
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __BIG_ENDIAN);
>> +  long int tv_nsec;
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __LITTLE_ENDIAN);
>>   };
>>     #endif
>>
>>>
>>> and report back on the minimum kernel version for which this works correctly.
>>
>> The above does not show any regression on a recent 5.13, but I am not sure which
>> is the minimal version it does work correctly on x32.
>>
>> I don't mind bump the minimal kernel version for x32, however it would be good
>> to check with x86 maintainers.
> 
> If that works, this seems to me a clean workaround to the X32 bug (IMO both spec and kernel bug, in others' opinions kernel bug, in others' opinions spec bug, but we agree there's a bug).  We simply forget the bug ever existed, and hope no one triggers it again in a future implementation.  In the manual page we can add a small bug note for past implementations, noting that it doesn't apply anymore; instead of a prominent note that some implementations are currently buggy, which would be much worse.

It is clearly a bug [1], which was initially closed as not-implement but
I think we should fix it anyway.  I have sent a proposed fix [2], which
should cover old kernels that do not sanitize the high bits.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=16437
[2] https://sourceware.org/pipermail/libc-alpha/2021-December/133919.html

> 
>>
>>>
>>> (I would still want the type changed in the standards, but only as futureproofing.)
>>>
> 
> Regarding this, I completely agree with Zack, that this is a bug spec, and since the standards simply wrote in stone existing practice, we can only conclude that existing practice wasn't ideal.  Having a contract so specific about the type of tv_nsec is unnecessary and in general C has moved in the other direction: use int or typedefs almost everywhere; rare are the interfaces that use long (and even more rarely or never short), and usually justified.  This case is just an accident of history.
> 
> I also see the value of keeping some accidents just for the sake of not accidentally making bigger accidents, and C has traditionally done this, so I could live with 'long' for tv_nsec, even if it is wrong.
> 
> In this case, in principle, snseconds_t would be backwards compatible, and it would allow future implementations (say 100 years from now?) to move to int if needed (maybe for ABI stability, maybe for other future reasons that we cannot foresee), or would allow to keep it defined as long forever if API stability is god.
> 
> But if you disagree with that, the above changes seem reasonable and enough to me.

Sorry but I still see no reason to push for snseconds_t, nor __snseconds_t.
The *main* motivation is clearly a *bad* decision to use a non-defined type 
on x32 for the sake of avoid fixing it on glibc (which I give you back then 
would require a lot of code changes).  Now that we are push for more code 
unification, the possible fix is not that complex.

Also the type is already futureproof, unless you have a future ABI that defined
long with less than 32-bit (which I think it will hit another issues than this
one).  Different time_t, the tv_nsec is really bounded by hard constraint (it
need to hold nanoseconds, instead of an unbounded time in future).

> 
> Paul:
> 
> On 12/8/21 19:12, Paul Eggert wrote:
>>
>> I'm still not seeing the need for a user-visible type "snseconds_t".
>>
> 
> I hope Zack and I made our reasons visible to you in our latest (and this) emails.
> 
>> In response to the earlier proposal "[RFC v2 1/2] sys/types.h: Define
>> new type: snseconds_t", Joseph suggested omitting snseconds_t[1], I
>> supported this suggestion[2], and I don't recall seeing any statement
>> explaining why this type needs to be user-visible.
> 
> Joseph suggested omitting it, and also suggested that if making it visible, do it in a separate patch for a separate discussion, and that's exactly what I did.
> 
> I enjoy these discussions, even when I know beforehand that my proposed changes will be rejected, because I (and also the other part) usually learn something, and hopefully fix other (related or unrelated) things that help all of us.  In this case we learnt some better code from musl, and cleaned up some mess of code (or are in the path to do so).
> 
> So please don't feel ignored in your rejection; I took it into account :)
> 
>>
>> This is a separate issue of whether we need a __snseconds_t type at all
>> (which is the disagreement that Rich Felker is expressing). Although the
>> type __snseconds_t may well be useful for glibc's own purposes, my
>> experience is that user code doesn't need a snseconds_t type without the
>> leading underscores. Arguably a user-visible snseconds_t type would
>> cause more confusion than it'd cure, and so would be a net negative.
> 
> Yes, and that's why in v3 they are in different patches, although since they have some relation, they are still in the same patch set.
> 
> Thanks,
> Alex
> 
> 

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Paul Eggert]

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

On 12/8/21 13:12, Adhemerval Zanella wrote:
> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __BIG_ENDIAN);
> +  long int tv_nsec;
> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __LITTLE_ENDIAN);

Nice idea, and we can simplify things more: we don't need that last line 
as the compiler will insert that padding for us on all glibc platforms 
that need it. Also, the code should use 'long int' consistently. 
Further, there's no reason for struct timespec to mention __time64_t or 
__time_t; it can just use time_t consistently.

Something like the attached, say.

[-- Attachment #2: timespec.diff --]
[-- Type: text/x-patch, Size: 969 bytes --]

diff --git a/time/bits/types/struct_timespec.h b/time/bits/types/struct_timespec.h
index 489e81136d..72e8bb1e52 100644
--- a/time/bits/types/struct_timespec.h
+++ b/time/bits/types/struct_timespec.h
@@ -10,24 +10,12 @@
    has nanoseconds instead of microseconds.  */
 struct timespec
 {
-#ifdef __USE_TIME_BITS64
-  __time64_t tv_sec;		/* Seconds.  */
-#else
-  __time_t tv_sec;		/* Seconds.  */
-#endif
-#if __WORDSIZE == 64 \
-  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) \
-  || (__TIMESIZE == 32 && !defined __USE_TIME_BITS64)
-  __syscall_slong_t tv_nsec;	/* Nanoseconds.  */
-#else
-# if __BYTE_ORDER == __BIG_ENDIAN
-  int: 32;           /* Padding.  */
-  long int tv_nsec;  /* Nanoseconds.  */
-# else
+  time_t tv_sec;  /* Seconds.  */
+
+  int : (8 * (sizeof (time_t) - sizeof (long int))
+	 * (__BYTE_ORDER == __BIG_ENDIAN));
+
   long int tv_nsec;  /* Nanoseconds.  */
-  int: 32;           /* Padding.  */
-# endif
-#endif
 };
 
 #endif

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Adhemerval Zanella]

On 09/12/2021 16:42, Paul Eggert wrote:
> On 12/8/21 13:12, Adhemerval Zanella wrote:
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __BIG_ENDIAN);
>> +  long int tv_nsec;
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == __LITTLE_ENDIAN);
> 
> Nice idea, and we can simplify things more: we don't need that last line as the compiler will insert that padding for us on all glibc platforms that need it.

I think it does hurt to be explicit here.

> Also, the code should use 'long int' consistently. Further, there's no reason for struct timespec to mention __time64_t or __time_t; it can just use time_t consistently.> 
> Something like the attached, say.

Florian raised zero-width bitfields might not be ABI safe [1],
so I am using Zack suggestion:

struct timespec
{
  time_t tv_sec;                /* Seconds.  */
#endif
#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
  && __BYTE_ORDER == __BIG_ENDIAN
  int: 32;                      /* Padding.  */
#endif
  long int tv_nsec;             /* Nanoseconds.  */
#if __WORDSIZE == 32 && (__TIMESIZE == 64 || defined __USE_TIME_BITS64) \
  && __BYTE_ORDER == __LITTLE_ENDIAN
  int: 32;                      /* Padding.  */
#endif
};

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102024

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Joseph Myers]

On Thu, 9 Dec 2021, Paul Eggert wrote:

> Nice idea, and we can simplify things more: we don't need that last line as
> the compiler will insert that padding for us on all glibc platforms that need
> it. Also, the code should use 'long int' consistently. Further, there's no

On 32-bit x86, with _TIME_BITS=64, the structure is meant to have trailing 
padding, but won't unless it's explicit (the alignment of long long in 
structures is 4 bytes).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Rich Felker]

On Thu, Dec 09, 2021 at 08:13:53PM +0000, Joseph Myers wrote:
> On Thu, 9 Dec 2021, Paul Eggert wrote:
> 
> > Nice idea, and we can simplify things more: we don't need that last line as
> > the compiler will insert that padding for us on all glibc platforms that need
> > it. Also, the code should use 'long int' consistently. Further, there's no
> 
> On 32-bit x86, with _TIME_BITS=64, the structure is meant to have trailing 
> padding, but won't unless it's explicit (the alignment of long long in 
> structures is 4 bytes).

To be clear: omitting the explicit padding here would be an ABI break
*and* a buffer overflow, since the kernel will write 16 bytes but the
struct would only be 12 bytes. Thus the explicit padding is mandatory.

At least x86 (32-bit) and m68k are affected. I think there are a few
other obscure archs without 8-byte alignment (maybe or1k or
microblaze?) that might be too but I'm not sure of whether any of them
are supported by glibc.

Rich

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Alejandro Colomar (man-pages)]

Hi Paul,

On 12/9/21 20:42, Paul Eggert wrote:
> On 12/8/21 13:12, Adhemerval Zanella wrote:
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == 
>> __BIG_ENDIAN);
>> +  long int tv_nsec;
>> +  int : 8 * (sizeof (time_t) - sizeof (long)) * (__BYTE_ORDER == 
>> __LITTLE_ENDIAN);
> 
> Nice idea, and we can simplify things more: we don't need that last line 
> as the compiler will insert that padding for us on all glibc platforms 
> that need it. Also, the code should use 'long int' consistently. 
> Further, there's no reason for struct timespec to mention __time64_t or 
> __time_t; it can just use time_t consistently.

There are a few headers that POSIX says shall define timespec but it 
doesn't mention time_t in them, and AFAIR (I may remember wrongly), a 
header can't define a type if the standard doesn't say it's defined there.

The list is:

<aio.h>, <mqueue.h>, and <signal.h>.

See timespec and time_t on system_data_types(7).


Thanks,
Alex


-- 
Alejandro Colomar
Linux man-pages maintainer; https://www.kernel.org/doc/man-pages/

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Joseph Myers]

On Thu, 9 Dec 2021, Alejandro Colomar (man-pages) via Libc-alpha wrote:

> There are a few headers that POSIX says shall define timespec but it doesn't
> mention time_t in them, and AFAIR (I may remember wrongly), a header can't
> define a type if the standard doesn't say it's defined there.

In POSIX, unlike in ISO C, *_t is reserved in all headers.

In general we've moved towards defining POSIX *_t types for all POSIX 
headers the standard specifies to include some declaration involving those 
types, whether or not the standard requires that *_t name to be defined in 
that header.  (Newer POSIX versions mostly tend to require such *_t types 
to be defined in headers that use them; older POSIX versions have fewer 
such requirements, but still have the *_t reservation.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Alejandro Colomar (man-pages)]

Hi Joseph,

On 12/9/21 21:29, Joseph Myers wrote:
> On Thu, 9 Dec 2021, Alejandro Colomar (man-pages) via Libc-alpha wrote:
> 
>> There are a few headers that POSIX says shall define timespec but it doesn't
>> mention time_t in them, and AFAIR (I may remember wrongly), a header can't
>> define a type if the standard doesn't say it's defined there.
> 
> In POSIX, unlike in ISO C, *_t is reserved in all headers.
> 
> In general we've moved towards defining POSIX *_t types for all POSIX
> headers the standard specifies to include some declaration involving those
> types, whether or not the standard requires that *_t name to be defined in
> that header.  (Newer POSIX versions mostly tend to require such *_t types
> to be defined in headers that use them; older POSIX versions have fewer
> such requirements, but still have the *_t reservation.)
> 

Ahh, thanks!

On 12/9/21 21:23, Alejandro Colomar (man-pages) wrote:
 > <aio.h>, <mqueue.h>, and <signal.h>.

However... time_t is in ISO C, and <signal.h> is in ISO C too.  That may 
be a problem?

Cheers,
Alex


-- 
Alejandro Colomar
Linux man-pages maintainer; https://www.kernel.org/doc/man-pages/

Re: [RFC v3 3/3] sys/types.h: Make snseconds_t user visible

[Joseph Myers]

On Thu, 9 Dec 2021, Alejandro Colomar (man-pages) via Libc-alpha wrote:

> On 12/9/21 21:23, Alejandro Colomar (man-pages) wrote:
> > <aio.h>, <mqueue.h>, and <signal.h>.
> 
> However... time_t is in ISO C, and <signal.h> is in ISO C too.  That may be a
> problem?

ISO C <signal.h> doesn't include struct timespec, so there is no problem 
there.  We only expect to define *_t types that are actually used by some 
other declaration present in the header for the chosen standard 
(__USE_POSIX199309 is the condition for struct timespec in glibc 
<signal.h>).

-- 
Joseph S. Myers
joseph@codesourcery.com