| Message ID | cf3287a8009ab6104a4dec45b7b8160f35a358b0.1711045522.git.jstancek@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v2] syscalls/timer_getoverrun01: use kernel_timer_t type | expand |
On Fri, Mar 22, 2024 at 2:26 AM Jan Stancek <jstancek@redhat.com> wrote: > Testcase is failing on s390x, with glibc-2.39 and 6.9-rc0 (git commit > a4145ce1e7bc). Userspace defines timer_t as void * (8 bytes), while > __kernel_timer_t is defined as int (4 bytes). This means that kernel > only populates 4 bytes, and other 4 can remain uninitialized, possibly > containing some non-zero garbage, e.g.: > > timer_create(CLOCK_REALTIME, {sigev_signo=SIGALRM, > sigev_notify=SIGEV_SIGNAL}, <unfinished ...> > <... timer_create resumed>[0]) = 0 > timer_getoverrun(1 <unfinished ...> > timer_getoverrun resumed>) = -1 EINVAL (Invalid argument) > timer_delete(1) = -1 EINVAL (Invalid argument) > > Since we are dealing with syscalls directly, use kernel_timer_t. > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > Reviewed-by: Li Wang <liwang@redhat.com> --- > .../kernel/syscalls/timer_getoverrun/timer_getoverrun01.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git > a/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > index 5c444857aaa7..57c2147bf47f 100644 > --- a/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > +++ b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > @@ -19,10 +19,11 @@ > #include <time.h> > #include "tst_safe_clocks.h" > #include "lapi/syscalls.h" > +#include "lapi/common_timers.h" > > static void run(void) > { > - timer_t timer; > + kernel_timer_t timer; > struct sigevent ev; > > ev.sigev_value = (union sigval) 0; > -- > 2.39.3 > >
Hi Jan, ... > +++ b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > @@ -19,10 +19,11 @@ > #include <time.h> > #include "tst_safe_clocks.h" > #include "lapi/syscalls.h" > +#include "lapi/common_timers.h" > static void run(void) > { > - timer_t timer; > + kernel_timer_t timer; Good catch. Reviewed-by: Petr Vorel <pvorel@suse.cz> BTW in v1 you used memset(&timer, 0, sizeof(timer_t)); Could have we used with current compilers just timer_t timer = {} instead of memset()? Or what is the reason we keep using memset() instead of {}? Kind regards, Petr
On Fri, Mar 22, 2024 at 6:11 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Jan, > > ... > > +++ b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > > @@ -19,10 +19,11 @@ > > #include <time.h> > > #include "tst_safe_clocks.h" > > #include "lapi/syscalls.h" > > +#include "lapi/common_timers.h" > > > static void run(void) > > { > > - timer_t timer; > > + kernel_timer_t timer; > > Good catch. > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > BTW in v1 you used memset(&timer, 0, sizeof(timer_t)); > Could have we used with current compilers just timer_t timer = {} instead of > memset()? Or what is the reason we keep using memset() instead of {}? I treated it as opaque type. If it was a scalar, plain {} doesn't work with all compilers. For example this fails to compile for me with gcc 11: int main() { int i = {}; return i; } $ gcc -Wpedantic a.c a.c: In function ‘main’: a.c:1:22: warning: ISO C forbids empty initializer braces [-Wpedantic] 1 | int main() { int i = {}; return i; } | ^ a.c:1:22: error: empty scalar initializer a.c:1:22: note: (near initialization for ‘i’) "{0}" should work - I guess I was just being too careful. > > Kind regards, > Petr >
> On Fri, Mar 22, 2024 at 6:11 AM Petr Vorel <pvorel@suse.cz> wrote: > > Hi Jan, > > ... > > > +++ b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > > > @@ -19,10 +19,11 @@ > > > #include <time.h> > > > #include "tst_safe_clocks.h" > > > #include "lapi/syscalls.h" > > > +#include "lapi/common_timers.h" > > > static void run(void) > > > { > > > - timer_t timer; > > > + kernel_timer_t timer; > > Good catch. > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > BTW in v1 you used memset(&timer, 0, sizeof(timer_t)); > > Could have we used with current compilers just timer_t timer = {} instead of > > memset()? Or what is the reason we keep using memset() instead of {}? > I treated it as opaque type. If it was a scalar, plain {} doesn't work with all > compilers. For example this fails to compile for me with gcc 11: > int main() { int i = {}; return i; } > $ gcc -Wpedantic a.c > a.c: In function ‘main’: > a.c:1:22: warning: ISO C forbids empty initializer braces [-Wpedantic] > 1 | int main() { int i = {}; return i; } > | ^ > a.c:1:22: error: empty scalar initializer > a.c:1:22: note: (near initialization for ‘i’) > "{0}" should work - I guess I was just being too careful. Thanks for info, Jan. I suppose the problem is also on older gcc (nothing gcc 11 specific). We compile also on gcc 4.8 and 7.3. Is it also problem for older clang? If I understand correctly it should be safe to use {0}, and {} probably waits for the future. Kind regards, Petr > > Kind regards, > > Petr
On Fri, Mar 22, 2024 at 10:35 AM Petr Vorel <pvorel@suse.cz> wrote: > > > On Fri, Mar 22, 2024 at 6:11 AM Petr Vorel <pvorel@suse.cz> wrote: > > > > Hi Jan, > > > > ... > > > > +++ b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c > > > > @@ -19,10 +19,11 @@ > > > > #include <time.h> > > > > #include "tst_safe_clocks.h" > > > > #include "lapi/syscalls.h" > > > > +#include "lapi/common_timers.h" > > > > > static void run(void) > > > > { > > > > - timer_t timer; > > > > + kernel_timer_t timer; > > > > Good catch. > > > Reviewed-by: Petr Vorel <pvorel@suse.cz> Pushed. > > > > BTW in v1 you used memset(&timer, 0, sizeof(timer_t)); > > > Could have we used with current compilers just timer_t timer = {} instead of > > > memset()? Or what is the reason we keep using memset() instead of {}? > > > I treated it as opaque type. If it was a scalar, plain {} doesn't work with all > > compilers. For example this fails to compile for me with gcc 11: > > int main() { int i = {}; return i; } > > > $ gcc -Wpedantic a.c > > a.c: In function ‘main’: > > a.c:1:22: warning: ISO C forbids empty initializer braces [-Wpedantic] > > 1 | int main() { int i = {}; return i; } > > | ^ > > a.c:1:22: error: empty scalar initializer > > a.c:1:22: note: (near initialization for ‘i’) > > > "{0}" should work - I guess I was just being too careful. > > Thanks for info, Jan. I suppose the problem is also on older > gcc (nothing gcc 11 specific). We compile also on gcc 4.8 and 7.3. > > Is it also problem for older clang? It is with clang 16: # clang a.c a.c:1:22: error: scalar initializer cannot be empty int main() { int i = {}; return i; } > > If I understand correctly it should be safe to use {0}, and {} probably waits > for the future. > > Kind regards, > Petr > > > > Kind regards, > > > Petr > >
diff --git a/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c index 5c444857aaa7..57c2147bf47f 100644 --- a/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c +++ b/testcases/kernel/syscalls/timer_getoverrun/timer_getoverrun01.c @@ -19,10 +19,11 @@ #include <time.h> #include "tst_safe_clocks.h" #include "lapi/syscalls.h" +#include "lapi/common_timers.h" static void run(void) { - timer_t timer; + kernel_timer_t timer; struct sigevent ev; ev.sigev_value = (union sigval) 0;
Testcase is failing on s390x, with glibc-2.39 and 6.9-rc0 (git commit a4145ce1e7bc). Userspace defines timer_t as void * (8 bytes), while __kernel_timer_t is defined as int (4 bytes). This means that kernel only populates 4 bytes, and other 4 can remain uninitialized, possibly containing some non-zero garbage, e.g.: timer_create(CLOCK_REALTIME, {sigev_signo=SIGALRM, sigev_notify=SIGEV_SIGNAL}, <unfinished ...> <... timer_create resumed>[0]) = 0 timer_getoverrun(1 <unfinished ...> timer_getoverrun resumed>) = -1 EINVAL (Invalid argument) timer_delete(1) = -1 EINVAL (Invalid argument) Since we are dealing with syscalls directly, use kernel_timer_t. Signed-off-by: Jan Stancek <jstancek@redhat.com> --- .../kernel/syscalls/timer_getoverrun/timer_getoverrun01.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)