[RFC,v2,1/2] sys/types.h: Define new type: snseconds_t
Commit Message
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(+)
Comments
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.
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.
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
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
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
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
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
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).
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
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.
@@ -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
@@ -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. */
@@ -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>
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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