[PATCH] timespec: fill in other members

[Paul Eggert]

This problem was found when compiling GNU Emacs with
--enable-gcc-warnings on a platform where tv_sec is 64 bits and
tv_nsec is 32 bits, and struct timespec has padding.  GCC
-Wuse-of-uninitialized-value complained when a struct timespec
initialized only via assigning to tv_sec and tv_nsec was copied
via assignment (this was in lib/timespec.h’s make_timespec).
Although behavior is well-defined on this platform, the warning is
annoying and the behavior might not be well-defined on theoretical
platforms where struct timespec has other members.  To work around
this, initialize all the struct’s members.
* lib/getsockopt.c (rpl_getsockopt):
* lib/gettime.c (gettime):
* lib/gettimeofday.c (gettimeofday):
* lib/glthread/thread.c (gl_thread_self):
* lib/nanosleep.c (nanosleep):
* lib/parse-datetime.y (digits_to_date_time, set_hhmmss)
(signed_seconds, unsigned_seconds, yylex, parse_datetime_body):
* lib/poll.c (poll):
* lib/pselect.c (pselect):
* lib/pthread-cond.c (endlessly, pthread_cond_timedwait):
* lib/pthread-rwlock.c (pthread_rwlock_timedrdlock)
(pthread_rwlock_timedwrlock):
* lib/pthread_mutex_timedlock.c (pthread_mutex_timedlock):
* lib/select.c (rpl_select):
* lib/settime.c (settime):
* lib/stat-time.h (get_stat_atime, get_stat_ctime)
(get_stat_mtime, get_stat_birthtime):
* lib/thrd.c (rpl_thrd_current):
* lib/timespec.h (make_timespec):
* lib/timespec_getres.c (timespec_getres):
* lib/utimecmp.c (utimecmpat):
* lib/utimens.c (fdutimens):
When filling in a struct timespec or similar time-related structure
that might be copied elsewhere, also assign to any storage other
than tv_sec and tv_nsec, to avoid undefined behavior on (likely
theoretical) platforms where struct timespec has other members,
and also to avoid warnings from GCC and/or valgrind.
---
 ChangeLog | 39 +++
 lib/getsockopt.c  |  7 ---
 lib/gettime.c |  4 ++--
 lib/gettimeofday.c| 14 ++---
 lib/glthread/thread.c |  4 +---
 lib/nanosleep.c   |  3 +--
 lib/parse-datetime.y  | 17 +++
 lib/poll.c|  9 
 lib/pselect.c |  6 --
 lib/pthread-cond.c| 10 +++--
 lib/pthread-rwlock.c  |  8 ++-
 lib/pthread_mutex_timedlock.c |  4 +---
 lib/select.c  |  2 +-
 lib/settime.c |  6 ++
 lib/stat-time.h   | 33 ++---
 lib/thrd.c|  4 +---
 lib/timespec.h|  5 +
 lib/timespec_getres.c |  4 ++--
 lib/utimecmp.c| 11 +-
 lib/utimens.c | 20 +-
 20 files changed, 109 insertions(+), 101 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 34e7d72130..3ed1b1ef8f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,44 @@
 2023-05-14  Paul Eggert  
 
+   timespec: fill in other members
+   This problem was found when compiling GNU Emacs with
+   --enable-gcc-warnings on a platform where tv_sec is 64 bits and
+   tv_nsec is 32 bits, and struct timespec has padding.  GCC
+   -Wuse-of-uninitialized-value complained when a struct timespec
+   initialized only via assigning to tv_sec and tv_nsec was copied
+   via assignment (this was in lib/timespec.h’s make_timespec).
+   Although behavior is well-defined on this platform, the warning is
+   annoying and the behavior might not be well-defined on theoretical
+   platforms where struct timespec has other members.  To work around
+   this, initialize all the struct’s members.
+   * lib/getsockopt.c (rpl_getsockopt):
+   * lib/gettime.c (gettime):
+   * lib/gettimeofday.c (gettimeofday):
+   * lib/glthread/thread.c (gl_thread_self):
+   * lib/nanosleep.c (nanosleep):
+   * lib/parse-datetime.y (digits_to_date_time, set_hhmmss)
+   (signed_seconds, unsigned_seconds, yylex, parse_datetime_body):
+   * lib/poll.c (poll):
+   * lib/pselect.c (pselect):
+   * lib/pthread-cond.c (endlessly, pthread_cond_timedwait):
+   * lib/pthread-rwlock.c (pthread_rwlock_timedrdlock)
+   (pthread_rwlock_timedwrlock):
+   * lib/pthread_mutex_timedlock.c (pthread_mutex_timedlock):
+   * lib/select.c (rpl_select):
+   * lib/settime.c (settime):
+   * lib/stat-time.h (get_stat_atime, get_stat_ctime)
+   (get_stat_mtime, get_stat_birthtime):
+   * lib/thrd.c (rpl_thrd_current):
+   * lib/timespec.h (make_timespec):
+   * lib/timespec_getres.c (timespec_getres):
+   * lib/utimecmp.c (utimecmpat):
+   * lib/utimens.c (fdutimens):
+   When filling in a struct timespec or similar time-related structure
+   that might be copied elsewhere, also assign to any storage other
+   than tv_sec and tv_nsec, to avoid undefined behavior on (likely
+   theoretical) platforms where struct timespec has other members,
+   and also to avoid 

