| Message ID | 20211208144757.37641-2-alx.manpages@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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
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.
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
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
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
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 };
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(-)