Re: latest gcc vs lib/timespec.h:85

[Paul Eggert]

On 12/14/2017 12:25 AM, Tim Rühsen wrote:

Does it fix things to add -Wshorten-64-to-32 to build-aux/gcc-warning.spec and to 
build-aux/g++-warning.spec?  > No, it doesn't change anything (I am not using 
manywarnings.m4).
OK, in that case you should be able to fix the problem by specifying 
-Wno-shorten-64-to-32 in addition to whatever other flags that you are 
specifying by hand.

Re: latest gcc vs lib/timespec.h:85

[Tim Rühsen]

On 12/13/2017 10:55 PM, Paul Eggert wrote:
> On 12/13/2017 01:32 AM, Tim Rühsen wrote:
>> Now clang throws out an annoying warning about the return value of  >
>> timespec_cmp(): > > In file included from wget.c:51: > 
> ../lib/timespec.h:94:20: warning: implicit conversion loses integer >
> precision: 'long' to 'int' [-Wshorten-64-to-32] > return a.tv_nsec -
> b.tv_nsec; > ~~ ~~^~~ > > I wonder if we can't
> silence clang and gcc by keeping the 'assume()' > *and* using return
> (int) (a.tv_nsec - b.tv_nsec));
> I'd rather continue to omit the cast, as casts are too powerful (it's
> too easy to get them wrong, with no diagnostic).
> 
> -Wshorten-64-to-32 is like -Wconversion, and we should ask people not to
> use -Wshorten-64-to-32 in the same way that we ask them not to use
> -Wconversion. Does it fix things to add -Wshorten-64-to-32 to
> build-aux/gcc-warning.spec and to build-aux/g++-warning.spec?

No, it doesn't change anything (I am not using manywarnings.m4).

What about a #pragma here ?

And of course I can disable the warning for the gnulib code alone...
that isn't the point.
Switching on warnings for the gnulib code was meant as a help in the
means "more eyes see more things". It was once was a developers who
asked for not switching off warnings in gnulib code and I agreed. But I
don't definitely don't want to waste both your and my time with nitpicking.

With Best Regards, Tim



signature.asc
Description: OpenPGP digital signature

Re: latest gcc vs lib/timespec.h:85

[Paul Eggert]

On 12/13/2017 01:32 AM, Tim Rühsen wrote:
Now clang throws out an annoying warning about the return value of  > timespec_cmp(): > > In file included from wget.c:51: > 
../lib/timespec.h:94:20: warning: implicit conversion loses integer > 
precision: 'long' to 'int' [-Wshorten-64-to-32] > return a.tv_nsec - 
b.tv_nsec; > ~~ ~~^~~ > > I wonder if we can't 
silence clang and gcc by keeping the 'assume()' > *and* using return 
(int) (a.tv_nsec - b.tv_nsec));
I'd rather continue to omit the cast, as casts are too powerful (it's 
too easy to get them wrong, with no diagnostic).


-Wshorten-64-to-32 is like -Wconversion, and we should ask people not to 
use -Wshorten-64-to-32 in the same way that we ask them not to use 
-Wconversion. Does it fix things to add -Wshorten-64-to-32 to 
build-aux/gcc-warning.spec and to build-aux/g++-warning.spec?

Re: latest gcc vs lib/timespec.h:85

[Tim Rühsen]

On 10/30/2017 12:43 AM, Jim Meyering wrote:
> On Sun, Oct 29, 2017 at 4:27 PM, Paul Eggert  wrote:
>> Jim Meyering wrote:
>>>
>>> Here's a proposed patch:
>>
>> 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.

Now clang throws out an annoying warning about the return value of
timespec_cmp():

In file included from wget.c:51:
../lib/timespec.h:94:20: warning: implicit conversion loses integer
precision: 'long' to 'int' [-Wshorten-64-to-32]
  return a.tv_nsec - b.tv_nsec;
  ~~ ~~^~~

I wonder if we can't silence clang and gcc by keeping the 'assume()'
*and* using return (int) (a.tv_nsec - b.tv_nsec));

WDYT ?

With Best Regards, Tim



signature.asc
Description: OpenPGP digital signature

Re: latest gcc vs lib/timespec.h:85

[Jim Meyering]

