[RFC v4 00/24] RISC-V glibc port for the 32-bit

classic Classic list List threaded Threaded
73 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[RFC v4 20/24] RISC-V: Fix llrint and llround missing exceptions on RV32

Alistair Francis
From: Zong Li <[hidden email]>

Similar to the recent fix for MIPS, ARM and S/390, RV32 is missing
correct exception on overflow from llrint and llround functions because
cast from floating-point types to long long do not result in correct
exceptions on overflow.

2018-11-29  Zong Li  <[hidden email]>

        * sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h: New file.
---
 ChangeLog                                     | 10 +++++
 .../riscv/rv32/fix-fp-int-convert-overflow.h  | 38 +++++++++++++++++++
 2 files changed, 48 insertions(+)
 create mode 100644 sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h

diff --git a/ChangeLog b/ChangeLog
index a0399acdcbc..3316d22efaf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1491,6 +1491,16 @@
  * sysdeps/unix/sysv/linux/riscv/configure.ac: Likewise.
  * sysdeps/unix/sysv/linux/riscv/shlib-versions: Likewise.
  * sysdeps/riscv/preconfigure: Likewise.
+ * sysdeps/riscv/rv32/Implies-after: New file.
+ * sysdeps/riscv/rv32/rvd/Implies: Likewise.
+ * sysdeps/riscv/rv32/rvf/Implies: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/rv32/Implies: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/Makefile: Support rv32.
+ * sysdeps/unix/sysv/linux/riscv/configure: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/configure.ac: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/shlib-versions: Likewise.
+ * sysdeps/riscv/preconfigure: Likewise.
+ * sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h: New file.
 
 2019-06-20  Dmitry V. Levin  <[hidden email]>
     Florian Weimer  <[hidden email]>
diff --git a/sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h b/sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h
new file mode 100644
index 00000000000..42155048382
--- /dev/null
+++ b/sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h
@@ -0,0 +1,38 @@
+/* Fix for conversion of floating point to integer overflow.  ARM version.
+   Copyright (C) 2015-2018 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#ifndef FIX_FP_INT_CONVERT_OVERFLOW_H
+#define FIX_FP_INT_CONVERT_OVERFLOW_H 1
+
+/* The generic libgcc2.c conversions from floating point
+   to long long may not raise the correct exceptions on overflow (and
+   may raise spurious "inexact" exceptions even in non-overflow cases,
+   see <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59412>).  */
+#define FIX_FLT_LONG_CONVERT_OVERFLOW 0
+#define FIX_FLT_LLONG_CONVERT_OVERFLOW 1
+
+#define FIX_DBL_LONG_CONVERT_OVERFLOW 0
+#define FIX_DBL_LLONG_CONVERT_OVERFLOW 1
+
+#define FIX_LDBL_LONG_CONVERT_OVERFLOW 0
+#define FIX_LDBL_LLONG_CONVERT_OVERFLOW 0
+
+#define FIX_FLT128_LONG_CONVERT_OVERFLOW 0
+#define FIX_FLT128_LLONG_CONVERT_OVERFLOW 0
+
+#endif /* fix-fp-int-convert-overflow.h */
--
2.22.0

Reply | Threaded
Open this post in threaded view
|

[RFC v4 21/24] Add RISC-V 32-bit target to build-many-glibcs.py

Alistair Francis
In reply to this post by Alistair Francis
From: Zong Li <[hidden email]>

Support building three variant of 32 bit RISC-V glibc as follows:
- riscv32-linux-gnu-rv32imac-ilp32
- riscv32-linux-gnu-rv32imafdc-ilp32
- riscv32-linux-gnu-rv32imafdc-ilp32d

2018-11-29  Zong Li  <[hidden email]>

        * scripts/build-many-glibcs.py (Context): Add rv32 targets.
---
 ChangeLog                    | 11 +++++++++++
 scripts/build-many-glibcs.py | 15 +++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 3316d22efaf..a3c2443d090 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1501,6 +1501,17 @@
  * sysdeps/unix/sysv/linux/riscv/shlib-versions: Likewise.
  * sysdeps/riscv/preconfigure: Likewise.
  * sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h: New file.
+ * sysdeps/riscv/rv32/Implies-after: New file.
+ * sysdeps/riscv/rv32/rvd/Implies: Likewise.
+ * sysdeps/riscv/rv32/rvf/Implies: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/rv32/Implies: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/Makefile: Support rv32.
+ * sysdeps/unix/sysv/linux/riscv/configure: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/configure.ac: Likewise.
+ * sysdeps/unix/sysv/linux/riscv/shlib-versions: Likewise.
+ * sysdeps/riscv/preconfigure: Likewise.
+ * sysdeps/riscv/rv32/fix-fp-int-convert-overflow.h: New file.
+ * scripts/build-many-glibcs.py (Context): Add rv32 targets.
 
 2019-06-20  Dmitry V. Levin  <[hidden email]>
     Florian Weimer  <[hidden email]>
diff --git a/scripts/build-many-glibcs.py b/scripts/build-many-glibcs.py
index aa6884e046d..205f70168ed 100755
--- a/scripts/build-many-glibcs.py
+++ b/scripts/build-many-glibcs.py
@@ -320,6 +320,21 @@ class Context(object):
         self.add_config(arch='powerpc64le',
                         os_name='linux-gnu',
                         gcc_cfg=['--disable-multilib', '--enable-secureplt'])