Re: [PATCH] timespec: fill in other members

[Bruno Haible]

Paul Eggert wrote:
> diff --git a/lib/poll.c b/lib/poll.c
> index a0dc2c5226..941fecf2d3 100644
> --- a/lib/poll.c
> +++ b/lib/poll.c
> @@ -396,14 +396,15 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
>if (timeout == 0)
>  {
>ptv = &tv;
> -  ptv->tv_sec = 0;
> -  ptv->tv_usec = 0;
> +  tv = (struct timeval) {0};
>  }
>else if (timeout > 0)
>  {
>ptv = &tv;
> -  ptv->tv_sec = timeout / 1000;
> -  ptv->tv_usec = (timeout % 1000) * 1000;
> +  tv = (struct timeval) {
> +.tv_sec = timeout / 1000,
> +.tv_usec = (timeout % 1000) * 1000;
> +  };
>  }
>else if (timeout == INFTIM)
>  /* wait forever */

This produces a syntax error on AIX:

source='../../gllib/poll.c' object='poll.o' libtool=no  DEPDIR=.deps 
depmode=xlc /bin/sh ../../build-aux/depcomp  xlc -q64 -qthreaded -qtls 
-DHAVE_CONFIG_H -I. -I../../gllib -I..  -DGNULIB_STRICT_CHECKING=1  
-I/home/haible/prefix64/include -D_THREAD_SAFE  -g -c -o poll.o 
../../gllib/poll.c
"../../gllib/poll.c", line 406.43: 1506-275 (S) Unexpected text ';' encountered.
make: 1254-004 The error code from the last command is 1.

Fixed as follows.


2023-05-15  Bruno Haible  

poll: Fix syntax error (regression from yesterday).
* lib/poll.c (poll): Remove semicolon inside braces.

diff --git a/lib/poll.c b/lib/poll.c
index 941fecf2d3..ec8d2c2b55 100644
--- a/lib/poll.c
+++ b/lib/poll.c
@@ -403,7 +403,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   ptv = &tv;
   tv = (struct timeval) {
 .tv_sec = timeout / 1000,
-.tv_usec = (timeout % 1000) * 1000;
+.tv_usec = (timeout % 1000) * 1000
   };
 }
   else if (timeout == INFTIM)

Re: [PATCH] timespec: fill in other members

[Bruno Haible]

Paul Eggert wrote:
> diff --git a/lib/select.c b/lib/select.c
> index 6b6ca4154c..991f475431 100644
> --- a/lib/select.c
> +++ b/lib/select.c
> @@ -516,7 +516,7 @@ restart:
>goto restart;
>  }
>if (timeout && wait_timeout == 0 && rc == 0)
> -timeout->tv_sec = timeout->tv_usec = 0;
> +timeout = (struct timeval) {0};
>  }
>  
>/* Now fill in the results.  */

This produces a compilation error on native Windows:

select.c(519): error C2440: '=': cannot convert from 'timeval' to 'rpl_timeval 
*'

Changing the line to
*timeout = (struct timeval) {0};
does not fix it:

select.c(519): error C2440: '=': cannot convert from 'timeval' to 'rpl_timeval'

The reason is that on native Windows, @REPLACE_STRUCT_TIMEVAL@ evaluates to 1,
and thus gnulib's sys/time.h overrides 'struct timeval':

  # if @REPLACE_STRUCT_TIMEVAL@
  #  define timeval rpl_timeval
  # endif

  struct timeval
  {
time_t tv_sec;
long int tv_usec;
  };
  #  define GNULIB_defined_struct_timeval 1

