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

[Bruno Haible]

Hi Tim,

Post by Tim Rühsen
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".

Post by Tim Rühsen
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

[Eric Blake]

Post by Bruno Haible
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.

[Tim Rühsen]

Post by Eric Blake

Post by Bruno Haible
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

[Bruno Haible]

Hi Tim,

Post by Tim Rühsen

Post by Eric Blake
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 <https://www.gnu.org/licenses/>. */


+/* Tell clang not to warn about the 'child' variable, below. */
+#if defined __clang__
+# pragma clang diagnostic ignored "-Wconditional-uninitialized"
+#endif
+
#include <config.h>

/* Specification. */

[Tim Rühsen]

On 12/15/2017 12:28 AM, Bruno Haible wrote:
Hi Bruno,

Post by Bruno Haible

Post by Tim Rühsen

Post by Eric Blake
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.

It works, but... ;-)

Your patch disables that warning for the whole file. IMO, we should keep
the scope of the #pragma as narrow as possible.

We could have a pragma.h include file with the following C99 code (I
leave the clang and gcc version checks to you):

#define STRINGIFY(a) #a

#if defined __clang__
# define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
_Pragma( STRINGIFY( clang diagnostic push ) ) \
_Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#elif defined __GNUC__
# define NOWARN(a) _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
_Pragma( STRINGIFY( gcc diagnostic push ) ) \
_Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#else
# define NOWARN
# define NOWARN_PUSH
# define NOWARN_POP
#endif

Now we could include that file where needed and make use of the defines like

NOWARN("-Wconditional-uninitialized")

or to narrow the scope

NOWARN_PUSH("-Wconditional-uninitialized")
... code ...
NOWARN_POP


WDYT ?

Regards, Tim

[Tim Rühsen]

Post by Bruno Haible
#if defined __clang__
# define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
_Pragma( STRINGIFY( clang diagnostic push ) ) \
_Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )

^^ last second typo here - replace 'gcc' with 'clang'

[Bruno Haible]

Post by Tim Rühsen
It works, but... ;-)

OK, I've pushed it.

Post by Tim Rühsen
Your patch disables that warning for the whole file. IMO, we should keep
the scope of the #pragma as narrow as possible.
We could have a pragma.h include file with the following C99 code (I
#define STRINGIFY(a) #a
#if defined __clang__
# define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
_Pragma( STRINGIFY( clang diagnostic push ) ) \
_Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#elif defined __GNUC__
# define NOWARN(a) _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
_Pragma( STRINGIFY( gcc diagnostic push ) ) \
_Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#else
# define NOWARN
# define NOWARN_PUSH
# define NOWARN_POP
#endif

Indeed such macrology belongs in one single file. Would you like to provide
a patch?

The macros should cater for the case the gcc and clang have different warning
options (see e.g. anytostr.c).

Also if NOWARN and NOWARN_PUSH take an argument in one case, they need to take
an argument in the #else case as well.

Bruno

[Tim Rühsen]

Post by Bruno Haible

Post by Tim Rühsen
It works, but... ;-)

OK, I've pushed it.