+        self.add_config(arch='riscv32',
+                        os_name='linux-gnu',
+                        variant='rv32imac-ilp32',
+                        gcc_cfg=['--with-arch=rv32imac', '--with-abi=ilp32',
+                                 '--disable-multilib'])
+        self.add_config(arch='riscv32',
+                        os_name='linux-gnu',
+                        variant='rv32imafdc-ilp32',
+                        gcc_cfg=['--with-arch=rv32imafdc', '--with-abi=ilp32',
+                                 '--disable-multilib'])
+        self.add_config(arch='riscv32',
+                        os_name='linux-gnu',
+                        variant='rv32imafdc-ilp32d',
+                        gcc_cfg=['--with-arch=rv32imafdc', '--with-abi=ilp32d',
+                                 '--disable-multilib'])
         self.add_config(arch='riscv64',
                         os_name='linux-gnu',
                         variant='rv64imac-lp64',
--
2.22.0

Reply | Threaded
Open this post in threaded view
|

[RFC v4 22/24] RISC-V: Use 64-bit vdso syscalls for RV32

Alistair Francis
In reply to this post by Alistair Francis
Signed-off-by: Alistair Francis <[hidden email]>
---
 sysdeps/unix/sysv/linux/clock_getres.c     |  4 ++++
 sysdeps/unix/sysv/linux/riscv/init-first.c | 26 +++++++++++++++++-----
 sysdeps/unix/sysv/linux/riscv/libc-vdso.h  | 12 +++++++---
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/clock_getres.c b/sysdeps/unix/sysv/linux/clock_getres.c
index 24b2299938c..6982c754d6f 100644
--- a/sysdeps/unix/sysv/linux/clock_getres.c
+++ b/sysdeps/unix/sysv/linux/clock_getres.c
@@ -30,6 +30,10 @@
 int
 __clock_getres (clockid_t clock_id, struct timespec *res)
 {
+#ifndef __vdso_clock_getres
+   return INLINE_SYSCALL_CALL (clock_getres, clock_id, res);
+#else
   return INLINE_VSYSCALL (clock_getres, 2, clock_id, res);
+#endif
 }
 weak_alias (__clock_getres, clock_getres)
diff --git a/sysdeps/unix/sysv/linux/riscv/init-first.c b/sysdeps/unix/sysv/linux/riscv/init-first.c
index 35dc8a8d386..e5f72803464 100644
--- a/sysdeps/unix/sysv/linux/riscv/init-first.c
+++ b/sysdeps/unix/sysv/linux/riscv/init-first.c
@@ -22,12 +22,18 @@
 
 long int (*VDSO_SYMBOL (getcpu)) (unsigned int *, unsigned int *, void *)
     attribute_hidden;
-long int (*VDSO_SYMBOL (gettimeofday)) (struct timeval *, void *)
-    attribute_hidden;
-long int (*VDSO_SYMBOL (__clock_gettime64)) (clockid_t, struct __timespec64 *)
+
+#if __riscv_xlen == 64
+long int (*VDSO_SYMBOL (clock_gettime)) (clockid_t, struct timespec *)
     attribute_hidden;
 long int (*VDSO_SYMBOL (clock_getres)) (clockid_t, struct timespec *)
     attribute_hidden;
+long int (*VDSO_SYMBOL (gettimeofday)) (struct timeval *, void *)
+    attribute_hidden;
+#else
+long int (*VDSO_SYMBOL (clock_gettime64)) (clockid_t, struct timespec *)
+    attribute_hidden;
+#endif
 
 static inline void
 _libc_vdso_platform_setup (void)
@@ -38,17 +44,25 @@ _libc_vdso_platform_setup (void)
   PTR_MANGLE (p);
   VDSO_SYMBOL (getcpu) = p;
 
-  p = _dl_vdso_vsym ("__vdso_gettimeofday", &linux_version);
+#if __riscv_xlen == 32
+  p = _dl_vdso_vsym ("__vdso_clock_gettime64", &linux_version);
   PTR_MANGLE (p);
-  VDSO_SYMBOL (gettimeofday) = p;
-
+  VDSO_SYMBOL (clock_gettime64) = p;
+#else
   p = _dl_vdso_vsym ("__vdso_clock_gettime", &linux_version);
   PTR_MANGLE (p);
   VDSO_SYMBOL (clock_gettime) = p;
+#endif
+
+#if __riscv_xlen == 64
+  p = _dl_vdso_vsym ("__vdso_gettimeofday", &linux_version);
+  PTR_MANGLE (p);
+  VDSO_SYMBOL (gettimeofday) = p;
 
   p = _dl_vdso_vsym ("__vdso_clock_getres", &linux_version);
   PTR_MANGLE (p);
   VDSO_SYMBOL (clock_getres) = p;
+#endif
 }
 
 #define VDSO_SETUP _libc_vdso_platform_setup
diff --git a/sysdeps/unix/sysv/linux/riscv/libc-vdso.h b/sysdeps/unix/sysv/linux/riscv/libc-vdso.h
index 16905d5b78d..da4ec9b2ed7 100644
--- a/sysdeps/unix/sysv/linux/riscv/libc-vdso.h
+++ b/sysdeps/unix/sysv/linux/riscv/libc-vdso.h
@@ -24,11 +24,17 @@
 
 extern long int (*VDSO_SYMBOL (getcpu)) (unsigned int *, unsigned int *, void *)
     attribute_hidden;
-extern long int (*VDSO_SYMBOL (gettimeofday)) (struct timeval *, void *)
-    attribute_hidden;
-extern long int (*VDSO_SYMBOL (__clock_gettime64)) (clockid_t, struct __timespec64 *)
+
+#if __riscv_xlen == 64
+extern long int (*VDSO_SYMBOL (clock_gettime)) (clockid_t, struct timespec *)
     attribute_hidden;
 extern long int (*VDSO_SYMBOL (clock_getres)) (clockid_t, struct timespec *)
     attribute_hidden;
+extern long int (*VDSO_SYMBOL (gettimeofday)) (struct timeval *, void *)
+    attribute_hidden;
+#else
+extern long int (*VDSO_SYMBOL (clock_gettime64)) (clockid_t, struct timespec *)
+    attribute_hidden;
+#endif
 
 #endif /* _LIBC_VDSO_H */
--
2.22.0

Reply | Threaded
Open this post in threaded view
|

[RFC v4 23/24] WIP: syscall.list: Call 64-bit versions of syscalls

Alistair Francis
In reply to this post by Alistair Francis
Signed-off-by: Alistair Francis <[hidden email]>
---
 sysdeps/unix/sysv/linux/syscalls.list | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
index e374f97b5f8..4844d1a9a3b 100644
--- a/sysdeps/unix/sysv/linux/syscalls.list
+++ b/sysdeps/unix/sysv/linux/syscalls.list
@@ -5,7 +5,7 @@ alarm - alarm i:i alarm
 bdflush EXTRA bdflush i:ii __compat_bdflush bdflush@GLIBC_2.0:GLIBC_2.23
 capget EXTRA capget i:pp capget
 capset EXTRA capset i:pp capset
-clock_adjtime EXTRA clock_adjtime i:ip clock_adjtime
+clock_adjtime EXTRA clock_adjtime64 i:ip clock_adjtime
 create_module EXTRA create_module 3 __compat_create_module create_module@GLIBC_2.0:GLIBC_2.23
 delete_module EXTRA delete_module 3 delete_module
 epoll_create EXTRA epoll_create i:i epoll_create
@@ -52,7 +52,7 @@ sched_getp - sched_getparam i:ip __sched_getparam sched_getparam
 sched_gets - sched_getscheduler i:i __sched_getscheduler sched_getscheduler
 sched_primax - sched_get_priority_max i:i __sched_get_priority_max sched_get_priority_max
 sched_primin - sched_get_priority_min i:i __sched_get_priority_min sched_get_priority_min