Then select.c does
  #undef timeval

It would be possible to fix this by writing

  #if GNULIB_defined_struct_timeval
  *timeout = (struct rpl_timeval) {0};
  #else
  *timeout = (struct timeval) {0};
  #endif

but that is more complex than before. IMO, it is better to just ignore this
warning if someone uses mingw with GCC ≥ 13. We don't attempt warning-free
compilation on native Windows, and certainly not with extra compiler options
like -Wanalyzer-use-of-uninitialized-value.


2023-05-15  Bruno Haible  

select: Fix compilation error (regression from yesterday).
* lib/select.c (rpl_select): Revert last change.

diff --git a/lib/select.c b/lib/select.c
index 991f475431..6b6ca4154c 100644
--- a/lib/select.c
+++ b/lib/select.c
@@ -516,7 +516,7 @@ restart:
   goto restart;
 }
   if (timeout && wait_timeout == 0 && rc == 0)
-timeout = (struct timeval) {0};
+timeout->tv_sec = timeout->tv_usec = 0;
 }
 
   /* Now fill in the results.  */

Re: [PATCH] timespec: fill in other members

[Pádraig Brady]

On 15/05/2023 07:30, Paul Eggert wrote:

This problem was found when compiling GNU Emacs with
--enable-gcc-warnings on a platform where tv_sec is 64 bits and
tv_nsec is 32 bits, and struct timespec has padding.  GCC
-Wuse-of-uninitialized-value complained when a struct timespec
initialized only via assigning to tv_sec and tv_nsec was copied
via assignment (this was in lib/timespec.h’s make_timespec).
Although behavior is well-defined on this platform, the warning is
annoying and the behavior might not be well-defined on theoretical
platforms where struct timespec has other members.  To work around
this, initialize all the struct’s members.



* lib/utimecmp.c (utimecmpat):
* lib/utimens.c (fdutimens):
When filling in a struct timespec or similar time-related structure
that might be copied elsewhere, also assign to any storage other
than tv_sec and tv_nsec, to avoid undefined behavior on (likely
theoretical) platforms where struct timespec has other members,
and also to avoid warnings from GCC and/or valgrind.


The new coreutils CI I have in place is failing to build
with gcc (Debian 10.2.1-6) 10.2.1 with gnulib latest (ebd843b3) as follows.
I think this may be due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82283
It's a pity that doesn't seem to have been backported to gcc 10 (2020).
I was thinking I might need to ./configure --enable-gcc-warnings=no for CI
at some stage, but it seems that's sooner rather than later.

It might be worth reverting the change to lib/utimecmp.c
and perhaps instead initializing with `= {0}`.
I confirmed that builds without issue.

cheers,
Pádraig

lib/utimecmp.c: In function 'utimecmpat':
lib/utimecmp.c:348:17: error: missing initializer for field 'tv_nsec' of 
'struct timespec' [-Werror=missing-field-initializers]
  348 | [0].tv_nsec = dst_a_ns,
  | ^
In file included from /usr/include/x86_64-linux-gnu/sys/select.h:39,
 from ./lib/sys/select.h:114,
 from /usr/include/x86_64-linux-gnu/sys/types.h:179,
 from ./lib/sys/types.h:46,
 from lib/utimecmp.h:23,
 from lib/utimecmp.c:22:
/usr/include/x86_64-linux-gnu/bits/types/struct_timespec.h:16:21: note: 
'tv_nsec' declared here
   16 |   __syscall_slong_t tv_nsec; /* Nanoseconds.  */
  | ^~~
lib/utimecmp.c:350:17: error: missing initializer for field 'tv_nsec' of 
'struct timespec' [-Werror=missing-field-initializers]
  350 | [1].tv_nsec = dst_m_ns + res / 9
  | ^
In file included from /usr/include/x86_64-linux-gnu/sys/select.h:39,
 from ./lib/sys/select.h:114,
 from /usr/include/x86_64-linux-gnu/sys/types.h:179,
 from ./lib/sys/types.h:46,
 from lib/utimecmp.h:23,
 from lib/utimecmp.c:22:
/usr/include/x86_64-linux-gnu/bits/types/struct_timespec.h:16:21: note: 
'tv_nsec' declared here
   16 |   __syscall_slong_t tv_nsec; /* Nanoseconds.  */
  | ^~~

