Re: Warning in spawn-pipe.c (create_pipe)

[Eric Blake]

On 12/13/2017 04:24 PM, Bruno Haible wrote:

> 1) It is not a goal to have absolutely no warnings with GCC or
>with clang. It is perfectly OK, IMO, if a compilation with "gcc -Wall"
>shows, say, 5 warnings in 10 files. The maintainer will get used to
>these warnings and see new warnings when they arise.
> 
> 2) For the problem of uninitialized variables that lead to undefined
>behaviour, I don't see a GCC option that would warn about them [1].
>Instead, 'valgrind' is the ultimate tool for detecting these kinds
>of problems.
>So if someone has a habit of looking only at GCC warnings, they should
>change their habit and also use valgrind once in a while.
> 
> 3) Adding the 'child = 0' initialization has the effect that it will
>silence both the clang warning and valgrind. However, if there is
>a bug in the code that forgets to assign a value to the variable, the
>bug will not be detected. In other words, the bug will still be present
>but will be deterministic. => Such an initialization is a plus in
>some situations (when debugging with gdb) and a minus in others.
> 
> Is '-Wconditional-uninitialized' implied in -Wall? If yes, I vote for
> adding '-Wno-conditional-uninitialized' at least for this specific file
> (through a Makefile.am variable).

Or through an in-file pragma that specifically documents that we are
intentionally ignoring the warning.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: Warning in spawn-pipe.c (create_pipe)

[Bruno Haible]

Hi Tim,

> clang's warning:
> 
> spawn-pipe.c:364:34: warning: variable 'child' may be uninitialized when
> used here [-Wconditional-uninitialized]
>   register_slave_subprocess (child);
>  ^

I agree with your analysis that without massive inlining or other propagation,
the compiler cannot know whether the previous call to
  posix_spawnp (, ...)
will have initialized the variable 'child'.

So this class of warning is of the category "don't turn it on by default
if you want to have a warning-free build; but do turn it on occasionally
if you find that it detects real bugs in your code".

> That's why I vote for initializing 'child' to 0 as suggested below. It
> seems to be good programming practice.

No. Not for me.

1) It is not a goal to have absolutely no warnings with GCC or
   with clang. It is perfectly OK, IMO, if a compilation with "gcc -Wall"
   shows, say, 5 warnings in 10 files. The maintainer will get used to
   these warnings and see new warnings when they arise.

2) For the problem of uninitialized variables that lead to undefined
   behaviour, I don't see a GCC option that would warn about them [1].
   Instead, 'valgrind' is the ultimate tool for detecting these kinds
   of problems.
   So if someone has a habit of looking only at GCC warnings, they should
   change their habit and also use valgrind once in a while.

3) Adding the 'child = 0' initialization has the effect that it will
   silence both the clang warning and valgrind. However, if there is
   a bug in the code that forgets to assign a value to the variable, the
   bug will not be detected. In other words, the bug will still be present
   but will be deterministic. => Such an initialization is a plus in
   some situations (when debugging with gdb) and a minus in others.

Is '-Wconditional-uninitialized' implied in -Wall? If yes, I vote for
adding '-Wno-conditional-uninitialized' at least for this specific file
(through a Makefile.am variable).

Bruno

[1] https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html

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?

Warning in spawn-pipe.c (create_pipe)

[Tim Rühsen]

This seems to be a false positive from clang - but how does a compiler
know for sure that a function (posix_spawnp) always initializes a
pointer argument when returning 0 ? Ok, it's written in the docs and we
know it but there is no syntax to tell the compiler exactly that.

That's why I vote for initializing 'child' to 0 as suggested below. It
seems to be good programming practice.

clang's warning:

spawn-pipe.c:364:34: warning: variable 'child' may be uninitialized when
used here [-Wconditional-uninitialized]
  register_slave_subprocess (child);
 ^
spawn-pipe.c:257:14: note: initialize the variable 'child' to silence
this warning
  pid_t child;
 ^
  = 0




With Best Regards, Tim




signature.asc
Description: OpenPGP digital signature

Unneeded compiler warning in glob.c

[Tim Rühsen]

Here is a patch to silence this warning:

glob.c: In function 'rpl_glob':
glob.c:618:64: warning: pointer of type 'void *' used in arithmetic
[-Wpointer-arith]
   err = getpwnam_r (s.data, , s.data + ssize,


With Best Regards, Tim

From af192d3b675182c3944bfb14488942f4f55d63fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20R=C3=BChsen?= 
Date: Wed, 13 Dec 2017 10:37:08 +0100
Subject: [PATCH] glob.c: Silence warning about void pointer arithmetic

---
 lib/glob.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/glob.c b/lib/glob.c
index 511be12dd..563f580d5 100644
--- a/lib/glob.c
+++ b/lib/glob.c
@@ -615,7 +615,7 @@ glob (const char *pattern, int flags, int (*errfunc) (const char *, int),
 {
 # if defined HAVE_GETPWNAM_R || defined _LIBC
   size_t ssize = strlen (s.data) + 1;
-  err = getpwnam_r (s.data, , s.data + ssize,
+  err = getpwnam_r (s.data, , ((char *) s.data) + ssize,
 s.length - ssize, );
 # else
   p = getpwnam (s.data);
-- 
2.15.1



signature.asc
Description: OpenPGP digital signature

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