* [PATCH 1/2] selftests: timens: use 'llabs()' over 'abs()'
@ 2021-11-05 16:31 Anders Roxell
2021-11-05 16:31 ` [PATCH 2/2] selftests: timens: exec: use 'labs()' " Anders Roxell
2021-11-05 20:26 ` [PATCH 1/2] selftests: timens: use 'llabs()' " Nick Desaulniers
0 siblings, 2 replies; 11+ messages in thread
From: Anders Roxell @ 2021-11-05 16:31 UTC (permalink / raw)
To: shuah
Cc: nathan, ndesaulniers, linux-kselftest, linux-kernel, llvm, Anders Roxell
When building selftests/timens with clang, the compiler warn about the
function abs() see below:
timerfd.c:64:7: error: absolute value function 'abs' given an argument of type 'long long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value]
if (abs(elapsed - 3600) > 60) {
^
timerfd.c:64:7: note: use function 'llabs' instead
if (abs(elapsed - 3600) > 60) {
^~~
llabs
The note indicates what to do, Rework to use the function 'llabs()'.
Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
---
tools/testing/selftests/timens/timer.c | 2 +-
tools/testing/selftests/timens/timerfd.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/timens/timer.c b/tools/testing/selftests/timens/timer.c
index 5e7f0051bd7b..5b939f59dfa4 100644
--- a/tools/testing/selftests/timens/timer.c
+++ b/tools/testing/selftests/timens/timer.c
@@ -56,7 +56,7 @@ int run_test(int clockid, struct timespec now)
return pr_perror("timerfd_gettime");
elapsed = new_value.it_value.tv_sec;
- if (abs(elapsed - 3600) > 60) {
+ if (llabs(elapsed - 3600) > 60) {
ksft_test_result_fail("clockid: %d elapsed: %lld\n",
clockid, elapsed);
return 1;
diff --git a/tools/testing/selftests/timens/timerfd.c b/tools/testing/selftests/timens/timerfd.c
index 9edd43d6b2c1..a4196bbd6e33 100644
--- a/tools/testing/selftests/timens/timerfd.c
+++ b/tools/testing/selftests/timens/timerfd.c
@@ -61,7 +61,7 @@ int run_test(int clockid, struct timespec now)
return pr_perror("timerfd_gettime(%d)", clockid);
elapsed = new_value.it_value.tv_sec;
- if (abs(elapsed - 3600) > 60) {
+ if (llabs(elapsed - 3600) > 60) {
ksft_test_result_fail("clockid: %d elapsed: %lld\n",
clockid, elapsed);
return 1;
--
2.33.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-05 16:31 [PATCH 1/2] selftests: timens: use 'llabs()' over 'abs()' Anders Roxell @ 2021-11-05 16:31 ` Anders Roxell 2021-11-05 20:35 ` Nick Desaulniers 2021-11-05 20:26 ` [PATCH 1/2] selftests: timens: use 'llabs()' " Nick Desaulniers 1 sibling, 1 reply; 11+ messages in thread From: Anders Roxell @ 2021-11-05 16:31 UTC (permalink / raw) To: shuah Cc: nathan, ndesaulniers, linux-kselftest, linux-kernel, llvm, Anders Roxell When building selftests/timens with clang, the compiler warn about the function abs() see below: exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] if (abs(tst.tv_sec - now.tv_sec) > 5) ^ exec.c:33:8: note: use function 'labs' instead if (abs(tst.tv_sec - now.tv_sec) > 5) ^~~ labs The note indicates what to do, Rework to use the function 'labs()'. Signed-off-by: Anders Roxell <anders.roxell@linaro.org> --- tools/testing/selftests/timens/exec.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/timens/exec.c b/tools/testing/selftests/timens/exec.c index e40dc5be2f66..d12ff955de0d 100644 --- a/tools/testing/selftests/timens/exec.c +++ b/tools/testing/selftests/timens/exec.c @@ -30,7 +30,7 @@ int main(int argc, char *argv[]) for (i = 0; i < 2; i++) { _gettime(CLOCK_MONOTONIC, &tst, i); - if (abs(tst.tv_sec - now.tv_sec) > 5) + if (labs(tst.tv_sec - now.tv_sec) > 5) return pr_fail("%ld %ld\n", now.tv_sec, tst.tv_sec); } return 0; @@ -50,7 +50,7 @@ int main(int argc, char *argv[]) for (i = 0; i < 2; i++) { _gettime(CLOCK_MONOTONIC, &tst, i); - if (abs(tst.tv_sec - now.tv_sec) > 5) + if (labs(tst.tv_sec - now.tv_sec) > 5) return pr_fail("%ld %ld\n", now.tv_sec, tst.tv_sec); } @@ -70,7 +70,7 @@ int main(int argc, char *argv[]) /* Check that a child process is in the new timens. */ for (i = 0; i < 2; i++) { _gettime(CLOCK_MONOTONIC, &tst, i); - if (abs(tst.tv_sec - now.tv_sec - OFFSET) > 5) + if (labs(tst.tv_sec - now.tv_sec - OFFSET) > 5) return pr_fail("%ld %ld\n", now.tv_sec + OFFSET, tst.tv_sec); } -- 2.33.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-05 16:31 ` [PATCH 2/2] selftests: timens: exec: use 'labs()' " Anders Roxell @ 2021-11-05 20:35 ` Nick Desaulniers 2021-11-05 20:45 ` Arnd Bergmann 0 siblings, 1 reply; 11+ messages in thread From: Nick Desaulniers @ 2021-11-05 20:35 UTC (permalink / raw) To: Anders Roxell, Arnd Bergmann Cc: shuah, nathan, linux-kselftest, linux-kernel, llvm On Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > When building selftests/timens with clang, the compiler warn about the > function abs() see below: > > exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] > if (abs(tst.tv_sec - now.tv_sec) > 5) > ^ > exec.c:33:8: note: use function 'labs' instead > if (abs(tst.tv_sec - now.tv_sec) > 5) > ^~~ > labs Careful. Isn't the tv_sec member of `struct timespec` a `time_t` which is 32b on 32b hosts and 64b on 64b hosts? If I'm recalling that correctly, then this patch results in a harmless (though unnecessary) sign extension for 32b targets. That should be fine, but someone like Arnd should triple check if my concern is valid or not. So I'm in favor of this patch (dispatching to abs or labs based on 64b host) would hurt readability. > > The note indicates what to do, Rework to use the function 'labs()'. > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > --- > tools/testing/selftests/timens/exec.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/timens/exec.c b/tools/testing/selftests/timens/exec.c > index e40dc5be2f66..d12ff955de0d 100644 > --- a/tools/testing/selftests/timens/exec.c > +++ b/tools/testing/selftests/timens/exec.c > @@ -30,7 +30,7 @@ int main(int argc, char *argv[]) > > for (i = 0; i < 2; i++) { > _gettime(CLOCK_MONOTONIC, &tst, i); > - if (abs(tst.tv_sec - now.tv_sec) > 5) > + if (labs(tst.tv_sec - now.tv_sec) > 5) > return pr_fail("%ld %ld\n", now.tv_sec, tst.tv_sec); > } > return 0; > @@ -50,7 +50,7 @@ int main(int argc, char *argv[]) > > for (i = 0; i < 2; i++) { > _gettime(CLOCK_MONOTONIC, &tst, i); > - if (abs(tst.tv_sec - now.tv_sec) > 5) > + if (labs(tst.tv_sec - now.tv_sec) > 5) > return pr_fail("%ld %ld\n", > now.tv_sec, tst.tv_sec); > } > @@ -70,7 +70,7 @@ int main(int argc, char *argv[]) > /* Check that a child process is in the new timens. */ > for (i = 0; i < 2; i++) { > _gettime(CLOCK_MONOTONIC, &tst, i); > - if (abs(tst.tv_sec - now.tv_sec - OFFSET) > 5) > + if (labs(tst.tv_sec - now.tv_sec - OFFSET) > 5) > return pr_fail("%ld %ld\n", > now.tv_sec + OFFSET, tst.tv_sec); > } > -- > 2.33.0 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-05 20:35 ` Nick Desaulniers @ 2021-11-05 20:45 ` Arnd Bergmann 2021-11-05 20:50 ` Nick Desaulniers 2021-11-10 18:03 ` [PATCHv2] " Anders Roxell 0 siblings, 2 replies; 11+ messages in thread From: Arnd Bergmann @ 2021-11-05 20:45 UTC (permalink / raw) To: Nick Desaulniers Cc: Anders Roxell, Shuah Khan, Nathan Chancellor, open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List, llvm On Fri, Nov 5, 2021 at 9:35 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > > > When building selftests/timens with clang, the compiler warn about the > > function abs() see below: > > > > exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > ^ > > exec.c:33:8: note: use function 'labs' instead > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > ^~~ > > labs > > Careful. > > Isn't the tv_sec member of `struct timespec` a `time_t` which is 32b > on 32b hosts and 64b on 64b hosts? If I'm recalling that correctly, > then this patch results in a harmless (though unnecessary) sign > extension for 32b targets. That should be fine, but someone like Arnd > should triple check if my concern is valid or not. It could actually be 'int', 'long' or 'long long' depending on the architecture and C library. Maybe we need a temporary variable of type 'long long' to hold the difference, and pass that to llabs()? Arnd ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-05 20:45 ` Arnd Bergmann @ 2021-11-05 20:50 ` Nick Desaulniers 2021-11-10 18:03 ` [PATCHv2] " Anders Roxell 1 sibling, 0 replies; 11+ messages in thread From: Nick Desaulniers @ 2021-11-05 20:50 UTC (permalink / raw) To: Arnd Bergmann Cc: Anders Roxell, Shuah Khan, Nathan Chancellor, open list:KERNEL SELFTEST FRAMEWORK, Linux Kernel Mailing List, llvm On Fri, Nov 5, 2021 at 1:45 PM Arnd Bergmann <arnd@kernel.org> wrote: > > On Fri, Nov 5, 2021 at 9:35 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > On Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > > > > > When building selftests/timens with clang, the compiler warn about the > > > function abs() see below: > > > > > > exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] > > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > > ^ > > > exec.c:33:8: note: use function 'labs' instead > > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > > ^~~ > > > labs > > > > Careful. > > > > Isn't the tv_sec member of `struct timespec` a `time_t` which is 32b > > on 32b hosts and 64b on 64b hosts? If I'm recalling that correctly, > > then this patch results in a harmless (though unnecessary) sign > > extension for 32b targets. That should be fine, but someone like Arnd > > should triple check if my concern is valid or not. > > It could actually be 'int', 'long' or 'long long' depending on the architecture > and C library. Maybe we need a temporary variable of type 'long long' > to hold the difference, and pass that to llabs()? Yeah, that SGTM. Thanks for the review! -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-05 20:45 ` Arnd Bergmann 2021-11-05 20:50 ` Nick Desaulniers @ 2021-11-10 18:03 ` Anders Roxell 2021-11-10 20:09 ` Nick Desaulniers 1 sibling, 1 reply; 11+ messages in thread From: Anders Roxell @ 2021-11-10 18:03 UTC (permalink / raw) To: shuah Cc: nathan, ndesaulniers, linux-kselftest, linux-kernel, llvm, Anders Roxell, Arnd Bergmann When building selftests/timens with clang, the compiler warn about the function abs() see below: exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] if (abs(tst.tv_sec - now.tv_sec) > 5) ^ exec.c:33:8: note: use function 'labs' instead if (abs(tst.tv_sec - now.tv_sec) > 5) ^~~ labs Rework to store the time difference in a 'long long' and pass that to llabs(), since the variable can be an 'int', 'long' or 'long long' depending on the architecture and C library. Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> --- tools/testing/selftests/timens/exec.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/timens/exec.c b/tools/testing/selftests/timens/exec.c index e40dc5be2f66..04439e6ac8a2 100644 --- a/tools/testing/selftests/timens/exec.c +++ b/tools/testing/selftests/timens/exec.c @@ -21,6 +21,7 @@ int main(int argc, char *argv[]) { struct timespec now, tst; + long long timediff; int status, i; pid_t pid; @@ -30,7 +31,8 @@ int main(int argc, char *argv[]) for (i = 0; i < 2; i++) { _gettime(CLOCK_MONOTONIC, &tst, i); - if (abs(tst.tv_sec - now.tv_sec) > 5) + timediff = tst.tv_sec - now.tv_sec; + if (llabs(timediff) > 5) return pr_fail("%ld %ld\n", now.tv_sec, tst.tv_sec); } return 0; @@ -50,7 +52,8 @@ int main(int argc, char *argv[]) for (i = 0; i < 2; i++) { _gettime(CLOCK_MONOTONIC, &tst, i); - if (abs(tst.tv_sec - now.tv_sec) > 5) + timediff = tst.tv_sec - now.tv_sec; + if (llabs(timediff) > 5) return pr_fail("%ld %ld\n", now.tv_sec, tst.tv_sec); } @@ -70,7 +73,8 @@ int main(int argc, char *argv[]) /* Check that a child process is in the new timens. */ for (i = 0; i < 2; i++) { _gettime(CLOCK_MONOTONIC, &tst, i); - if (abs(tst.tv_sec - now.tv_sec - OFFSET) > 5) + timediff = tst.tv_sec - now.tv_sec - OFFSET; + if (llabs(timediff) > 5) return pr_fail("%ld %ld\n", now.tv_sec + OFFSET, tst.tv_sec); } -- 2.33.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-10 18:03 ` [PATCHv2] " Anders Roxell @ 2021-11-10 20:09 ` Nick Desaulniers 2021-11-10 20:11 ` Nick Desaulniers 0 siblings, 1 reply; 11+ messages in thread From: Nick Desaulniers @ 2021-11-10 20:09 UTC (permalink / raw) To: Anders Roxell Cc: shuah, nathan, linux-kselftest, linux-kernel, llvm, Arnd Bergmann On Wed, Nov 10, 2021 at 10:04 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > When building selftests/timens with clang, the compiler warn about the > function abs() see below: > > exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] > if (abs(tst.tv_sec - now.tv_sec) > 5) > ^ > exec.c:33:8: note: use function 'labs' instead > if (abs(tst.tv_sec - now.tv_sec) > 5) > ^~~ > labs > > Rework to store the time difference in a 'long long' and pass that to > llabs(), since the variable can be an 'int', 'long' or 'long long' > depending on the architecture and C library. > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> Thanks for the patch! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > tools/testing/selftests/timens/exec.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/timens/exec.c b/tools/testing/selftests/timens/exec.c > index e40dc5be2f66..04439e6ac8a2 100644 > --- a/tools/testing/selftests/timens/exec.c > +++ b/tools/testing/selftests/timens/exec.c > @@ -21,6 +21,7 @@ > int main(int argc, char *argv[]) > { > struct timespec now, tst; > + long long timediff; > int status, i; > pid_t pid; > > @@ -30,7 +31,8 @@ int main(int argc, char *argv[]) > > for (i = 0; i < 2; i++) { > _gettime(CLOCK_MONOTONIC, &tst, i); > - if (abs(tst.tv_sec - now.tv_sec) > 5) > + timediff = tst.tv_sec - now.tv_sec; > + if (llabs(timediff) > 5) > return pr_fail("%ld %ld\n", now.tv_sec, tst.tv_sec); > } > return 0; > @@ -50,7 +52,8 @@ int main(int argc, char *argv[]) > > for (i = 0; i < 2; i++) { > _gettime(CLOCK_MONOTONIC, &tst, i); > - if (abs(tst.tv_sec - now.tv_sec) > 5) > + timediff = tst.tv_sec - now.tv_sec; > + if (llabs(timediff) > 5) > return pr_fail("%ld %ld\n", > now.tv_sec, tst.tv_sec); > } > @@ -70,7 +73,8 @@ int main(int argc, char *argv[]) > /* Check that a child process is in the new timens. */ > for (i = 0; i < 2; i++) { > _gettime(CLOCK_MONOTONIC, &tst, i); > - if (abs(tst.tv_sec - now.tv_sec - OFFSET) > 5) > + timediff = tst.tv_sec - now.tv_sec - OFFSET; > + if (llabs(timediff) > 5) > return pr_fail("%ld %ld\n", > now.tv_sec + OFFSET, tst.tv_sec); > } > -- > 2.33.0 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-10 20:09 ` Nick Desaulniers @ 2021-11-10 20:11 ` Nick Desaulniers 2021-11-11 8:26 ` Anders Roxell 0 siblings, 1 reply; 11+ messages in thread From: Nick Desaulniers @ 2021-11-10 20:11 UTC (permalink / raw) To: Anders Roxell Cc: shuah, nathan, linux-kselftest, linux-kernel, llvm, Arnd Bergmann On Wed, Nov 10, 2021 at 12:09 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Nov 10, 2021 at 10:04 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > > > When building selftests/timens with clang, the compiler warn about the > > function abs() see below: > > > > exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > ^ > > exec.c:33:8: note: use function 'labs' instead > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > ^~~ > > labs > > > > Rework to store the time difference in a 'long long' and pass that to > > llabs(), since the variable can be an 'int', 'long' or 'long long' > > depending on the architecture and C library. > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > > Thanks for the patch! > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> ah, gmail doesn't do a great job at showing the subject when a v2 is sent in-reply-to. Should the oneline mention llabs rather than labs now? > > > --- > > tools/testing/selftests/timens/exec.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/tools/testing/selftests/timens/exec.c b/tools/testing/selftests/timens/exec.c > > index e40dc5be2f66..04439e6ac8a2 100644 > > --- a/tools/testing/selftests/timens/exec.c > > +++ b/tools/testing/selftests/timens/exec.c > > @@ -21,6 +21,7 @@ > > int main(int argc, char *argv[]) > > { > > struct timespec now, tst; > > + long long timediff; > > int status, i; > > pid_t pid; > > > > @@ -30,7 +31,8 @@ int main(int argc, char *argv[]) > > > > for (i = 0; i < 2; i++) { > > _gettime(CLOCK_MONOTONIC, &tst, i); > > - if (abs(tst.tv_sec - now.tv_sec) > 5) > > + timediff = tst.tv_sec - now.tv_sec; > > + if (llabs(timediff) > 5) > > return pr_fail("%ld %ld\n", now.tv_sec, tst.tv_sec); > > } > > return 0; > > @@ -50,7 +52,8 @@ int main(int argc, char *argv[]) > > > > for (i = 0; i < 2; i++) { > > _gettime(CLOCK_MONOTONIC, &tst, i); > > - if (abs(tst.tv_sec - now.tv_sec) > 5) > > + timediff = tst.tv_sec - now.tv_sec; > > + if (llabs(timediff) > 5) > > return pr_fail("%ld %ld\n", > > now.tv_sec, tst.tv_sec); > > } > > @@ -70,7 +73,8 @@ int main(int argc, char *argv[]) > > /* Check that a child process is in the new timens. */ > > for (i = 0; i < 2; i++) { > > _gettime(CLOCK_MONOTONIC, &tst, i); > > - if (abs(tst.tv_sec - now.tv_sec - OFFSET) > 5) > > + timediff = tst.tv_sec - now.tv_sec - OFFSET; > > + if (llabs(timediff) > 5) > > return pr_fail("%ld %ld\n", > > now.tv_sec + OFFSET, tst.tv_sec); > > } > > -- > > 2.33.0 > > > > > -- > Thanks, > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] selftests: timens: exec: use 'labs()' over 'abs()' 2021-11-10 20:11 ` Nick Desaulniers @ 2021-11-11 8:26 ` Anders Roxell 0 siblings, 0 replies; 11+ messages in thread From: Anders Roxell @ 2021-11-11 8:26 UTC (permalink / raw) To: Nick Desaulniers Cc: shuah, nathan, linux-kselftest, linux-kernel, llvm, Arnd Bergmann On Wed, 10 Nov 2021 at 21:11, Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Nov 10, 2021 at 12:09 PM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Wed, Nov 10, 2021 at 10:04 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > > > > > When building selftests/timens with clang, the compiler warn about the > > > function abs() see below: > > > > > > exec.c:33:8: error: absolute value function 'abs' given an argument of type 'long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] > > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > > ^ > > > exec.c:33:8: note: use function 'labs' instead > > > if (abs(tst.tv_sec - now.tv_sec) > 5) > > > ^~~ > > > labs > > > > > > Rework to store the time difference in a 'long long' and pass that to > > > llabs(), since the variable can be an 'int', 'long' or 'long long' > > > depending on the architecture and C library. > > > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de> > > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > > > > Thanks for the patch! > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > ah, gmail doesn't do a great job at showing the subject when a v2 is > sent in-reply-to. oh right, sorry. > Should the oneline mention llabs rather than labs > now? You are correct, can this be changed when the patch gets applied or should I send a v3? Cheers, Anders ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] selftests: timens: use 'llabs()' over 'abs()' 2021-11-05 16:31 [PATCH 1/2] selftests: timens: use 'llabs()' over 'abs()' Anders Roxell 2021-11-05 16:31 ` [PATCH 2/2] selftests: timens: exec: use 'labs()' " Anders Roxell @ 2021-11-05 20:26 ` Nick Desaulniers 2021-11-20 0:18 ` Shuah Khan 1 sibling, 1 reply; 11+ messages in thread From: Nick Desaulniers @ 2021-11-05 20:26 UTC (permalink / raw) To: Anders Roxell; +Cc: shuah, nathan, linux-kselftest, linux-kernel, llvm On Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > When building selftests/timens with clang, the compiler warn about the > function abs() see below: > > timerfd.c:64:7: error: absolute value function 'abs' given an argument of type 'long long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] > if (abs(elapsed - 3600) > 60) { > ^ > timerfd.c:64:7: note: use function 'llabs' instead > if (abs(elapsed - 3600) > 60) { > ^~~ > llabs > > The note indicates what to do, Rework to use the function 'llabs()'. > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> Thanks for the patch! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > --- > tools/testing/selftests/timens/timer.c | 2 +- > tools/testing/selftests/timens/timerfd.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/timens/timer.c b/tools/testing/selftests/timens/timer.c > index 5e7f0051bd7b..5b939f59dfa4 100644 > --- a/tools/testing/selftests/timens/timer.c > +++ b/tools/testing/selftests/timens/timer.c > @@ -56,7 +56,7 @@ int run_test(int clockid, struct timespec now) > return pr_perror("timerfd_gettime"); > > elapsed = new_value.it_value.tv_sec; > - if (abs(elapsed - 3600) > 60) { > + if (llabs(elapsed - 3600) > 60) { > ksft_test_result_fail("clockid: %d elapsed: %lld\n", > clockid, elapsed); > return 1; > diff --git a/tools/testing/selftests/timens/timerfd.c b/tools/testing/selftests/timens/timerfd.c > index 9edd43d6b2c1..a4196bbd6e33 100644 > --- a/tools/testing/selftests/timens/timerfd.c > +++ b/tools/testing/selftests/timens/timerfd.c > @@ -61,7 +61,7 @@ int run_test(int clockid, struct timespec now) > return pr_perror("timerfd_gettime(%d)", clockid); > > elapsed = new_value.it_value.tv_sec; > - if (abs(elapsed - 3600) > 60) { > + if (llabs(elapsed - 3600) > 60) { > ksft_test_result_fail("clockid: %d elapsed: %lld\n", > clockid, elapsed); > return 1; > -- > 2.33.0 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] selftests: timens: use 'llabs()' over 'abs()' 2021-11-05 20:26 ` [PATCH 1/2] selftests: timens: use 'llabs()' " Nick Desaulniers @ 2021-11-20 0:18 ` Shuah Khan 0 siblings, 0 replies; 11+ messages in thread From: Shuah Khan @ 2021-11-20 0:18 UTC (permalink / raw) To: Nick Desaulniers, Anders Roxell Cc: shuah, nathan, linux-kselftest, linux-kernel, llvm, Shuah Khan On 11/5/21 2:26 PM, Nick Desaulniers wrote: > On Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <anders.roxell@linaro.org> wrote: >> >> When building selftests/timens with clang, the compiler warn about the >> function abs() see below: >> >> timerfd.c:64:7: error: absolute value function 'abs' given an argument of type 'long long' but has parameter of type 'int' which may cause truncation of value [-Werror,-Wabsolute-value] >> if (abs(elapsed - 3600) > 60) { >> ^ >> timerfd.c:64:7: note: use function 'llabs' instead >> if (abs(elapsed - 3600) > 60) { >> ^~~ >> llabs >> >> The note indicates what to do, Rework to use the function 'llabs()'. >> >> Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > > Thanks for the patch! > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > >> --- >> tools/testing/selftests/timens/timer.c | 2 +- >> tools/testing/selftests/timens/timerfd.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/tools/testing/selftests/timens/timer.c b/tools/testing/selftests/timens/timer.c >> index 5e7f0051bd7b..5b939f59dfa4 100644 >> --- a/tools/testing/selftests/timens/timer.c >> +++ b/tools/testing/selftests/timens/timer.c >> @@ -56,7 +56,7 @@ int run_test(int clockid, struct timespec now) >> return pr_perror("timerfd_gettime"); >> >> elapsed = new_value.it_value.tv_sec; >> - if (abs(elapsed - 3600) > 60) { >> + if (llabs(elapsed - 3600) > 60) { >> ksft_test_result_fail("clockid: %d elapsed: %lld\n", >> clockid, elapsed); >> return 1; >> diff --git a/tools/testing/selftests/timens/timerfd.c b/tools/testing/selftests/timens/timerfd.c >> index 9edd43d6b2c1..a4196bbd6e33 100644 >> --- a/tools/testing/selftests/timens/timerfd.c >> +++ b/tools/testing/selftests/timens/timerfd.c >> @@ -61,7 +61,7 @@ int run_test(int clockid, struct timespec now) >> return pr_perror("timerfd_gettime(%d)", clockid); >> >> elapsed = new_value.it_value.tv_sec; >> - if (abs(elapsed - 3600) > 60) { >> + if (llabs(elapsed - 3600) > 60) { >> ksft_test_result_fail("clockid: %d elapsed: %lld\n", >> clockid, elapsed); >> return 1; >> -- >> 2.33.0 >> > > Same comment on llabs() define in stdlib.h made earlier in the context of timer test changes. thanks, -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-11-20 0:18 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-05 16:31 [PATCH 1/2] selftests: timens: use 'llabs()' over 'abs()' Anders Roxell 2021-11-05 16:31 ` [PATCH 2/2] selftests: timens: exec: use 'labs()' " Anders Roxell 2021-11-05 20:35 ` Nick Desaulniers 2021-11-05 20:45 ` Arnd Bergmann 2021-11-05 20:50 ` Nick Desaulniers 2021-11-10 18:03 ` [PATCHv2] " Anders Roxell 2021-11-10 20:09 ` Nick Desaulniers 2021-11-10 20:11 ` Nick Desaulniers 2021-11-11 8:26 ` Anders Roxell 2021-11-05 20:26 ` [PATCH 1/2] selftests: timens: use 'llabs()' " Nick Desaulniers 2021-11-20 0:18 ` Shuah Khan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.