-sched_rr_gi - sched_rr_get_interval i:ip __sched_rr_get_interval sched_rr_get_interval
+sched_rr_gi - sched_rr_get_interval_time64 i:ip __sched_rr_get_interval sched_rr_get_interval
 sched_setp - sched_setparam i:ip __sched_setparam sched_setparam
 sched_sets - sched_setscheduler i:iip __sched_setscheduler sched_setscheduler
 sched_yield - sched_yield i: __sched_yield sched_yield
@@ -96,8 +96,8 @@ fremovexattr - fremovexattr i:is fremovexattr
 mq_setattr - mq_getsetattr i:ipp mq_setattr
 
 timerfd_create EXTRA timerfd_create i:ii timerfd_create
-timerfd_settime EXTRA timerfd_settime i:iipp timerfd_settime
-timerfd_gettime EXTRA timerfd_gettime i:ip timerfd_gettime
+timerfd_settime EXTRA timerfd_settime64 i:iipp timerfd_settime
+timerfd_gettime EXTRA timerfd_gettime64 i:ip timerfd_gettime
 
 fanotify_init EXTRA fanotify_init i:ii fanotify_init
 
--
2.22.0

Reply | Threaded
Open this post in threaded view
|

[RFC v4 24/24] timerfd_settime: Use 64-bit call if avaliable

Alistair Francis
In reply to this post by Alistair Francis
Signed-off-by: Alistair Francis <[hidden email]>
---
 sysdeps/unix/sysv/linux/Makefile          |   2 +
 sysdeps/unix/sysv/linux/sys/timerfd.h     |  21 +++--
 sysdeps/unix/sysv/linux/syscalls.list     |   1 -
 sysdeps/unix/sysv/linux/timerfd_settime.c | 104 ++++++++++++++++++++++
 4 files changed, 120 insertions(+), 8 deletions(-)
 create mode 100644 sysdeps/unix/sysv/linux/timerfd_settime.c

diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 1ab6bcbfc81..89128e80868 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -19,6 +19,8 @@ sysdep_routines += clone umount umount2 readahead \
    eventfd eventfd_read eventfd_write prlimit \
    personality epoll_wait tee vmsplice splice \
    open_by_handle_at mlock2 pkey_mprotect pkey_set pkey_get
+   timerfd_settime
+
 
 CFLAGS-gethostid.c = -fexceptions
 CFLAGS-tee.c = -fexceptions -fasynchronous-unwind-tables
diff --git a/sysdeps/unix/sysv/linux/sys/timerfd.h b/sysdeps/unix/sysv/linux/sys/timerfd.h
index 5e5ad351a0d..51f63b357c1 100644
--- a/sysdeps/unix/sysv/linux/sys/timerfd.h
+++ b/sysdeps/unix/sysv/linux/sys/timerfd.h
@@ -24,6 +24,13 @@
 /* Get the platform-dependent flags.  */
 #include <bits/timerfd.h>
 
+#if __TIMESIZE == 32
+struct __itimerspec64
+  {
+    struct __timespec64 it_interval;
+    struct __timespec64 it_value;
+  };
+#endif
 
 /* Bits to be set in the FLAGS parameter of `timerfd_settime'.  */
 enum
@@ -40,16 +47,16 @@ __BEGIN_DECLS
 /* Return file descriptor for new interval timer source.  */
 extern int timerfd_create (__clockid_t __clock_id, int __flags) __THROW;
 
-/* Set next expiration time of interval timer source UFD to UTMR.  If
-   FLAGS has the TFD_TIMER_ABSTIME flag set the timeout value is
-   absolute.  Optionally return the old expiration time in OTMR.  */
-extern int timerfd_settime (int __ufd, int __flags,
-    const struct itimerspec *__utmr,
-    struct itimerspec *__otmr) __THROW;
-
 /* Return the next expiration time of UFD.  */
 extern int timerfd_gettime (int __ufd, struct itimerspec *__otmr) __THROW;
 
 __END_DECLS
 
+/* Set next expiration time of interval timer source UFD to UTMR.  If
+   FLAGS has the TFD_TIMER_ABSTIME flag set the timeout value is
+   absolute.  Optionally return the old expiration time in OTMR.  */
+int timerfd_settime (int __ufd, int __flags,
+        const struct itimerspec *__utmr,
+        struct itimerspec *__otmr) __THROW;
+
 #endif /* sys/timerfd.h */
diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
index 4844d1a9a3b..3fa1a5de63a 100644
--- a/sysdeps/unix/sysv/linux/syscalls.list
+++ b/sysdeps/unix/sysv/linux/syscalls.list
@@ -96,7 +96,6 @@ fremovexattr - fremovexattr i:is fremovexattr
 mq_setattr - mq_getsetattr i:ipp mq_setattr
 
 timerfd_create EXTRA timerfd_create i:ii timerfd_create
-timerfd_settime EXTRA timerfd_settime64 i:iipp timerfd_settime
 timerfd_gettime EXTRA timerfd_gettime64 i:ip timerfd_gettime
 
 fanotify_init EXTRA fanotify_init i:ii fanotify_init
