Discussion:
latest gcc vs lib/timespec.h:85
Add Reply
Jim Meyering
2017-10-03 01:24:09 UTC
Reply
Permalink
Raw Message
Hi Paul,
Mainly just a heads up, since this certainly isn't blocking me.

When trying to build coreutils using gcc built from very recent (with
some change committed since Sep 26), I see this new warning/error:

In file included from src/system.h:140:0,
from src/ls.c:84:
src/ls.c: In function 'print_long_format':
./lib/timespec.h:85:11: error: assuming signed overflow does not occur
when simplifying conditional to constant [-Werror=strict-overflow]
return (a.tv_sec < b.tv_sec ? -1
~~~~~~~~~~~~~~~~~~~~~~~~~
: a.tv_sec > b.tv_sec ? 1
^~~~~~~~~~~~~~~~~~~~~~~~~
: (int) (a.tv_nsec - b.tv_nsec));
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

When compiling with gcc built from latest sources on September 26,
there is no such problem.

Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.
Paul Eggert
2017-10-03 01:31:37 UTC
Reply
Permalink
Raw Message
Post by Jim Meyering
Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.
That would work. Another possibility would be to include verify.h and
add something like this to the start of timespec_cmp:

  assume (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);

  assume (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);

We might be able to make these 'assume' calls fancier, to exactly match
the comments, but I'm not sure it's worth the bother.
Jim Meyering
2017-10-28 04:33:41 UTC
Reply
Permalink
Raw Message
Post by Jim Meyering
Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.
That would work. Another possibility would be to include verify.h and add
assume (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
assume (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
We might be able to make these 'assume' calls fancier, to exactly match the
comments, but I'm not sure it's worth the bother.
Thanks. I prefer that. Here's a proposed patch:
Jim Meyering
2017-10-29 21:51:01 UTC
Reply
Permalink
Raw Message
Post by Jim Meyering
Given all of the comments on that function, I'd be tempted to suppress
this warning in that function.
That would work. Another possibility would be to include verify.h and add
assume (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
assume (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
We might be able to make these 'assume' calls fancier, to exactly match the
comments, but I'm not sure it's worth the bother.
Pushed.
Paul Eggert
2017-10-29 23:27:12 UTC
Reply
Permalink
Raw Message
I prefer 'assume' to 'assure' here, since it's a low-level time-comparison
primitive and lots of other code in the module already silently assumes that the
timestamps are valid. Also, while I was in the neighborhood I noticed that the
cast is no longer needed, since the module provokes -Wconversion warnings in
several other places now (and I expect nobody notices because nobody looks at
those warnings any more). So I installed the attached followup.
Jim Meyering
2017-10-29 23:43:52 UTC
Reply
Permalink
Raw Message
Post by Paul Eggert
I prefer 'assume' to 'assure' here, since it's a low-level time-comparison
primitive and lots of other code in the module already silently assumes that
the timestamps are valid. Also, while I was in the neighborhood I noticed
that the cast is no longer needed, since the module provokes -Wconversion
warnings in several other places now (and I expect nobody notices because
nobody looks at those warnings any more). So I installed the attached
followup.
Oh, yes. Definitely prefer assume. Thanks for the fix.

Loading...