Message ID | 20200203183153.11635-5-alistair.francis@wdc.com |
---|---|
State | New |
Headers | show |
Series | Always use 32-bit time_t for certain syscalls | expand |
Hi Alistair, Maybe it would be good to rewrite the subject line to also reflect the introduction of __[gs]etitimer64 ? > The Linux kernel expects itimerval to use a 32-bit time_t, even on > archs with a 64-bit time_t (like RV32). To address this let's convert > itimerval to/from 32-bit and 64-bit to ensure the kernel always gets > a 32-bit time_t. > > This means that all 32-bit architectures with a 64-bit time_t will be > able to use this generic implementation. > > This code is based on similar code in alpha, but adjusted to pass the > 32-bit time_t to the kernel. > > We can't directly call the __getitimer/__setitimer functions as they > expect a struct itimerval but we need to pass in a struct itimerval32. Please update this patch series to also include information about build/run testing. For example: Build-tests: ./src/scripts/build-many-glibcs.py glibcs > --- > include/time.h | 12 +++ > .../linux/generic/wordsize-32/getitimer.c | 54 +++++++++++++ > .../linux/generic/wordsize-32/setitimer.c | 77 > +++++++++++++++++++ .../linux/generic/wordsize-32/tv32-compat.h | > 35 +++++++++ 4 files changed, 178 insertions(+) > create mode 100644 > sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c create mode The same issue as pointed out in patch 6/6. The path sysdeps/unix/sysv/linux/generic/wordsize-32 is NOT searched by ARMv7 (ARM32) for files to be built. I've removed getitimer, getrusage and setitimer from ./sysdeps/unix/syscalls.list, so they are NOT auto generated wrappers anymore, but this doesn't help. It seems like for RV32 some extra paths are added... I've dug to the @sysnames@ variable set in configure in the source of glibc. I'm wondering how RV32 modifies it (and how it differs from ARM32)? > 100644 sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c create > mode 100644 sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > > diff --git a/include/time.h b/include/time.h > index 898ff0fb2d..e1d80b4190 100644 > --- a/include/time.h > +++ b/include/time.h > @@ -6,6 +6,7 @@ > # include <bits/types/locale_t.h> > # include <stdbool.h> > # include <time/mktime-internal.h> > +# include <sys/time.h> > # include <endian.h> > # include <time-clockid.h> > > @@ -118,6 +119,17 @@ struct __itimerval64 > }; > #endif > > +#if __TIMESIZE == 64 > +# define __getitimer64 __getitimer > +# define __setitimer64 __setitimer > +#else > +extern int __getitimer64 (enum __itimer_which __which, > + struct __itimerval64 *__value); Please add here: libc_hidden_proto (__getitimer64) > +extern int __setitimer64 (enum __itimer_which __which, > + const struct __itimerval64 *__restrict > __new, > + struct __itimerval64 *__restrict __old); Please add here: libc_hidden_proto (__setitimer64) > +#endif > + > #if __TIMESIZE == 64 > # define __ctime64 ctime > #else > diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c > b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c new file > mode 100644 index 0000000000..28a3e31126 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c > @@ -0,0 +1,54 @@ > +/* getitimer -- Get the state of an interval timer. Linux/tv32 > version. > + Copyright (C) 2020 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/>. */ > + > +#include <time.h> > +#include <sys/time.h> > +#include <sysdep.h> > +#include <tv32-compat.h> > + > +int > +__getitimer64 (__itimer_which_t which, struct __itimerval64 > *curr_value) +{ > + struct __itimerval32 curr_value_32; > + if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == -1) > + return -1; > + > + /* Write all fields of 'curr_value' regardless of overflow. */ > + curr_value->it_interval > + = valid_timeval32_to_timeval64 (curr_value_32.it_interval); > + curr_value->it_value > + = valid_timeval32_to_timeval64 (curr_value_32.it_value); > + return 0; > +} > + > + > +#if __TIMESIZE != 64 Please add here: libc_hidden_def (__gettimer64) Rationale: Before we introduce the Y2038 support (with -D_TIME_BITS=64) the __gettimer64 shall be hidden (to avoid resolving it via PLT). > +int > +__getitimer (__itimer_which_t which, struct itimerval *curr_value) > +{ > + struct __itimerval64 val64; > + > + val64.it_interval > + = valid_timeval_to_timeval64 (curr_value->it_interval); > + val64.it_value > + = valid_timeval_to_timeval64 (curr_value->it_value); > + > + return __getitimer64 (which, &val64); > +} Ok. As we expect 32 bit values - there is no need for checking (and 64 <-> 32 conversions will be OK). > +#endif > +weak_alias (__getitimer, getitimer) > diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c > b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c new file > mode 100644 index 0000000000..fabc7f2c0c > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c > @@ -0,0 +1,77 @@ > +/* setitimer -- Set the state of an interval timer. Linux/tv32 > version. > + Copyright (C) 2020 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/>. */ > + > +#include <time.h> > +#include <sys/time.h> > +#include <sysdep.h> > +#include <tv32-compat.h> > + > +int > +__setitimer64 (__itimer_which_t which, > + const struct __itimerval64 *restrict new_value, > + struct __itimerval64 *restrict old_value) > +{ > + struct __itimerval32 new_value_32; > + new_value_32.it_interval > + = valid_timeval64_to_timeval32 (new_value->it_interval); > + new_value_32.it_value > + = valid_timeval64_to_timeval32 (new_value->it_value); > + > + if (old_value == NULL) > + return INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, > NULL); + The above code is correct of course. However, I've used another approach for timerfd_settime (IMHO a bit more concise): https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/timerfd_settime.c;h=164b4e860acb6a365a3890f30e2eac364a518c22;hb=eae22432723b877354291aca4dbbfde5891dad59#l49 > + struct __itimerval32 old_value_32; > + if (INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, > &old_value_32) == -1) > + return -1; > + > + /* Write all fields of 'old_value' regardless of overflow. */ > + old_value->it_interval > + = valid_timeval32_to_timeval64 (old_value_32.it_interval); > + old_value->it_value > + = valid_timeval32_to_timeval64 (old_value_32.it_value); > + return 0; > +} > + > +#if __TIMESIZE != 64 Please add here: libc_hidden_def (__settimer64) > +int > +__setitimer (__itimer_which_t which, > + const struct itimerval *restrict new_value, > + struct itimerval *restrict old_value) > +{ > + int ret; > + struct __itimerval64 new64, old64; > + > + new64.it_interval > + = valid_timeval_to_timeval64 (new_value->it_interval); > + new64.it_value > + = valid_timeval_to_timeval64 (new_value->it_value); > + > + ret = __setitimer64 (which, &new64, &old64); I think that here we shall also consider the NULL pointer for old_value (as stated in http://man7.org/linux/man-pages/man2/setitimer.2.html) ret = __setitimer64 (which, &new64, old_value ? &old64 : NULL); > + > + if (ret != 0) > + return ret; > + > + old_value->it_interval > + = valid_timeval64_to_timeval (old64.it_interval); > + old_value->it_value > + = valid_timeval64_to_timeval (old64.it_value); > + Maybe it would be better to change this code to: if (ret == 0 && old_value) { old_value->it_interval = valid_timeval64_to_timeval (old64.it_interval); old_value->it_value = valid_timeval64_to_timeval (old64.it_value); } return ret Please find the timerfd_settime as a reference: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/timerfd_settime.c;h=164b4e860acb6a365a3890f30e2eac364a518c22;hb=eae22432723b877354291aca4dbbfde5891dad59#l77 > + return ret; > +} > +#endif > +weak_alias (__setitimer, setitimer) > diff --git > a/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h new file > mode 100644 index 0000000000..4eb6f216ea --- /dev/null > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > @@ -0,0 +1,35 @@ > +/* Compatibility definitions for `struct timeval' with 32-bit time_t. > + Copyright (C) 2020 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 _TV32_COMPAT_H > +#define _TV32_COMPAT_H 1 > + > +#include <features.h> > + > +#include <bits/types.h> > +#include <bits/types/time_t.h> > +#include <bits/types/struct_timeval.h> > + > +/* Structures containing 'struct timeval' with 32-bit time_t. */ > +struct __itimerval32 > +{ > + struct __timeval32 it_interval; > + struct __timeval32 it_value; > +}; I do guess that here (with the introduction of tv32-compat.h) we do follow what is done for alpha port? > + > +#endif /* tv32-compat.h */ Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Feb 04 2020, Lukasz Majewski wrote: > I've dug to the @sysnames@ variable set in configure in the source of glibc. > I'm wondering how RV32 modifies it (and how it differs from ARM32)? It is explicitly added through Implies (git grep -e wordsize-32 -- '*/Implies*') Andreas.
Hi Andreas, Alistair, > On Feb 04 2020, Lukasz Majewski wrote: > > > I've dug to the @sysnames@ variable set in configure in the source > > of glibc. I'm wondering how RV32 modifies it (and how it differs > > from ARM32)? > > It is explicitly added through Implies (git grep -e wordsize-32 -- > '*/Implies*') Thanks for pointing this out. It seems like no riscv (rv32) sets this particular patch. Only it is set for csky and nios2: sysdeps/unix/sysv/linux/csky/Implies:unix/sysv/linux/generic/wordsize-32 sysdeps/unix/sysv/linux/nios2/Implies:unix/sysv/linux/generic/wordsize-32 However, it seems like I could: 1. Remove setitimer, getitimer, getrusage from sysdeps/unix/syscalls.list 2. Extend Imply file for ARM32 (sysdeps/unix/sysv/linux/arm/Implies) by adding unix/sysv/linux/generic/wordsize-32 Unfortunately, this brings some issues with overflow header ../sysdeps/unix/sysv/linux/generic/wordsize-32/overflow.h:42:12: error: 'struct stat' has no member named '__st_ino_pad'; did you mean 'st_ino'? | if (buf->__st_ino_pad == 0 && buf->__st_size_pad == 0 | ^~~~~~~~~~~~ | st_ino Maybe it would be better to not introduce setitimer, getitimer and getrusage in: sysdeps/unix/sysv/linux/generic/wordsize-32/ but instead in: sysdeps/unix/sysv/linux/ So, it would be widely reusable as for example the __getitimer64 is aliased anyway to getitimer for __TIMESIZE == 64 and __WORDSIZE==64 ? (The same approach was taken with __timerfd_settime64 conversion: https://sourceware.org/git/?p=glibc.git;a=commit;h=eae22432723b877354291aca4dbbfde5891dad59 > > Andreas. > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Tue, Feb 4, 2020 at 6:49 AM Lukasz Majewski <lukma@denx.de> wrote: > > Hi Alistair, > > Maybe it would be good to rewrite the subject line to also reflect the > introduction of __[gs]etitimer64 ? > > > The Linux kernel expects itimerval to use a 32-bit time_t, even on > > archs with a 64-bit time_t (like RV32). To address this let's convert > > itimerval to/from 32-bit and 64-bit to ensure the kernel always gets > > a 32-bit time_t. > > > > This means that all 32-bit architectures with a 64-bit time_t will be > > able to use this generic implementation. > > > > This code is based on similar code in alpha, but adjusted to pass the > > 32-bit time_t to the kernel. > > > > We can't directly call the __getitimer/__setitimer functions as they > > expect a struct itimerval but we need to pass in a struct itimerval32. > > Please update this patch series to also include information about > build/run testing. > > For example: > > Build-tests: > ./src/scripts/build-many-glibcs.py glibcs Sorry Lukasz, I somehow lost your response in my v2. I have updated my v3 with your comments here. > > > > --- > > include/time.h | 12 +++ > > .../linux/generic/wordsize-32/getitimer.c | 54 +++++++++++++ > > .../linux/generic/wordsize-32/setitimer.c | 77 > > +++++++++++++++++++ .../linux/generic/wordsize-32/tv32-compat.h | > > 35 +++++++++ 4 files changed, 178 insertions(+) > > create mode 100644 > > sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c create mode > > The same issue as pointed out in patch 6/6. The path > sysdeps/unix/sysv/linux/generic/wordsize-32 is NOT searched by ARMv7 > (ARM32) for files to be built. > > I've removed getitimer, getrusage and setitimer from > ./sysdeps/unix/syscalls.list, so they are NOT auto generated wrappers > anymore, but this doesn't help. > > It seems like for RV32 some extra paths are added... > > I've dug to the @sysnames@ variable set in configure in the source of glibc. > I'm wondering how RV32 modifies it (and how it differs from ARM32)? This has been addressed in v2. > > > > 100644 sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c create > > mode 100644 sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > > > > diff --git a/include/time.h b/include/time.h > > index 898ff0fb2d..e1d80b4190 100644 > > --- a/include/time.h > > +++ b/include/time.h > > @@ -6,6 +6,7 @@ > > # include <bits/types/locale_t.h> > > # include <stdbool.h> > > # include <time/mktime-internal.h> > > +# include <sys/time.h> > > # include <endian.h> > > # include <time-clockid.h> > > > > @@ -118,6 +119,17 @@ struct __itimerval64 > > }; > > #endif > > > > +#if __TIMESIZE == 64 > > +# define __getitimer64 __getitimer > > +# define __setitimer64 __setitimer > > +#else > > +extern int __getitimer64 (enum __itimer_which __which, > > + struct __itimerval64 *__value); > > Please add here: > libc_hidden_proto (__getitimer64) Fixed in v3 > > > +extern int __setitimer64 (enum __itimer_which __which, > > + const struct __itimerval64 *__restrict > > __new, > > + struct __itimerval64 *__restrict __old); > > Please add here: > libc_hidden_proto (__setitimer64) Fixed in v3 > > > +#endif > > + > > #if __TIMESIZE == 64 > > # define __ctime64 ctime > > #else > > diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c > > b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c new file > > mode 100644 index 0000000000..28a3e31126 > > --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c > > @@ -0,0 +1,54 @@ > > +/* getitimer -- Get the state of an interval timer. Linux/tv32 > > version. > > + Copyright (C) 2020 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/>. */ > > + > > +#include <time.h> > > +#include <sys/time.h> > > +#include <sysdep.h> > > +#include <tv32-compat.h> > > + > > +int > > +__getitimer64 (__itimer_which_t which, struct __itimerval64 > > *curr_value) +{ > > + struct __itimerval32 curr_value_32; > > + if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == -1) > > + return -1; > > + > > + /* Write all fields of 'curr_value' regardless of overflow. */ > > + curr_value->it_interval > > + = valid_timeval32_to_timeval64 (curr_value_32.it_interval); > > + curr_value->it_value > > + = valid_timeval32_to_timeval64 (curr_value_32.it_value); > > + return 0; > > +} > > + > > + > > +#if __TIMESIZE != 64 > > Please add here: > libc_hidden_def (__gettimer64) Fixed in v3 > > Rationale: > > Before we introduce the Y2038 support (with -D_TIME_BITS=64) the > __gettimer64 shall be hidden (to avoid resolving it via PLT). > > > +int > > +__getitimer (__itimer_which_t which, struct itimerval *curr_value) > > +{ > > + struct __itimerval64 val64; > > + > > + val64.it_interval > > + = valid_timeval_to_timeval64 (curr_value->it_interval); > > + val64.it_value > > + = valid_timeval_to_timeval64 (curr_value->it_value); > > + > > + return __getitimer64 (which, &val64); > > +} > > Ok. As we expect 32 bit values - there is no need for checking (and 64 > <-> 32 conversions will be OK). > > > +#endif > > +weak_alias (__getitimer, getitimer) > > diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c > > b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c new file > > mode 100644 index 0000000000..fabc7f2c0c > > --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c > > @@ -0,0 +1,77 @@ > > +/* setitimer -- Set the state of an interval timer. Linux/tv32 > > version. > > + Copyright (C) 2020 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/>. */ > > + > > +#include <time.h> > > +#include <sys/time.h> > > +#include <sysdep.h> > > +#include <tv32-compat.h> > > + > > +int > > +__setitimer64 (__itimer_which_t which, > > + const struct __itimerval64 *restrict new_value, > > + struct __itimerval64 *restrict old_value) > > +{ > > + struct __itimerval32 new_value_32; > > + new_value_32.it_interval > > + = valid_timeval64_to_timeval32 (new_value->it_interval); > > + new_value_32.it_value > > + = valid_timeval64_to_timeval32 (new_value->it_value); > > + > > + if (old_value == NULL) > > + return INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, > > NULL); + > > The above code is correct of course. > > However, I've used another approach for timerfd_settime (IMHO a bit > more concise): > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/timerfd_settime.c;h=164b4e860acb6a365a3890f30e2eac364a518c22;hb=eae22432723b877354291aca4dbbfde5891dad59#l49 > > > + struct __itimerval32 old_value_32; > > + if (INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, > > &old_value_32) == -1) > > + return -1; > > + > > + /* Write all fields of 'old_value' regardless of overflow. */ > > + old_value->it_interval > > + = valid_timeval32_to_timeval64 (old_value_32.it_interval); > > + old_value->it_value > > + = valid_timeval32_to_timeval64 (old_value_32.it_value); > > + return 0; > > +} > > + > > +#if __TIMESIZE != 64 > > Please add here: > libc_hidden_def (__settimer64) Fixed in v3 > > > +int > > +__setitimer (__itimer_which_t which, > > + const struct itimerval *restrict new_value, > > + struct itimerval *restrict old_value) > > +{ > > + int ret; > > + struct __itimerval64 new64, old64; > > + > > + new64.it_interval > > + = valid_timeval_to_timeval64 (new_value->it_interval); > > + new64.it_value > > + = valid_timeval_to_timeval64 (new_value->it_value); > > + > > + ret = __setitimer64 (which, &new64, &old64); > > I think that here we shall also consider the NULL pointer for old_value > (as stated in http://man7.org/linux/man-pages/man2/setitimer.2.html) > > ret = __setitimer64 (which, &new64, old_value ? &old64 : NULL); > > > + > > + if (ret != 0) > > + return ret; > > + > > + old_value->it_interval > > + = valid_timeval64_to_timeval (old64.it_interval); > > + old_value->it_value > > + = valid_timeval64_to_timeval (old64.it_value); > > + > > Maybe it would be better to change this code to: > > if (ret == 0 && old_value) > { > old_value->it_interval > = valid_timeval64_to_timeval (old64.it_interval); > old_value->it_value > = valid_timeval64_to_timeval (old64.it_value); > } > > return ret Fixed in v3 > > Please find the timerfd_settime as a reference: > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/timerfd_settime.c;h=164b4e860acb6a365a3890f30e2eac364a518c22;hb=eae22432723b877354291aca4dbbfde5891dad59#l77 > > > + return ret; > > +} > > +#endif > > +weak_alias (__setitimer, setitimer) > > diff --git > > a/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > > b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h new file > > mode 100644 index 0000000000..4eb6f216ea --- /dev/null > > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > > @@ -0,0 +1,35 @@ > > +/* Compatibility definitions for `struct timeval' with 32-bit time_t. > > + Copyright (C) 2020 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 _TV32_COMPAT_H > > +#define _TV32_COMPAT_H 1 > > + > > +#include <features.h> > > + > > +#include <bits/types.h> > > +#include <bits/types/time_t.h> > > +#include <bits/types/struct_timeval.h> > > + > > +/* Structures containing 'struct timeval' with 32-bit time_t. */ > > +struct __itimerval32 > > +{ > > + struct __timeval32 it_interval; > > + struct __timeval32 it_value; > > +}; > > I do guess that here (with the introduction of tv32-compat.h) we do > follow what is done for alpha port? Alpha always uses a 32-bit value, while in the generic case we use a long (so the wordsize). Alistair > > > + > > +#endif /* tv32-compat.h */ > > > > > Best regards, > > Lukasz Majewski > > -- > > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Mon, 10 Feb 2020 09:56:40 -0800 Alistair Francis <alistair23@gmail.com> wrote: > On Tue, Feb 4, 2020 at 6:49 AM Lukasz Majewski <lukma@denx.de> wrote: > > > > Hi Alistair, > > > > Maybe it would be good to rewrite the subject line to also reflect > > the introduction of __[gs]etitimer64 ? > > > > > The Linux kernel expects itimerval to use a 32-bit time_t, even on > > > archs with a 64-bit time_t (like RV32). To address this let's > > > convert itimerval to/from 32-bit and 64-bit to ensure the kernel > > > always gets a 32-bit time_t. > > > > > > This means that all 32-bit architectures with a 64-bit time_t > > > will be able to use this generic implementation. > > > > > > This code is based on similar code in alpha, but adjusted to pass > > > the 32-bit time_t to the kernel. > > > > > > We can't directly call the __getitimer/__setitimer functions as > > > they expect a struct itimerval but we need to pass in a struct > > > itimerval32. > > > > Please update this patch series to also include information about > > build/run testing. > > > > For example: > > > > Build-tests: > > ./src/scripts/build-many-glibcs.py glibcs > > Sorry Lukasz, I somehow lost your response in my v2. I have updated my > v3 with your comments here. Ok, I'm looking forward for v3 then :-). Thanks for your patches. > > > > > > > > --- > > > include/time.h | 12 +++ > > > .../linux/generic/wordsize-32/getitimer.c | 54 +++++++++++++ > > > .../linux/generic/wordsize-32/setitimer.c | 77 > > > +++++++++++++++++++ .../linux/generic/wordsize-32/tv32-compat.h > > > | 35 +++++++++ 4 files changed, 178 insertions(+) > > > create mode 100644 > > > sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c create > > > mode > > > > The same issue as pointed out in patch 6/6. The path > > sysdeps/unix/sysv/linux/generic/wordsize-32 is NOT searched by ARMv7 > > (ARM32) for files to be built. > > > > I've removed getitimer, getrusage and setitimer from > > ./sysdeps/unix/syscalls.list, so they are NOT auto generated > > wrappers anymore, but this doesn't help. > > > > It seems like for RV32 some extra paths are added... > > > > I've dug to the @sysnames@ variable set in configure in the source > > of glibc. I'm wondering how RV32 modifies it (and how it differs > > from ARM32)? > > This has been addressed in v2. > > > > > > > > 100644 sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c > > > create mode 100644 > > > sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > > > > > > diff --git a/include/time.h b/include/time.h > > > index 898ff0fb2d..e1d80b4190 100644 > > > --- a/include/time.h > > > +++ b/include/time.h > > > @@ -6,6 +6,7 @@ > > > # include <bits/types/locale_t.h> > > > # include <stdbool.h> > > > # include <time/mktime-internal.h> > > > +# include <sys/time.h> > > > # include <endian.h> > > > # include <time-clockid.h> > > > > > > @@ -118,6 +119,17 @@ struct __itimerval64 > > > }; > > > #endif > > > > > > +#if __TIMESIZE == 64 > > > +# define __getitimer64 __getitimer > > > +# define __setitimer64 __setitimer > > > +#else > > > +extern int __getitimer64 (enum __itimer_which __which, > > > + struct __itimerval64 *__value); > > > > Please add here: > > libc_hidden_proto (__getitimer64) > > Fixed in v3 > > > > > > +extern int __setitimer64 (enum __itimer_which __which, > > > + const struct __itimerval64 *__restrict > > > __new, > > > + struct __itimerval64 *__restrict > > > __old); > > > > Please add here: > > libc_hidden_proto (__setitimer64) > > Fixed in v3 > > > > > > +#endif > > > + > > > #if __TIMESIZE == 64 > > > # define __ctime64 ctime > > > #else > > > diff --git > > > a/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c > > > b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c new > > > file mode 100644 index 0000000000..28a3e31126 --- /dev/null > > > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c > > > @@ -0,0 +1,54 @@ > > > +/* getitimer -- Get the state of an interval timer. Linux/tv32 > > > version. > > > + Copyright (C) 2020 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/>. */ > > > + > > > +#include <time.h> > > > +#include <sys/time.h> > > > +#include <sysdep.h> > > > +#include <tv32-compat.h> > > > + > > > +int > > > +__getitimer64 (__itimer_which_t which, struct __itimerval64 > > > *curr_value) +{ > > > + struct __itimerval32 curr_value_32; > > > + if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == > > > -1) > > > + return -1; > > > + > > > + /* Write all fields of 'curr_value' regardless of overflow. */ > > > + curr_value->it_interval > > > + = valid_timeval32_to_timeval64 (curr_value_32.it_interval); > > > + curr_value->it_value > > > + = valid_timeval32_to_timeval64 (curr_value_32.it_value); > > > + return 0; > > > +} > > > + > > > + > > > +#if __TIMESIZE != 64 > > > > Please add here: > > libc_hidden_def (__gettimer64) > > Fixed in v3 > > > > > Rationale: > > > > Before we introduce the Y2038 support (with -D_TIME_BITS=64) the > > __gettimer64 shall be hidden (to avoid resolving it via PLT). > > > > > +int > > > +__getitimer (__itimer_which_t which, struct itimerval > > > *curr_value) +{ > > > + struct __itimerval64 val64; > > > + > > > + val64.it_interval > > > + = valid_timeval_to_timeval64 (curr_value->it_interval); > > > + val64.it_value > > > + = valid_timeval_to_timeval64 (curr_value->it_value); > > > + > > > + return __getitimer64 (which, &val64); > > > +} > > > > Ok. As we expect 32 bit values - there is no need for checking (and > > 64 <-> 32 conversions will be OK). > > > > > +#endif > > > +weak_alias (__getitimer, getitimer) > > > diff --git > > > a/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c > > > b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c new > > > file mode 100644 index 0000000000..fabc7f2c0c --- /dev/null > > > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c > > > @@ -0,0 +1,77 @@ > > > +/* setitimer -- Set the state of an interval timer. Linux/tv32 > > > version. > > > + Copyright (C) 2020 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/>. */ > > > + > > > +#include <time.h> > > > +#include <sys/time.h> > > > +#include <sysdep.h> > > > +#include <tv32-compat.h> > > > + > > > +int > > > +__setitimer64 (__itimer_which_t which, > > > + const struct __itimerval64 *restrict new_value, > > > + struct __itimerval64 *restrict old_value) > > > +{ > > > + struct __itimerval32 new_value_32; > > > + new_value_32.it_interval > > > + = valid_timeval64_to_timeval32 (new_value->it_interval); > > > + new_value_32.it_value > > > + = valid_timeval64_to_timeval32 (new_value->it_value); > > > + > > > + if (old_value == NULL) > > > + return INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, > > > NULL); + > > > > The above code is correct of course. > > > > However, I've used another approach for timerfd_settime (IMHO a bit > > more concise): > > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/timerfd_settime.c;h=164b4e860acb6a365a3890f30e2eac364a518c22;hb=eae22432723b877354291aca4dbbfde5891dad59#l49 > > > > > + struct __itimerval32 old_value_32; > > > + if (INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, > > > &old_value_32) == -1) > > > + return -1; > > > + > > > + /* Write all fields of 'old_value' regardless of overflow. */ > > > + old_value->it_interval > > > + = valid_timeval32_to_timeval64 (old_value_32.it_interval); > > > + old_value->it_value > > > + = valid_timeval32_to_timeval64 (old_value_32.it_value); > > > + return 0; > > > +} > > > + > > > +#if __TIMESIZE != 64 > > > > Please add here: > > libc_hidden_def (__settimer64) > > Fixed in v3 > > > > > > +int > > > +__setitimer (__itimer_which_t which, > > > + const struct itimerval *restrict new_value, > > > + struct itimerval *restrict old_value) > > > +{ > > > + int ret; > > > + struct __itimerval64 new64, old64; > > > + > > > + new64.it_interval > > > + = valid_timeval_to_timeval64 (new_value->it_interval); > > > + new64.it_value > > > + = valid_timeval_to_timeval64 (new_value->it_value); > > > + > > > + ret = __setitimer64 (which, &new64, &old64); > > > > I think that here we shall also consider the NULL pointer for > > old_value (as stated in > > http://man7.org/linux/man-pages/man2/setitimer.2.html) > > > > ret = __setitimer64 (which, &new64, old_value ? &old64 : NULL); > > > > > + > > > + if (ret != 0) > > > + return ret; > > > + > > > + old_value->it_interval > > > + = valid_timeval64_to_timeval (old64.it_interval); > > > + old_value->it_value > > > + = valid_timeval64_to_timeval (old64.it_value); > > > + > > > > Maybe it would be better to change this code to: > > > > if (ret == 0 && old_value) > > { > > old_value->it_interval > > = valid_timeval64_to_timeval (old64.it_interval); > > old_value->it_value > > = valid_timeval64_to_timeval (old64.it_value); > > } > > > > return ret > > Fixed in v3 > > > > > Please find the timerfd_settime as a reference: > > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/timerfd_settime.c;h=164b4e860acb6a365a3890f30e2eac364a518c22;hb=eae22432723b877354291aca4dbbfde5891dad59#l77 > > > > > + return ret; > > > +} > > > +#endif > > > +weak_alias (__setitimer, setitimer) > > > diff --git > > > a/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > > > b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h new > > > file mode 100644 index 0000000000..4eb6f216ea --- /dev/null > > > +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h > > > @@ -0,0 +1,35 @@ > > > +/* Compatibility definitions for `struct timeval' with 32-bit > > > time_t. > > > + Copyright (C) 2020 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 _TV32_COMPAT_H > > > +#define _TV32_COMPAT_H 1 > > > + > > > +#include <features.h> > > > + > > > +#include <bits/types.h> > > > +#include <bits/types/time_t.h> > > > +#include <bits/types/struct_timeval.h> > > > + > > > +/* Structures containing 'struct timeval' with 32-bit time_t. */ > > > +struct __itimerval32 > > > +{ > > > + struct __timeval32 it_interval; > > > + struct __timeval32 it_value; > > > +}; > > > > I do guess that here (with the introduction of tv32-compat.h) we do > > follow what is done for alpha port? > > Alpha always uses a 32-bit value, while in the generic case we use a > long (so the wordsize). > > Alistair > > > > > > + > > > +#endif /* tv32-compat.h */ > > > > > > > > > > Best regards, > > > > Lukasz Majewski > > > > -- > > > > DENX Software Engineering GmbH, Managing Director: Wolfgang > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: > > lukma@denx.de Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/include/time.h b/include/time.h index 898ff0fb2d..e1d80b4190 100644 --- a/include/time.h +++ b/include/time.h @@ -6,6 +6,7 @@ # include <bits/types/locale_t.h> # include <stdbool.h> # include <time/mktime-internal.h> +# include <sys/time.h> # include <endian.h> # include <time-clockid.h> @@ -118,6 +119,17 @@ struct __itimerval64 }; #endif +#if __TIMESIZE == 64 +# define __getitimer64 __getitimer +# define __setitimer64 __setitimer +#else +extern int __getitimer64 (enum __itimer_which __which, + struct __itimerval64 *__value); +extern int __setitimer64 (enum __itimer_which __which, + const struct __itimerval64 *__restrict __new, + struct __itimerval64 *__restrict __old); +#endif + #if __TIMESIZE == 64 # define __ctime64 ctime #else diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c new file mode 100644 index 0000000000..28a3e31126 --- /dev/null +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/getitimer.c @@ -0,0 +1,54 @@ +/* getitimer -- Get the state of an interval timer. Linux/tv32 version. + Copyright (C) 2020 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/>. */ + +#include <time.h> +#include <sys/time.h> +#include <sysdep.h> +#include <tv32-compat.h> + +int +__getitimer64 (__itimer_which_t which, struct __itimerval64 *curr_value) +{ + struct __itimerval32 curr_value_32; + if (INLINE_SYSCALL_CALL (getitimer, which, &curr_value_32) == -1) + return -1; + + /* Write all fields of 'curr_value' regardless of overflow. */ + curr_value->it_interval + = valid_timeval32_to_timeval64 (curr_value_32.it_interval); + curr_value->it_value + = valid_timeval32_to_timeval64 (curr_value_32.it_value); + return 0; +} + + +#if __TIMESIZE != 64 +int +__getitimer (__itimer_which_t which, struct itimerval *curr_value) +{ + struct __itimerval64 val64; + + val64.it_interval + = valid_timeval_to_timeval64 (curr_value->it_interval); + val64.it_value + = valid_timeval_to_timeval64 (curr_value->it_value); + + return __getitimer64 (which, &val64); +} +#endif +weak_alias (__getitimer, getitimer) diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c new file mode 100644 index 0000000000..fabc7f2c0c --- /dev/null +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/setitimer.c @@ -0,0 +1,77 @@ +/* setitimer -- Set the state of an interval timer. Linux/tv32 version. + Copyright (C) 2020 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/>. */ + +#include <time.h> +#include <sys/time.h> +#include <sysdep.h> +#include <tv32-compat.h> + +int +__setitimer64 (__itimer_which_t which, + const struct __itimerval64 *restrict new_value, + struct __itimerval64 *restrict old_value) +{ + struct __itimerval32 new_value_32; + new_value_32.it_interval + = valid_timeval64_to_timeval32 (new_value->it_interval); + new_value_32.it_value + = valid_timeval64_to_timeval32 (new_value->it_value); + + if (old_value == NULL) + return INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, NULL); + + struct __itimerval32 old_value_32; + if (INLINE_SYSCALL_CALL (setitimer, which, &new_value_32, &old_value_32) == -1) + return -1; + + /* Write all fields of 'old_value' regardless of overflow. */ + old_value->it_interval + = valid_timeval32_to_timeval64 (old_value_32.it_interval); + old_value->it_value + = valid_timeval32_to_timeval64 (old_value_32.it_value); + return 0; +} + +#if __TIMESIZE != 64 +int +__setitimer (__itimer_which_t which, + const struct itimerval *restrict new_value, + struct itimerval *restrict old_value) +{ + int ret; + struct __itimerval64 new64, old64; + + new64.it_interval + = valid_timeval_to_timeval64 (new_value->it_interval); + new64.it_value + = valid_timeval_to_timeval64 (new_value->it_value); + + ret = __setitimer64 (which, &new64, &old64); + + if (ret != 0) + return ret; + + old_value->it_interval + = valid_timeval64_to_timeval (old64.it_interval); + old_value->it_value + = valid_timeval64_to_timeval (old64.it_value); + + return ret; +} +#endif +weak_alias (__setitimer, setitimer) diff --git a/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h new file mode 100644 index 0000000000..4eb6f216ea --- /dev/null +++ b/sysdeps/unix/sysv/linux/generic/wordsize-32/tv32-compat.h @@ -0,0 +1,35 @@ +/* Compatibility definitions for `struct timeval' with 32-bit time_t. + Copyright (C) 2020 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 _TV32_COMPAT_H +#define _TV32_COMPAT_H 1 + +#include <features.h> + +#include <bits/types.h> +#include <bits/types/time_t.h> +#include <bits/types/struct_timeval.h> + +/* Structures containing 'struct timeval' with 32-bit time_t. */ +struct __itimerval32 +{ + struct __timeval32 it_interval; + struct __timeval32 it_value; +}; + +#endif /* tv32-compat.h */