diff --git a/sysdeps/unix/sysv/linux/timerfd_settime.c b/sysdeps/unix/sysv/linux/timerfd_settime.c
new file mode 100644
index 00000000000..830faeada77
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/timerfd_settime.c
@@ -0,0 +1,104 @@
+/* Copyright (C) 2003-2019 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+   Contributed by Ulrich Drepper <[hidden email]>, 2003.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <stdlib.h>
+#include <time.h>
+#include <sysdep.h>
+
+int
+timerfd_settime (int __ufd, int __flags, const struct itimerspec *__utmr,
+                 struct itimerspec *__otmr)
+{
+#ifdef __ASSUME_TIME64_SYSCALLS
+  /* Delete the kernel timer object.  */
+   return INLINE_SYSCALL (timerfd_settime64, 4, __ufd, __flags, __utmr, __otmr);
+#else
+   int ret;
+# ifdef __NR_timerfd_settime64
+#  if __TIMESIZE == 64
+  ret = INLINE_SYSCALL (timerfd_settime64, 4, __ufd, __flags, __utmr, __otmr);
+
+  if (ret == 0 || errno != ENOSYS)
+    {
+      return ret;
+    }
+#  else
+  struct __itimerspec64 ts64;
+  ret = INLINE_SYSCALL (timerfd_settime64, 4, __ufd, __flags, __utmr, &ts64);
+
+  if (ret == 0 || errno != ENOSYS)
+    {
+      __utmr->it_interval.tv_sec = ts64.it_interval.tv_sec
+      __utmr->it_interval.tv_nsec = ts64.it_interval.tv_nsec
+      if (! in_time_t_range (__utmr->it_interval.tv_sec))
+        {
+          __set_errno (EOVERFLOW);
+          return -1;
+        }
+
+      __utmr->it_value.tv_sec = ts64.it_value.tv_sec
+      __utmr->it_value.tv_nsec = ts64.it_value.tv_nsec
+      if (! in_time_t_range (__utmr->it_value.tv_sec))
+        {
+          __set_errno (EOVERFLOW);
+          return -1;
+        }
+
+      return 0;
+    }
+#  endif /* __TIMESIZE == 64 */
+# endif /* __NR_timerfd_settime */
+# if __TIMESIZE == 64
+  struct itimerspec ts32;
+
+  if (! in_time_t_range (__utmr->it_interval.tv_sec) ||
+      ! in_time_t_range (__utmr->it_value.tv_sec))
+    {
+      __set_errno (EOVERFLOW);
+      return -1;
+    }
+
+  ret = INLINE_SYSCALL (timerfd_settime, 4, __ufd, __flags, __utmr, &ts32);
+
+  if (ret == 0 || errno != ENOSYS)
+    {
+      __utmr->it_interval.tv_sec = ts32.it_interval.tv_sec
+      __utmr->it_interval.tv_nsec = ts32.it_interval.tv_nsec
+      if (! in_time_t_range (__utmr->it_interval.tv_sec))
+        {
+          __set_errno (EOVERFLOW);
+          return -1;
+        }
+
+      __utmr->it_value.tv_sec = ts32.it_value.tv_sec
+      __utmr->it_value.tv_nsec = ts32.it_value.tv_nsec
+      if (! in_time_t_range (__utmr->it_value.tv_sec))
+        {
+          __set_errno (EOVERFLOW);
+          return -1;
+        }
+
+      return 0;
+    }
+  return ret;
+# else
+    return INLINE_SYSCALL (timerfd_settime, 4, __ufd, __flags, __utmr, __otmr);
+# endif /* __TIMESIZE == 64 */
+#endif /* __ASSUME_TIME64_SYSCALLS */
+}
--
2.22.0

Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Paul Eggert
In reply to this post by Alistair Francis
Alistair Francis wrote:
> -    int tz_minuteswest; /* Minutes west of GMT.  */
> -    int tz_dsttime; /* Nonzero if DST is ever in effect.  */
> +    int tz_minuteswest_dep; /* Minutes west of GMT.  */
> +    int tz_dsttime_dep; /* Nonzero if DST is ever in effect.  */
These two members should be declared with __attribute_deprecated__. And once we
do that, do we still need to change their names? I don't recall any other
instance of such name-changing.

If we do change their names, I suggest a more-obvious suffix than "_dep" (which
could stand for a lot of things); "_deprecated" would be clearer.
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 24/24] timerfd_settime: Use 64-bit call if avaliable

Alistair Francis-2
In reply to this post by Alistair Francis
On Fri, Aug 9, 2019 at 6:04 PM Alistair Francis
<[hidden email]> wrote:
>
> Signed-off-by: Alistair Francis <[hidden email]>
> ---

This commit was clearly not tested very well (it's missing
semicolons), just see it more as pseudo code.

Alistair