On Sun, Oct 29, 2017 at 4:27 PM, Paul Eggert  wrote:
> Jim Meyering wrote:
>>
>> Here's a proposed patch:
>
> 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.

Re: latest gcc vs lib/timespec.h:85

[Paul Eggert]

Jim Meyering wrote:

Here's a proposed patch:


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.


From f466816e06ec3516567c3edcd0219bd1f9b736eb Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Sun, 29 Oct 2017 16:22:41 -0700
Subject: [PATCH] =?UTF-8?q?timespec:=20prefer=20=E2=80=98assume=E2=80=99?=
 =?UTF-8?q?=20to=20=E2=80=98assure=E2=80=99?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This avoids some runtime tests.  The rest of the module makes
similar assumptions and there is little point to testing here.
* lib/timespec.h: Include verify.h instead of assure.h.
(timespec_cmp): Use ‘assume’, not ‘assure’.
Also, remove an unnecessary cast to ‘int’, as lots of other
code in this module now causes -Wconversion to complain, and
this is a problem with -Wconversion not with the code.

* modules/timespec (Depends-on): Depend on ‘verify’, not ‘assure’.
---
 ChangeLog| 11 +++
 lib/timespec.h   | 26 ++
 modules/timespec |  2 +-
 3 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 16506ba..87a9297 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2017-10-29  Paul Eggert  
 
