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

[Bruno Haible]

Hi Tim,

> > 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.
> 
> That is really bad and it makes me sad. You are saying it's a good thing
> to get used to a bad situation. I hope you don't mean it.

Sorry but why do you call it "really bad", given that these warnings
  - are so few that the maintainer is not hindered in their development,
  - we know that these warnings are false positives?

> > 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.
> 
> ... the quality of Valgrind
> depends on the code path coverage - that is not the same as code
> coverage. To get the test data to cover most code paths you need a
> fuzzer, at least for the more complex functions / functionality. Writing
> good fuzzers takes time and sometimes need a lot of human time for
> tuning.

I did not say anything negative about fuzzying. Like you say, efforts on
valgrind testing and efforts on fuzzying are complementary: With the
fuzzying you increase the code coverage and code path coverage; with
valgrind you check against undefined behaviour caused by uninitialized
variables.

> since there are
> tests in glibc/gnulib for glob() that are also used with Valgrind,
> aren't there ?

We do have a problem with the valgrind integration into projects that use
Automake: There's not one clearly best way to use valgrind that is documented,
therefore every package maintainer spends time on a valgrind integration.

Bruno

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

[Bruno Haible]

Hi Tim,

> > Or through an in-file pragma that specifically documents that we are
> > intentionally ignoring the warning.
> 
> I vote for this.

Does this patch eliminate the warning?

I couldn't reproduce the issue with clang 3.9.1 on Linux, therefore I have
to ask you to test it.

Bruno

diff --git a/lib/spawn-pipe.c b/lib/spawn-pipe.c
index cbcc4ee..3170a4f 100644
--- a/lib/spawn-pipe.c
+++ b/lib/spawn-pipe.c
@@ -16,6 +16,11 @@
along with this program.  If not, see .  */
 
 
+/* Tell clang not to warn about the 'child' variable, below.  */
+#if defined __clang__
+# pragma clang diagnostic ignored "-Wconditional-uninitialized"
+#endif
+
 #include 
 
 /* Specification.  */

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

[Tim Rühsen]

On 12/13/2017 11:24 PM, Bruno Haible wrote:
> 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 (&child, ...)
> 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.

That is really bad and it makes me sad. You are saying it's a good thing
to get used to a bad situation. I hope you don't mean it.


> 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.

Valgrind is not the ultimate tool. It is just one tool (or one line of
defense) out of several. One is the coder itself, another one is the
compiler with it's code analysis. Before I start testing with valgrind,
most all of the compiler warnings have to be dealt with.

Valgrind is slow, especially if you are testing applications (think of
(then)thousands of valgrind invocations). And the quality of Valgrind
depends on the code path coverage - that is not the same as code
coverage. To get the test data to cover most code paths you need a
fuzzer, at least for the more complex functions / functionality. Writing
good fuzzers takes time and sometimes need a lot of human time for
tuning. If you have done that you are ready to use Valgrind in a pretty
reliable way for regression testing, at least you are beyond
'random|human' code path coverage.

BTW, the recent findings in glob() were random findings when fuzzing
wget2 (which uses glob() at some point). Just to underline the power of
fuzzing in comparison with human generated test data - since there are
tests in glibc/gnulib for glob() that are also used with Valgrind,
aren't there ?

Aiming at fuzzing glob() to get full code path coverage likely reveals
more issues. I wish I had the time to work on fuzzing gnulib... but
currently I don't see any time window for the future.

> 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.

Catched automatically by (description above).


With Best Regards, Tim




signature.asc
Description: OpenPGP digital signature

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

[Tim Rühsen]

On 12/13/2017 11:29 PM, Eric Blake wrote:
> On 12/13/2017 04:24 PM, Bruno Haible wrote:
> 
>> 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.

I vote for this.

Regards, Tim



signature.asc
Description: OpenPGP digital signature

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