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 <[email protected]>
---
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 Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <[email protected]> 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 <[email protected]>
> ---
> 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
On Fri, Nov 5, 2021 at 9:35 PM Nick Desaulniers <[email protected]> wrote:
>
> On Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <[email protected]> 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
On Fri, Nov 5, 2021 at 1:45 PM Arnd Bergmann <[email protected]> wrote:
>
> On Fri, Nov 5, 2021 at 9:35 PM Nick Desaulniers <[email protected]> wrote:
> >
> > On Fri, Nov 5, 2021 at 9:31 AM Anders Roxell <[email protected]> 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
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 <[email protected]>
Signed-off-by: Anders Roxell <[email protected]>
---
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 Wed, Nov 10, 2021 at 10:04 AM Anders Roxell <[email protected]> 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 <[email protected]>
> Signed-off-by: Anders Roxell <[email protected]>
Thanks for the patch!
Reviewed-by: Nick Desaulniers <[email protected]>
> ---
> 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
On Wed, Nov 10, 2021 at 12:09 PM Nick Desaulniers
<[email protected]> wrote:
>
> On Wed, Nov 10, 2021 at 10:04 AM Anders Roxell <[email protected]> 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 <[email protected]>
> > Signed-off-by: Anders Roxell <[email protected]>
>
> Thanks for the patch!
> Reviewed-by: Nick Desaulniers <[email protected]>
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
On Wed, 10 Nov 2021 at 21:11, Nick Desaulniers <[email protected]> wrote:
>
> On Wed, Nov 10, 2021 at 12:09 PM Nick Desaulniers
> <[email protected]> wrote:
> >
> > On Wed, Nov 10, 2021 at 10:04 AM Anders Roxell <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Anders Roxell <[email protected]>
> >
> > Thanks for the patch!
> > Reviewed-by: Nick Desaulniers <[email protected]>
>
> 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