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

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

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