Post by Tim Rühsen
Your patch disables that warning for the whole file. IMO, we should keep
the scope of the #pragma as narrow as possible.
We could have a pragma.h include file with the following C99 code (I
#define STRINGIFY(a) #a
#if defined __clang__
# define NOWARN(a) _Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
_Pragma( STRINGIFY( clang diagnostic push ) ) \
_Pragma( STRINGIFY( clang diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#elif defined __GNUC__
# define NOWARN(a) _Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_PUSH(a) \
_Pragma( STRINGIFY( gcc diagnostic push ) ) \
_Pragma( STRINGIFY( gcc diagnostic ignored a ) )
# define NOWARN_POP _Pragma( STRINGIFY( gcc diagnostic pop ) )
#else
# define NOWARN
# define NOWARN_PUSH
# define NOWARN_POP
#endif

Indeed such macrology belongs in one single file. Would you like to provide
a patch?

Attached is a quick commit with a new lib/pragmas.h. Sorry, I currently
don't have time for anything more (e.g. a module).

Post by Bruno Haible
The macros should cater for the case the gcc and clang have different warning
options (see e.g. anytostr.c).

Added a pragma to silence clang on unknown options. gcc doesn't complain
anyways. So if we need two different options, just use both.

Post by Bruno Haible
Also if NOWARN and NOWARN_PUSH take an argument in one case, they need to take
an argument in the #else case as well.

Fixed.

Regards, Tim

[Gisle Vanem]

Post by Tim Rühsen
Attached is a quick commit with a new lib/pragmas.h. Sorry, I currently
don't have time for anything more (e.g. a module).

These are nice. MSVC also has the '__pragma()' keyword
documented here:
https://msdn.microsoft.com/en-us/library/d9x1s805.aspx

But that would need a full set of 'NOWARN_xx(a)'
for all warnings needed. E.g.
__pragma(warning(disable: 4101)) for 'unreferenced local variable'

[Tim Rühsen]

Post by Gisle Vanem

Post by Tim Rühsen
Attached is a quick commit with a new lib/pragmas.h. Sorry, I currently
don't have time for anything more (e.g. a module).

These are nice. MSVC also has the '__pragma()' keyword
  https://msdn.microsoft.com/en-us/library/d9x1s805.aspx
But that would need a full set of 'NOWARN_xx(a)'
for all warnings needed. E.g.
 __pragma(warning(disable: 4101)) for 'unreferenced local variable'

We would need MSVC variants of NOWARN and NOWARN_PUSH, e.g.
NOWARN_MSVC(4705 4706) or NOWARN_PUSH_MSVC(4705 4706).

Maybe you could add a tested version ?

Regards, Tim

[Paul Eggert]

# define NOWARN_PUSH(a) \ >>> _Pragma( STRINGIFY( clang

diagnostic push ) ) \ >>> _Pragma( STRINGIFY( clang diagnostic ignored a
) )
I would rather not clutter the code with calls to macros like these, as
such calls detract from ordinary development. It's OK to put this stuff
in a place that we ordinarily don't have to look at, but it's not OK to
add needless complexity to code that people need to read to understand
what the code is doing.

It ought to be easy to disable the warnings entirely, for the entire
build, and that is better than to waste peoples' time inserting these
calls and then later having to read them. If it's easy to do disable
warnings on a per-module basis then I guess that's OK too. But it's not
OK to clutter the actual code. We don't have enough resources to waste
them on pacifying compilers that cry wolf so often.

[Tim Rühsen]

Post by Bruno Haible
Hi Tim,

Post by Tim Rühsen
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".

Post by Tim Rühsen
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.

Post by Bruno Haible
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.

Post by Bruno Haible
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

[Bruno Haible]

Hi Tim,

Post by Tim Rühsen

Post by Bruno Haible
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?

Post by Tim Rühsen

Post by Bruno Haible
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.

Post by Tim Rühsen
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

[Tim Rühsen]

Hi Bruno,

Post by Bruno Haible

Post by Tim Rühsen

Post by Bruno Haible
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?

- people who don't regularly build the code will get upset/alarmed by
those warnings, either thinking the maintainer is ignorant or/and even
trying to find out what's going on (wasting precious time).

- this might even repel possible contributors, especially when they
invested time, created patches and the upstream answer is "haha, sorry,
we don't want to fix these warnings - we got used to them".

- the situation might lead to switching off gnulib warnings completely
in projects that use gnulib. This means one 'line of defense' less, and
less possible contributors.

- when projects use static analysis tools and there is too much wave
from gnulib, the gnulib will simply be excluded from analysis.

If the maintainer of e.g. libz got used to some warnings... well, not so
many people/devs are affected. Most projects just use the binary
library, not seeing the warnings at all.
But the situation is special for gnulib since it is a source code library.

Post by Bruno Haible

Post by Tim Rühsen

Post by Bruno Haible
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.

You normally fuzz with sanitizers (ASAN, UBSAN) switched on. I remember
only one thing that valgrind found but the sanitizers/fuzzing not.

Uninitialized variables *should* be found by the compiler.

Post by Bruno Haible

Post by Tim Rühsen
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.

Not sure how much part automake takes here. The way to use valgrind
heavily depends on the test suite. E.g. in wget we only want to test the
wget utility which gets called by the tests itself. So we check a
certain env var and add valgrind to the system()/popen() command line.
How can automake know about that ?

With Best Regards, Tim