>  sysdeps/unix/sysv/linux/Makefile          |   2 +
>  sysdeps/unix/sysv/linux/sys/timerfd.h     |  21 +++--
>  sysdeps/unix/sysv/linux/syscalls.list     |   1 -
>  sysdeps/unix/sysv/linux/timerfd_settime.c | 104 ++++++++++++++++++++++
>  4 files changed, 120 insertions(+), 8 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/timerfd_settime.c
>
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 1ab6bcbfc81..89128e80868 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -19,6 +19,8 @@ sysdep_routines += clone umount umount2 readahead \
>                    eventfd eventfd_read eventfd_write prlimit \
>                    personality epoll_wait tee vmsplice splice \
>                    open_by_handle_at mlock2 pkey_mprotect pkey_set pkey_get
> +                  timerfd_settime
> +
>
>  CFLAGS-gethostid.c = -fexceptions
>  CFLAGS-tee.c = -fexceptions -fasynchronous-unwind-tables
> diff --git a/sysdeps/unix/sysv/linux/sys/timerfd.h b/sysdeps/unix/sysv/linux/sys/timerfd.h
> index 5e5ad351a0d..51f63b357c1 100644
> --- a/sysdeps/unix/sysv/linux/sys/timerfd.h
> +++ b/sysdeps/unix/sysv/linux/sys/timerfd.h
> @@ -24,6 +24,13 @@
>  /* Get the platform-dependent flags.  */
>  #include <bits/timerfd.h>
>
> +#if __TIMESIZE == 32
> +struct __itimerspec64
> +  {
> +    struct __timespec64 it_interval;
> +    struct __timespec64 it_value;
> +  };
> +#endif
>
>  /* Bits to be set in the FLAGS parameter of `timerfd_settime'.  */
>  enum
> @@ -40,16 +47,16 @@ __BEGIN_DECLS
>  /* Return file descriptor for new interval timer source.  */
>  extern int timerfd_create (__clockid_t __clock_id, int __flags) __THROW;
>
> -/* Set next expiration time of interval timer source UFD to UTMR.  If
> -   FLAGS has the TFD_TIMER_ABSTIME flag set the timeout value is
> -   absolute.  Optionally return the old expiration time in OTMR.  */
> -extern int timerfd_settime (int __ufd, int __flags,
> -                           const struct itimerspec *__utmr,
> -                           struct itimerspec *__otmr) __THROW;
> -
>  /* Return the next expiration time of UFD.  */
>  extern int timerfd_gettime (int __ufd, struct itimerspec *__otmr) __THROW;
>
>  __END_DECLS
>
> +/* Set next expiration time of interval timer source UFD to UTMR.  If
> +   FLAGS has the TFD_TIMER_ABSTIME flag set the timeout value is
> +   absolute.  Optionally return the old expiration time in OTMR.  */
> +int timerfd_settime (int __ufd, int __flags,
> +        const struct itimerspec *__utmr,
> +        struct itimerspec *__otmr) __THROW;
> +
>  #endif /* sys/timerfd.h */
> diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
> index 4844d1a9a3b..3fa1a5de63a 100644
> --- a/sysdeps/unix/sysv/linux/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/syscalls.list
> @@ -96,7 +96,6 @@ fremovexattr  -       fremovexattr    i:is    fremovexattr
>  mq_setattr     -       mq_getsetattr   i:ipp   mq_setattr
>
>  timerfd_create EXTRA   timerfd_create  i:ii    timerfd_create
> -timerfd_settime        EXTRA   timerfd_settime64       i:iipp  timerfd_settime
>  timerfd_gettime        EXTRA   timerfd_gettime64       i:ip    timerfd_gettime
>
>  fanotify_init  EXTRA   fanotify_init   i:ii    fanotify_init
> diff --git a/sysdeps/unix/sysv/linux/timerfd_settime.c b/sysdeps/unix/sysv/linux/timerfd_settime.c
> new file mode 100644
> index 00000000000..830faeada77
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/timerfd_settime.c
> @@ -0,0 +1,104 @@
> +/* Copyright (C) 2003-2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <[hidden email]>, 2003.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; see the file COPYING.LIB.  If
> +   not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <time.h>
> +#include <sysdep.h>
> +
> +int
> +timerfd_settime (int __ufd, int __flags, const struct itimerspec *__utmr,
> +                 struct itimerspec *__otmr)
> +{
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +  /* Delete the kernel timer object.  */
> +   return INLINE_SYSCALL (timerfd_settime64, 4, __ufd, __flags, __utmr, __otmr);
> +#else
> +   int ret;
> +# ifdef __NR_timerfd_settime64
> +#  if __TIMESIZE == 64
> +  ret = INLINE_SYSCALL (timerfd_settime64, 4, __ufd, __flags, __utmr, __otmr);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      return ret;
> +    }
> +#  else
> +  struct __itimerspec64 ts64;
> +  ret = INLINE_SYSCALL (timerfd_settime64, 4, __ufd, __flags, __utmr, &ts64);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      __utmr->it_interval.tv_sec = ts64.it_interval.tv_sec
> +      __utmr->it_interval.tv_nsec = ts64.it_interval.tv_nsec
> +      if (! in_time_t_range (__utmr->it_interval.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      __utmr->it_value.tv_sec = ts64.it_value.tv_sec
> +      __utmr->it_value.tv_nsec = ts64.it_value.tv_nsec
> +      if (! in_time_t_range (__utmr->it_value.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      return 0;
> +    }
> +#  endif /* __TIMESIZE == 64 */
> +# endif /* __NR_timerfd_settime */
> +# if __TIMESIZE == 64
> +  struct itimerspec ts32;
> +
> +  if (! in_time_t_range (__utmr->it_interval.tv_sec) ||
> +      ! in_time_t_range (__utmr->it_value.tv_sec))
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +
> +  ret = INLINE_SYSCALL (timerfd_settime, 4, __ufd, __flags, __utmr, &ts32);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      __utmr->it_interval.tv_sec = ts32.it_interval.tv_sec
> +      __utmr->it_interval.tv_nsec = ts32.it_interval.tv_nsec
> +      if (! in_time_t_range (__utmr->it_interval.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      __utmr->it_value.tv_sec = ts32.it_value.tv_sec
> +      __utmr->it_value.tv_nsec = ts32.it_value.tv_nsec
> +      if (! in_time_t_range (__utmr->it_value.tv_sec))
> +        {
> +          __set_errno (EOVERFLOW);
> +          return -1;
> +        }
> +
> +      return 0;
> +    }
> +  return ret;
> +# else
> +    return INLINE_SYSCALL (timerfd_settime, 4, __ufd, __flags, __utmr, __otmr);
> +# endif /* __TIMESIZE == 64 */
> +#endif /* __ASSUME_TIME64_SYSCALLS */
> +}
> --
> 2.22.0
>
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Alistair Francis-2
In reply to this post by Paul Eggert
On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <[hidden email]> wrote:
>
> Alistair Francis wrote:
> > -    int tz_minuteswest;              /* Minutes west of GMT.  */
> > -    int tz_dsttime;          /* Nonzero if DST is ever in effect.  */
> > +    int tz_minuteswest_dep;          /* Minutes west of GMT.  */
> > +    int tz_dsttime_dep;              /* Nonzero if DST is ever in effect.  */
> These two members should be declared with __attribute_deprecated__. And once we
> do that, do we still need to change their names? I don't recall any other
> instance of such name-changing.

The name changing is coming from a suggestion here [1]. I do find it a
little aggressive though as I do see some build failures, specifically
for systemd.

>
> If we do change their names, I suggest a more-obvious suffix than "_dep" (which
> could stand for a lot of things); "_deprecated" would be clearer.

I'll change the name if we still decided to stick with the name change.

1: https://sourceware.org/ml/libc-alpha/2019-07/msg00614.html

Alistair
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Florian Weimer-5
* Alistair Francis:

> On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <[hidden email]> wrote:
>>
>> Alistair Francis wrote:
>> > -    int tz_minuteswest;              /* Minutes west of GMT.  */
>> > -    int tz_dsttime;          /* Nonzero if DST is ever in effect.  */
>> > +    int tz_minuteswest_dep;          /* Minutes west of GMT.  */
>> > +    int tz_dsttime_dep;              /* Nonzero if DST is ever in effect.  */
>> These two members should be declared with __attribute_deprecated__. And once we
>> do that, do we still need to change their names? I don't recall any other
>> instance of such name-changing.
>
> The name changing is coming from a suggestion here [1]. I do find it a
> little aggressive though as I do see some build failures, specifically
> for systemd.

Good point.  systemd does this:

        tz.tz_minuteswest = -minutesdelta;
        tz.tz_dsttime = 0; /* DST_NONE */

        /*
         * If the RTC does not run in UTC but in local time, the very first
         * call to settimeofday() will set the kernel's timezone and will warp the
         * system clock, so that it runs in UTC instead of the local time we
         * have read from the RTC.
         */
        if (settimeofday(tv_null, &tz) < 0)
                return negative_errno();

It seems to me that struct timezone is not actually deprecated on the
kernel side when used with settimeofday.  This would be something which
has to happen before we can change its definition.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Alistair Francis-2
On Sun, Aug 11, 2019 at 6:52 AM Florian Weimer <[hidden email]> wrote:

>
> * Alistair Francis:
>
> > On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <[hidden email]> wrote:
> >>
> >> Alistair Francis wrote:
> >> > -    int tz_minuteswest;              /* Minutes west of GMT.  */
> >> > -    int tz_dsttime;          /* Nonzero if DST is ever in effect.  */
> >> > +    int tz_minuteswest_dep;          /* Minutes west of GMT.  */
> >> > +    int tz_dsttime_dep;              /* Nonzero if DST is ever in effect.  */
> >> These two members should be declared with __attribute_deprecated__. And once we
> >> do that, do we still need to change their names? I don't recall any other
> >> instance of such name-changing.
> >
> > The name changing is coming from a suggestion here [1]. I do find it a
> > little aggressive though as I do see some build failures, specifically
> > for systemd.
>
> Good point.  systemd does this:
>
>         tz.tz_minuteswest = -minutesdelta;
>         tz.tz_dsttime = 0; /* DST_NONE */
>
>         /*
>          * If the RTC does not run in UTC but in local time, the very first
>          * call to settimeofday() will set the kernel's timezone and will warp the
>          * system clock, so that it runs in UTC instead of the local time we
>          * have read from the RTC.
>          */
>         if (settimeofday(tv_null, &tz) < 0)
>                 return negative_errno();
>
> It seems to me that struct timezone is not actually deprecated on the
> kernel side when used with settimeofday.  This would be something which
> has to happen before we can change its definition.

Ok, so in this case for 32-bit y2038 safe platforms (without
gettimeofday and settimeofday) we still seem fine to ignore the struct
timezone. It doesn't look like we can rename the members yet though.
Can we mark them as deprecated in the glibc source? That will just
generate warnings instead of errors right?

Alistair

>
> Thanks,
> Florian
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Rich Felker-2
In reply to this post by Florian Weimer-5
On Sun, Aug 11, 2019 at 03:52:45PM +0200, Florian Weimer wrote:

> * Alistair Francis:
>
> > On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <[hidden email]> wrote:
> >>
> >> Alistair Francis wrote:
> >> > -    int tz_minuteswest;              /* Minutes west of GMT.  */
> >> > -    int tz_dsttime;          /* Nonzero if DST is ever in effect.  */
> >> > +    int tz_minuteswest_dep;          /* Minutes west of GMT.  */
> >> > +    int tz_dsttime_dep;              /* Nonzero if DST is ever in effect.  */
> >> These two members should be declared with __attribute_deprecated__. And once we
> >> do that, do we still need to change their names? I don't recall any other
> >> instance of such name-changing.
> >
> > The name changing is coming from a suggestion here [1]. I do find it a
> > little aggressive though as I do see some build failures, specifically
> > for systemd.
>
> Good point.  systemd does this:
>
>         tz.tz_minuteswest = -minutesdelta;
>         tz.tz_dsttime = 0; /* DST_NONE */
>
>         /*
>          * If the RTC does not run in UTC but in local time, the very first
>          * call to settimeofday() will set the kernel's timezone and will warp the
>          * system clock, so that it runs in UTC instead of the local time we
>          * have read from the RTC.
>          */
>         if (settimeofday(tv_null, &tz) < 0)
>                 return negative_errno();
>
> It seems to me that struct timezone is not actually deprecated on the
> kernel side when used with settimeofday.  This would be something which
> has to happen before we can change its definition.

The only effect I'm aware of it ever having had on the kernel side was
actively harmful: applying the offset to the timestamps of all files
on fatfs mounts. This has all kinds of consistency problems with DST,
timezone changes, etc. and is lossy/destructive of data. Back in 2005
I recall writing a script to do a best-case repair on digital camera
timestamps (correctly recorded on the fatfs in UTC) that Linux mangled
via moving them to hdd under several different local timezones.

systemd should not be doing this at all. If anyone wants this feature
it should be a mount option tzoff=..., not a global that's secretly
set to match your system tz.

Rich
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Florian Weimer-5
* Rich Felker:

> On Sun, Aug 11, 2019 at 03:52:45PM +0200, Florian Weimer wrote:
>> * Alistair Francis:
>>
>> > On Fri, Aug 9, 2019 at 6:20 PM Paul Eggert <[hidden email]> wrote:
>> >>
>> >> Alistair Francis wrote:
>> >> > -    int tz_minuteswest;              /* Minutes west of GMT.  */
>> >> > -    int tz_dsttime;          /* Nonzero if DST is ever in effect.  */
>> >> > +    int tz_minuteswest_dep;          /* Minutes west of GMT.  */
>> >> > +    int tz_dsttime_dep;              /* Nonzero if DST is ever in effect.  */
>> >> These two members should be declared with __attribute_deprecated__. And once we
>> >> do that, do we still need to change their names? I don't recall any other
>> >> instance of such name-changing.
>> >
>> > The name changing is coming from a suggestion here [1]. I do find it a
>> > little aggressive though as I do see some build failures, specifically
>> > for systemd.
>>
>> Good point.  systemd does this:
>>
>>         tz.tz_minuteswest = -minutesdelta;
>>         tz.tz_dsttime = 0; /* DST_NONE */
>>
>>         /*
>>          * If the RTC does not run in UTC but in local time, the very first
>>          * call to settimeofday() will set the kernel's timezone and will warp the
>>          * system clock, so that it runs in UTC instead of the local time we
>>          * have read from the RTC.
>>          */
>>         if (settimeofday(tv_null, &tz) < 0)
>>                 return negative_errno();
>>
>> It seems to me that struct timezone is not actually deprecated on the
>> kernel side when used with settimeofday.  This would be something which
>> has to happen before we can change its definition.
>
> The only effect I'm aware of it ever having had on the kernel side was
> actively harmful: applying the offset to the timestamps of all files
> on fatfs mounts. This has all kinds of consistency problems with DST,
> timezone changes, etc. and is lossy/destructive of data. Back in 2005
> I recall writing a script to do a best-case repair on digital camera
> timestamps (correctly recorded on the fatfs in UTC) that Linux mangled
> via moving them to hdd under several different local timezones.

Fair enough.  I filed:

  <https://github.com/systemd/systemd/issues/13305>

Let's see what happens.

> systemd should not be doing this at all. If anyone wants this feature
> it should be a mount option tzoff=..., not a global that's secretly
> set to match your system tz.

It looks like time_offset already exists.

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 07/24] time: Deprecate struct timezone members

Zack Weinberg-2
In reply to this post by Alistair Francis
On Fri, Aug 9, 2019 at 9:04 PM Alistair Francis
<[hidden email]> wrote:
>
> Append the struct timezone members with '_dep'. This indicates that
> these members are deprecated and will cause build failures on code that
> is currently using the members.

I had been working on a more thorough version of this patch, one that
removed these fields altogether (replacing them with `char
__preserve_historic_size[2*sizeof(int)]`) and changed _all_ of the
code that touches them, within glibc, to either ignore the fields or
zero them out.  This patch tripped over an Alpha-only ABI headache
(Alpha has compatibility versions of both gettimeofday and
settimeofday) and then I ran out of time to work on it.

In view of the reported behavior of systemd, though, I now think
neither your approach nor mine is exactly what we should do.  We
should do something along these lines for the next release:

 - We should _not_ rename the fields of struct timezone, but we should
mark them __attribute_deprecated__ so that code still using them
triggers compile warnings.
 - We should do the same for the timezone fields of struct timeb.
 - Our implementation of gettimeofday should always pass NULL for
struct timezone to the kernel, and write zeroes to any struct timezone
argument that is supplied.  (This will transitively apply to ftime.)
 - We should leave settimeofday alone until the kernel has an
alternative means of doing the "RTC clock warp" thing that systemd is
trying to do.

Do you have time to work that patch up?  You can get my
patch-in-progress from
https://sourceware.org/git/?p=glibc.git;a=log;h=refs/heads/zack/gtod-no-timezone
(please grab at least the changes to manual/time.texi).  If you don't
have time, I will try to find time to work on it this week.

zw
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 02/24] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

Joseph Myers
In reply to this post by Alistair Francis
On Fri, 9 Aug 2019, Alistair Francis wrote:

> diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> index 07a51808df2..fc495b56c67 100644
> --- a/nptl/thrd_sleep.c
> +++ b/nptl/thrd_sleep.c
> @@ -25,14 +25,68 @@ int
>  thrd_sleep (const struct timespec* time_point, struct timespec* remaining)
>  {
>    INTERNAL_SYSCALL_DECL (err);
> -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point, remaining);
> +  int ret = -1;
> +
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> +                                 0, time_point, remaining);
> +#else