Re: [PATCH] timespec: fill in other members

[Bruno Haible]

Paul Eggert wrote:
> @@ -143,8 +142,7 @@ gettimeofday (struct timeval *restrict tv, void *restrict 
> tz)
>  #   error "Only 1-second nominal clock resolution found.  Is that intended?" 
> \
>"If so, compile with the -DOK_TO_USE_1S_CLOCK option."
>  #  endif
> -  tv->tv_sec = time (NULL);
> -  tv->tv_usec = 0;
> +  *tv = (struct timeval) { .tv_sec = time (NULL) };
>  
>return 0;
>  

This coding style hampers maintainability: The reader will wonder what's the
intent of this code. "Why is it initializing tv_sec but not tv_usec?" IMO,
if the code intends to initialize tv_usec, it should do so explicitly.
If a field that a programmer who reads the code is relevant (in the sense
that the programmer thinks about it), it should be listed.
Even if the reader knows that an omitted field is zero-initialized by this
syntax, according to C99, it costs them one fewer brain cycle if the
initializer is explicit.

Omitting the initializer should IMO be limited to cases where it's irrelevant,
such as the .tm_wday field of a 'struct tm' used as input to 'mktime'.

I'm fixing this in the part of gnulib that I personally maintain.


2023-05-15  Bruno Haible  

gettimeofday, pthread-*, thread, thrd: Don't omit intended initializers.
* lib/gettimeofday.c (gettimeofday): List the initializers of both
tv_sec and tv_usec.
* lib/glthread/thread.c (gl_thread_self): List the initializers of both
tv_sec and tv_nsec.
* lib/pthread-cond.c (pthread_cond_wait): Likewise.
* lib/thrd.c (rpl_thrd_current): Likewise.
* lib/pthread-rwlock.c (MIN): New macro.
(pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock): List the
initializers of both tv_sec and tv_nsec. Don't modify the duration after
having initialized it.
* lib/pthread_mutex_timedlock.c (MIN): New macro.
(pthread_mutex_timedlock): List the initializers of both tv_sec and
tv_nsec. Don't modify the duration after having initialized it.

diff --git a/lib/gettimeofday.c b/lib/gettimeofday.c
index 69b43a62c8..c71629cbc5 100644
--- a/lib/gettimeofday.c
+++ b/lib/gettimeofday.c
@@ -142,7 +142,7 @@ gettimeofday (struct timeval *restrict tv, void *restrict 
tz)
 #   error "Only 1-second nominal clock resolution found.  Is that intended?" \
   "If so, compile with the -DOK_TO_USE_1S_CLOCK option."
 #  endif
-  *tv = (struct timeval) { .tv_sec = time (NULL) };
+  *tv = (struct timeval) { .tv_sec = time (NULL), .tv_usec = 0 };
 
   return 0;
 
diff --git a/lib/glthread/thread.c b/lib/glthread/thread.c
index 3352159edc..ce6b9a83ed 100644
--- a/lib/glthread/thread.c
+++ b/lib/glthread/thread.c
@@ -139,7 +139,11 @@ gl_thread_self (void)
 /* Memory allocation failed.  There is not much we can do.  Have to
busy-loop, waiting for the availability of memory.  */
 {
-  struct timespec ts = { .tv_sec = 1 };
+  struct timespec ts =
+{
+  .tv_sec = 1,
+  .tv_nsec = 0
+};
   thrd_sleep (&ts, NULL);
 }
   }
diff --git a/lib/pthread-cond.c b/lib/pthread-cond.c
index 6980cc6ed9..7c94e47b1a 100644
--- a/lib/pthread-cond.c
+++ b/lib/pthread-cond.c
@@ -115,7 +115,11 @@ pthread_cond_wait (_GL_UNUSED pthread_cond_t *cond,
  Wait endlessly.  */
   for (;;)
 {
-  struct timespec duration = { .tv_sec = 86400 };
+  struct timespec duration =
+{
+  .tv_sec = 86400,
+  .tv_nsec = 0
+};
   nanosleep (&duration, NULL);
 }
 }
diff --git a/lib/pthread-rwlock.c b/lib/pthread-rwlock.c
index 5087661b0b..25ff5eeb37 100644
--- a/lib/pthread-rwlock.c
+++ b/lib/pthread-rwlock.c
@@ -30,6 +30,10 @@
 # include 
 #endif
 
