clang 3.3 suggests initializing pthread_t to NULL

[Quuxplusone]

Bugzilla Link	PR20827
Status	NEW
Importance	P normal
Reported by	Keith Bostic (keith@bostic.com)
Reported on	2014-09-01 14:23:28 -0700
Last modified on	2014-09-03 19:55:39 -0700
Version	3.3
Hardware	PC FreeBSD
CC	david.majnemer@gmail.com, keith@bostic.com, llvm-bugs@lists.llvm.org, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

wtperf.c:1867:28: warning: variable 'monitor_thread' may be uninitialized when
      used here [-Wconditional-uninitialized]
            (t_ret = pthread_join(monitor_thread, NULL)) != 0) {
                                  ^~~~~~~~~~~~~~
wtperf.c:1744:26: note: initialize the variable 'monitor_thread' to silence this
      warning
        pthread_t monitor_thread;
                                ^
                                 = NULL

AFAIK, it's never legal to initialize a pthread_t value to anything at all.

[Quuxplusone]

We shouldn't even be warning here, it looks like a total false positive:
https://github.com/wiredtiger/wiredtiger/blob/master/bench/wtperf/wtperf.c

pthread_t used to be required to be an arithmetic type before IEEE Std 1003.1-
2001/Cor 2-2004 had item XBD/TC2/D6/26 applied; this item permits structure
types to be used as the underlying type for pthread_t.

I see no text in the specification that says it's not OK to fill a pthread_t
with whatever byte pattern one would like so long as it was not used with
pthread_{join,detach,equal,cancel, etc.}.

Seeing as how the standard permits various underlying types for pthread_t, I
believe the following is benign:

pthread_t monitor_thread;
memset(&monitor_thread, 0, sizeof(monitor_thread));

[Quuxplusone]

Yes; I agree it is also a false positive (and we get other false positives in the same situation, that is, a guard variable associated with a pthread_t).

It's a little odd: we use guard variables around locks in similar situations, they don't trigger a false positive, only the pthread_t's.

I can reduce the case to where the clear/set/test of the guard variable are within about 5 lines of code and it still triggers.

I'm happy to put together a standalone test case for this if it's useful, of course.

[Quuxplusone]

Yes, it's a false positive, but that's expected with -Wconditional-
uninitialized. Use -Wuninitialized instead if you don't want any false
positives (but -Wuninitialized has more false negatives). The raison d'être of -
Wconditional-uninitialized is to trade false positives for fewer false
negatives.

It doesn't seem reasonable to expect Clang to special-case pthread_t and avoid
suggesting how to initialize it (it's just too special a special case). Maybe
we should only suggest the '= NULL' correction if the user actually wrote the
'*' in the variable declaration, and not if it comes from a typedef?

[Quuxplusone]

There is a *lot* of C code which typedefs pointer types.

Off of the top of my head, XNU's vnode_t type:
typedef struct vnode * vnode_t;

[Quuxplusone]

(In reply to comment #4)
> There is a *lot* of C code which typedefs pointer types.

Sure. The question is, how often is the fact that the type is a pointer part of
the interface, and how often is it an implementation detail? How big is the
gain from a correct suggestion to use NULL, and how big is the loss from an
incorrect suggestion?

My thoughts: there are some cases where the user of a typedef knows it's a
pointer and other cases where they don't or shouldn't know that (I don't want
to hazard a guess to numbers, but I'd expect double-digits percent on each
side). The badness of a bad guess is much worse than the goodness of a good
guess. So we shouldn't be guessing unless the pointer type is written "nearby".