This still has the same problem as has been explained for previous
versions of the patch series: if time_t is 32-bit but
__ASSUME_TIME64_SYSCALLS is defined, it is not valid to call the
clock_nanosleep_time64 with a 32-bit timespec; you have to call it with a
64-bit timespec.  That is, if you call clock_nanosleep_time64 at all in
the case where the original function was called with a 32-bit timespec
(and I think you should do so) then you need to have conversions.

Please fix this *throughout* the patch series *before* posting the next
version.  That is, for every patch in the series you need to consider what
is appropriate for systems with 32-bit time - *not* just what is
appropriate for RV32.

The pattern we have previously discussed for 64-bit time support is that
the code should (a) define the function (__thrd_sleep_time64, say) for
64-bit time (which then only needs conversions in the reverse direction -
if the 64-bit syscall is not in fact available, but the 32-bit one is),
(b) if __TIMESIZE != 64, defines the 32-bit function as a thin wrapper
round that, (c) in the appropriate internal header, has a #define of the
name such as __thrd_sleep_time64 to the name such as thrd_sleep, in the
case where __TIMESIZE == 64 and thus no wrapper is needed.

> +# ifdef __NR_clock_nanosleep_time64
> +#  if __TIMESIZE == 64
> +  long int ret_64;
> +
> +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, CLOCK_REALTIME,
> +                                    0, time_point, remaining);
> +
> +  if (ret_64 == 0 || errno != ENOSYS)
> +    ret = ret_64;
> +#  else
> +  timespec64 ts64;
> +
> +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> +                                 CLOCK_REALTIME, 0, time_point,
> +                                 ts64);
> +
> +  if (ret == 0 || errno != ENOSYS)
> +    {
> +      remaining->tv_sec = ts64.tv_sec;
> +      remaining->tv_nsec = ts64.tv_nsec;
> +    }