+#ifndef MIN
+# define MIN(a, b) ((a) < (b) ? (a) : (b))
+#endif
+
 #if ((defined _WIN32 && ! defined __CYGWIN__) && USE_WINDOWS_THREADS) || 
!HAVE_PTHREAD_H
 
 int
@@ -409,9 +413,11 @@ pthread_rwlock_timedrdlock (pthread_rwlock_t *lock,
 return ETIMEDOUT;
 
   /* Sleep 1 ms.  */
-  struct timespec duration = { .tv_nsec = 100 };
-  if (duration.tv_nsec > remaining)
-duration.tv_nsec = remaining;
+  struct timespec duration =
+{
+  .tv_sec = 0,
+  .tv_nsec = MIN (100, remaining)
+};
   nanosleep (&duration, NULL);
 }
 }
@@ -464,9 +470,11 @@ pthread_rwlock_timedwrlock (pthread_rwlock_t *lock,
 return ETIMEDOUT;
 
   /* Sleep 1 ms.  */
-  struct timespec duration = { .tv_nsec = 100 };
-  if (duration.tv_nsec > remaining)
-duration.tv_nsec = remaining;
+  struct timespec duration =
+{
+  .tv_sec = 0,
+  .tv_nsec = MIN (100, remaining)
+};
   nanosleep (&duration, NULL);
 }
 }
diff --git a/lib/pthread_mutex_timedlock.c b/lib/pthread_mutex_timedlock.c
index 2394092e83..2e931bb1d8 100644
--- a/lib/pthread_mutex_timedlo

Re: [PATCH] timespec: fill in other members

[Bruno Haible]

Pádraig Brady wrote:
> The new coreutils CI I have in place is failing to build
> with gcc (Debian 10.2.1-6) 10.2.1 with gnulib latest (ebd843b3) as follows.

> lib/utimecmp.c: In function 'utimecmpat':
> lib/utimecmp.c:348:17: error: missing initializer for field 'tv_nsec' of 
> 'struct timespec' [-Werror=missing-field-initializers]
>348 | [0].tv_nsec = dst_a_ns,
>| ^

> I think this may be due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82283
> It's a pity that doesn't seem to have been backported to gcc 10 (2020).

Thanks for the heads-up and analysis.

> I was thinking I might need to ./configure --enable-gcc-warnings=no for CI
> at some stage

coreutils is probably not the only GNU package that will see this warning.
Therefore better silence it in gnulib rather than in coreutils.

This patch should do it.


2023-05-15  Bruno Haible  

Work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82283 .
Reported by Pádraig Brady  in
.
* m4/gnulib-common.m4 (gl_CC_GNULIB_WARNINGS): Add
-Wno-missing-field-initializers for GCC < 11.

diff --git a/m4/gnulib-common.m4 b/m4/gnulib-common.m4
index edb8572da2..a2b53d33dc 100644
--- a/m4/gnulib-common.m4
+++ b/m4/gnulib-common.m4
@@ -1,4 +1,4 @@
-# gnulib-common.m4 serial 86
+# gnulib-common.m4 serial 87
 dnl Copyright (C) 2007-2023 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -1053,6 +1053,7 @@ AC_DEFUN([gl_CC_GNULIB_WARNINGS]
 dnl -Wno-float-conversion >= 4.9  >= 3.9
 dnl -Wno-float-equal  >= 3>= 3.9
 dnl -Wimplicit-fallthrough>= 7>= 3.9
+dnl -Wno-missing-field-initializers   >= 4.0, < 11
 dnl -Wno-pedantic >= 4.8  >= 3.9
 dnl -Wno-sign-compare >= 3>= 3.9
 dnl -Wno-sign-conversion  >= 4.3  >= 3.9
@@ -1078,6 +1079,9 @@ AC_DEFUN([gl_CC_GNULIB_WARNINGS]
   #if __GNUC__ >= 7 || (__clang_major__ + (__clang_minor__ >= 9) > 3)
   -Wimplicit-fallthrough
   #endif
+  #if __GNUC__ >= 4 && __GNUC__ < 11 && !defined __clang__
+  -Wno-missing-field-initializers
+  #endif
   #if __GNUC__ + (__GNUC_MINOR__ >= 8) > 4 || (__clang_major__ + 
(__clang_minor__ >= 9) > 3)
   -Wno-pedantic
   #endif