+   timespec: prefer ‘assume’ to ‘assure’
+   This avoids some runtime tests.  The rest of the module makes
+   similar assumptions and there is little point to testing here.
+   * lib/timespec.h: Include verify.h instead of assure.h.
+   (timespec_cmp): Use ‘assume’, not ‘assure’.
+   Also, remove an unnecessary cast to ‘int’, as lots of other
+   code in this module now causes -Wconversion to complain, and
+   this is a problem with -Wconversion not with the code.
+
+   * modules/timespec (Depends-on): Depend on ‘verify’, not ‘assure’.
+
Port recent gnulib-tool change to Dash
* gnulib-tool (func_create_testdir): Don't assume that the shell
retokenizes after expanding "$@" inside the call to
diff --git a/lib/timespec.h b/lib/timespec.h
index 61cfebb..cc34067 100644
--- a/lib/timespec.h
+++ b/lib/timespec.h
@@ -33,7 +33,7 @@ _GL_INLINE_HEADER_BEGIN
 extern "C" {
 #endif
 
-#include "assure.h"
+#include "verify.h"
 
 /* Resolution of timespec timestamps (in units per second), and log
base 10 of the resolution.  */
@@ -69,27 +69,29 @@ make_timespec (time_t s, long int ns)
any platform of interest to the GNU project, since all such
platforms have 32-bit int or wider.
 
-   Replacing "(int) (a.tv_nsec - b.tv_nsec)" with something like
+   Replacing "a.tv_nsec - b.tv_nsec" with something like
"a.tv_nsec < b.tv_nsec ? -1 : a.tv_nsec > b.tv_nsec" would cause
this function to work in some cases where the above assumption is
violated, but not in all cases (e.g., a.tv_sec==1, a.tv_nsec==-2,
b.tv_sec==0, b.tv_nsec==9) and is arguably not worth the
extra instructions.  Using a subtraction has the advantage of
detecting some invalid cases on platforms that detect integer
-   overflow.
-
-   The (int) cast avoids a gcc -Wconversion warning.  */
+   overflow.  */
 
 _GL_TIMESPEC_INLINE int _GL_ATTRIBUTE_PURE
 timespec_cmp (struct timespec a, struct timespec b)
 {
-  /* These assure calls teach gcc7 enough so that its
- -Wstrict-overflow does not complain about the following code.  */
-  assure (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
-  assure (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
-  return (a.tv_sec < b.tv_sec ? -1
-  : a.tv_sec > b.tv_sec ? 1
-  : (int) (a.tv_nsec - b.tv_nsec));
+  if (a.tv_sec < b.tv_sec)
+return -1;
+  if (a.tv_sec > b.tv_sec)
+return 1;
+
+  /* Pacify gcc -Wstrict-overflow (bleeding-edge circa 2017-10-02).  See:
+ http://lists.gnu.org/archive/html/bug-gnulib/2017-10/msg6.html  */
+  assume (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
+  assume (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
+
+  return a.tv_nsec - b.tv_nsec;
 }
 
 /* Return -1, 0, 1, depending on the sign of A.  A.tv_nsec must be
diff --git a/modules/timespec b/modules/timespec
index 01ab6ad..e6e1514 100644
--- a/modules/timespec
+++ b/modules/timespec
@@ -7,9 +7,9 @@ lib/timespec.c
 m4/timespec.m4
 
 Depends-on:
-assure
 extern-inline
 time
+verify
 
 configure.ac:
 gl_TIMESPEC
-- 
2.7.4

Re: latest gcc vs lib/timespec.h:85

[Jim Meyering]

On Fri, Oct 27, 2017 at 9:33 PM, Jim Meyering  wrote:
> On Mon, Oct 2, 2017 at 6:31 PM, Paul Eggert  wrote:
>> On 10/02/2017 06:24 PM, Jim Meyering wrote:
>>>
>>> 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.
>
> Thanks. I prefer that. Here's a proposed patch:

Pushed.

Re: latest gcc vs lib/timespec.h:85

[Jim Meyering]

On Mon, Oct 2, 2017 at 6:31 PM, Paul Eggert  wrote:
> On 10/02/2017 06:24 PM, Jim Meyering wrote:
>>
>> 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.

Thanks. I prefer that. Here's a proposed patch:
From c587f5cff388417f5c584a7125cc886df9750c9b Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Fri, 27 Oct 2017 21:28:47 -0700
Subject: [PATCH] timespec.h: use "assure" to avoid a spurious warning

* lib/timespec.h: Include "assure.h" and use it to help
gcc7's -Wstrict-overflow avoid a false positive warning
for a use in coreutils' ls.c.  Suggested by Paul Eggert in
https://lists.gnu.org/r/bug-gnulib/2017-10/msg7.html
* modules/timespec (Depends-on): Add assure.
---
 ChangeLog| 9 +
 lib/timespec.h   | 6 ++
 modules/timespec | 1 +
 3 files changed, 16 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 7ce63c22f..e31bb6dc4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2017-10-27  Jim Meyering  
+
+   timespec.h: use "assure" to avoid a spurious warning
+   * lib/timespec.h: Include "assure.h" and use it to help
+   gcc7's -Wstrict-overflow avoid a false positive warning
+   for a use in coreutils' ls.c.  Suggested by Paul Eggert in
+   https://lists.gnu.org/r/bug-gnulib/2017-10/msg7.html
+   * modules/timespec (Depends-on): Add assure.
+
 2017-10-26  Bruno Haible  

havelib: Fix value of LD for 32-bit compilation on NetBSD/sparc64.
diff --git a/lib/timespec.h b/lib/timespec.h
index 383130157..61cfebbea 100644
--- a/lib/timespec.h
+++ b/lib/timespec.h
@@ -33,6 +33,8 @@ _GL_INLINE_HEADER_BEGIN
 extern "C" {
 #endif

+#include "assure.h"
+
 /* Resolution of timespec timestamps (in units per second), and log
base 10 of the resolution.  */

@@ -81,6 +83,10 @@ make_timespec (time_t s, long int ns)
 _GL_TIMESPEC_INLINE int _GL_ATTRIBUTE_PURE
 timespec_cmp (struct timespec a, struct timespec b)
 {
+  /* These assure calls teach gcc7 enough so that its
+ -Wstrict-overflow does not complain about the following code.  */
+  assure (-1 <= a.tv_nsec && a.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
+  assure (-1 <= b.tv_nsec && b.tv_nsec <= 2 * TIMESPEC_RESOLUTION);
   return (a.tv_sec < b.tv_sec ? -1
   : a.tv_sec > b.tv_sec ? 1
   : (int) (a.tv_nsec - b.tv_nsec));
diff --git a/modules/timespec b/modules/timespec
index d18d1464f..01ab6add2 100644
--- a/modules/timespec
+++ b/modules/timespec
@@ -7,6 +7,7 @@ lib/timespec.c
 m4/timespec.m4

 Depends-on:
+assure
 extern-inline
 time

-- 
2.14.1.729.g59c0ea183

Re: latest gcc vs lib/timespec.h:85

[Paul Eggert]

On 10/02/2017 06:24 PM, Jim Meyering wrote:

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.

latest gcc vs lib/timespec.h:85

[Jim Meyering]

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.