thrd_sleep has *two* timespec arguments, one input and one output.  It's
not sufficient to just convert the output, as here; if you're doing
conversions, you have to convert *both*.

It is valid for the output argument to be NULL.  You need to avoid writing
to *remaining in that case.

Please try to test such 64-bit time changes in configurations covering as
many of the different cases in the code as possible, both at compile time
and at run time, and include details in the patch submissions of what
configurations you tested (architecture, kernel headers version,
--enable-kernel version, kernel version used when running the testsuite).  
I'd hope such testing would have shown up the issue with the output
argument being NULL, as well as the ABI breakage.

Relevant configurations should include at least (a) one that has always
had 64-bit time, e.g. x86_64; (b) one with 32-bit time and old kernel
headers (any kernel version at runtime); (c) one with 32-bit time and new
kernel headers, old kernel at runtime; (d) one with 32-bit time and new
kernel headers, new kernel at runtime but no --enable-kernel; (e) one with
32-bit time and new kernel at runtime and new --enable-kernel.  (New = 5.1
or later.)  I think that's a basic minimum for testing any patches related
to 64-bit time.  (If Florian's changes to provide syscall tables within
glibc go in, case (b) disappears.)

(In this case, you're also passing the struct ts64 by value to the syscall
rather than a pointer to it.  And as this is C, not C++, I'm not sure a
declaration "timespec64 ts64;" without "struct" would have compiled.)

> diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c

> +#if __TIMESIZE == 32
> +struct timespec64
> +{
> +  long long int tv_sec;   /* Seconds.  */
> +  long int tv_nsec;  /* Nanoseconds.  */
> +};

If we need such a structure different from the "struct __timespec64" in
Lukasz's patches, it surely does not belong in the .c file for one
particular function without a very detailed comment explaining exactly why
it's so specific to that function rather than in a more generic internal
header.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 03/24] sysdeps/gettimeofday: Use clock_gettime64 if avaliable

Joseph Myers
In reply to this post by Alistair Francis
On Fri, 9 Aug 2019, Alistair Francis wrote:

> The Linux documentation says that "The use of the timezone structure
> is obsolete; the tz argument should normally be specified as NULL."

The relevant information to include in commit messages here isn't what the
documentation says, it's what the interfaces actually do, as detailed in
<https://sourceware.org/ml/libc-alpha/2019-07/msg00574.html>.

>  __gettimeofday (struct timeval *tv, struct timezone *tz)

> +#ifdef __ASSUME_TIME64_SYSCALLS
> +  long int ret;
> +  struct timespec now;
> +
> +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,
> +                         &now);

This is using a timespec (possibly 32-bit) with a syscall that requires
the 64-bit version.  You need to address such issues throughout the patch
series.  As illustrated here, it's not just cases where you have a
mismatch with the function interface; you need to make sure, in every
case, that where you pass internal variables to 64-bit time syscalls they
have the correct type as well.

> +#else
> +# ifdef __NR_clock_nanosleep_time64

No, this code isn't using clock_nanosleep_time64.  The #ifdef should be
for the syscall it actually uses.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 05/24] sysdeps/clock_gettime: Use clock_gettime64 if avaliable

Joseph Myers
In reply to this post by Alistair Francis
On Fri, 9 Aug 2019, Alistair Francis wrote:

>  /* Get current value of CLOCK and store it in TP.  */
> +
>  int
>  __clock_gettime (clockid_t clock_id, struct timespec *tp)
>  {
> -  return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> +
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +   return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> +#else

This has the usual problems with missing conversions.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 06/24] sysdeps/timespec_get: Use clock_gettime64 if avaliable

Joseph Myers
In reply to this post by Alistair Francis
On Fri, 9 Aug 2019, Alistair Francis wrote:

> diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c
> index 52080ddf08a..97ef9c5f799 100644
> --- a/sysdeps/unix/sysv/linux/timespec_get.c
> +++ b/sysdeps/unix/sysv/linux/timespec_get.c
> @@ -24,6 +24,58 @@
>  #endif
>  #include <sysdep-vdso.h>
>  
> +int
> +__timespec_get (struct timespec *ts, int base)
> +{
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +  return INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, ts);
> +#else

This has the usual problems with missing conversions.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 08/24] sysdeps/stat: Copy the statx struct to stat instead of stat64

Joseph Myers
In reply to this post by Alistair Francis
On Fri, 9 Aug 2019, Alistair Francis wrote:

> When copying the statx struct to the stat stuct use the original stat
> struct instead of the stat64 struct. As the padding in the original is
> type 'unsigned short int' but the padding in the stat64 is 'unsigned int'
> the copy  can result in misallgined data. This would then incorrectly
> trigger the stat_overflow() failure.

This indicates there's something else wrong with the port.  By design, the
following apply for linux/generic/wordsize-32 ports of glibc:

* The layout of struct stat and struct stat64 is identical, except that
some bytes that are padding in struct stat serve as high parts of fields
that are wider in struct stat64 (and thus have endian-dependent positions
as determined by the __field64 macro in bits/stat.h).

* Conversions from statx have to go to stat64, including setting those
high parts as appropriate, so that the subsequent overflow checks (which
work by examining those padding fields) can correctly detect whether
overflow occurred and set errno to EOVERFLOW accordingly.  See my C-SKY
port reviews that resulted in the code we have now
<https://sourceware.org/ml/libc-alpha/2018-11/msg00624.html>
<https://sourceware.org/ml/libc-alpha/2018-11/msg00668.html>.

> This would be very obvious when using a 64-bit ino_t type on a 32-bit
> system, such as the RV32 port.

If those types are 64-bit, you should not have padding around them in
struct stat, so as to preserve the property that struct stat and struct
stat64 have the same layout.  I suppose this means bits/stat.h needs to
check further macros such as __OFF_T_MATCHES_OFF64_T.

You'll also need to ensure that XSTAT_IS_XSTAT64 is defined to 1.  And
you'll need to make wordsize-32/overflow.h define trivial versions of the
*_overflow functions in cases where the types match (this should be done
in that file, rather than making an RV32-specific copy, to benefit future
ports that make the same choices as RV32).

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 09/24] Documentation for the RISC-V 32-bit port

Joseph Myers
In reply to this post by Alistair Francis
On Fri, 9 Aug 2019, Alistair Francis wrote:

> diff --git a/NEWS b/NEWS
> index 4326997dddf..27742944e9a 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -306,6 +306,12 @@ Major new features:
>    "%EY" to control how the year number is formatted; they have the
>    same effect that they would on "%Ey".
>  
> +* Support RISC-V port for 32-bit. The ISA and ABI pairs supported as follows:
> +
> +    - rv32imac ilp32
> +    - rv32imafdc ilp32
> +    - rv32imafdc ilp32d

This is the 2.29 section of NEWS.  You need to add to the 2.31 section.

--
Joseph S. Myers
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [RFC v4 24/24] timerfd_settime: Use 64-bit call if avaliable

Joseph Myers
In reply to this post by Alistair Francis
On Fri, 9 Aug 2019, Alistair Francis wrote:

> diff --git a/sysdeps/unix/sysv/linux/sys/timerfd.h b/sysdeps/unix/sysv/linux/sys/timerfd.h
> index 5e5ad351a0d..51f63b357c1 100644
> --- a/sysdeps/unix/sysv/linux/sys/timerfd.h
> +++ b/sysdeps/unix/sysv/linux/sys/timerfd.h
> @@ -24,6 +24,13 @@
>  /* Get the platform-dependent flags.  */
>  #include <bits/timerfd.h>
>  
> +#if __TIMESIZE == 32
> +struct __itimerspec64
> +  {
> +    struct __timespec64 it_interval;
> +    struct __timespec64 it_value;
> +  };
> +#endif

This does not belong in an installed header (until we have actual
_TIME_BITS support added to all installed headers).

>  enum
> @@ -40,16 +47,16 @@ __BEGIN_DECLS
>  /* Return file descriptor for new interval timer source.  */
>  extern int timerfd_create (__clockid_t __clock_id, int __flags) __THROW;
>  
> -/* Set next expiration time of interval timer source UFD to UTMR.  If
> -   FLAGS has the TFD_TIMER_ABSTIME flag set the timeout value is
> -   absolute.  Optionally return the old expiration time in OTMR.  */
> -extern int timerfd_settime (int __ufd, int __flags,
> -    const struct itimerspec *__utmr,
> -    struct itimerspec *__otmr) __THROW;
> -
>  /* Return the next expiration time of UFD.  */
>  extern int timerfd_gettime (int __ufd, struct itimerspec *__otmr) __THROW;
>  
>  __END_DECLS
>  
> +/* Set next expiration time of interval timer source UFD to UTMR.  If
> +   FLAGS has the TFD_TIMER_ABSTIME flag set the timeout value is
> +   absolute.  Optionally return the old expiration time in OTMR.  */
> +int timerfd_settime (int __ufd, int __flags,
> +        const struct itimerspec *__utmr,
> +        struct itimerspec *__otmr) __THROW;

This move is wrong.  All function declarations in installed headers must
be between __BEGIN_DECLS and __END_DECLS so they have C linkage in C++.

This patch has no commit message explaining what it's supposed to be doing
and why.  Please include a proper explanation with *every* patch of what
it's doing, how it's been tested, etc.

> diff --git a/sysdeps/unix/sysv/linux/timerfd_settime.c b/sysdeps/unix/sysv/linux/timerfd_settime.c
> new file mode 100644
> index 00000000000..830faeada77
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/timerfd_settime.c
> @@ -0,0 +1,104 @@
> +/* Copyright (C) 2003-2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +   Contributed by Ulrich Drepper <[hidden email]>, 2003.

We don't use "Contributed by" in new source files.

What file with copyrightable content dating from 2003 is this based on?  
You should state that in your ChangeLog entry, if you're copying
significant content from another file (if not, of course the copyright
years should just be 2019).

--
Joseph S. Myers
[hidden email]
1234