The compiler warnings are one of the most important features of modern optimising compilers. They replaced, for the good part, the use of the lint tool to identify possible errors in source code. Ignoring too many warnings usually mean that the software hasn’t been looked at and properly polished before release. More importantly, warnings can be promoted to errors with time, since often they show obsolete code structure or unstable code. An example of this is the warning about the size mismatch between integers and pointers, which became an error with the GCC 3.4 release, to make sure that code would run smoothly on 64-bit machines (which started to become much more common).
Usually, a new batch of warnings is added with each minor or major GCC release, but for some reason the latest micro release (GCC 4.3.3) enabled a few more of them by default. This is usually a good sign since a stricter compiler usually mean that code improves with it. Unfortunately I’m afraid the GCC developers have been a little too picky this time around.
You might remember how the most important warnings cannot become errors even if support for turning particular warnings in errors is present with recent GCC releases. But before that I also had some other problem with GCC warnings, in particular warnings against system headers .
That latter problem was quite nasty since by default GCC would hide from me cases where my code defined some preprocessor macro that the system headers were also going to define (maybe with different values). For that reason, I started building xine-lib with -Wsystem-headers to make sure that these case could be handled. It wasn’t much of a problem since the system headers were always quite clean after all, without throwing around warnings and similar. But this is no longer the case, with GCC 4.3.3 I started to get a much lower signal-to-noise ratio, since a lot of internal system headers started throwing warnings like “no previous prototype”. This is not good and really shows how glibc and gcc aren’t really well synchronised (similarly, readline and gdb, but that’s a story for another day).
So okay I disabled -Wsystem-headers since it produces too much noise and went looking for the remaining issues. The most common problem in xine-lib that the new gcc shows is that all the calls to asprintf() ignore the return value. This is technically wrong, but it should lead to little problems there since a negative value for asprintf mean that there is not enough memory to allocate the string, and thus most likely the program would like to abort. As it is, I’m not going to give them too much weight but rather remind myself I should branch xine-lib-1.2 and port it to glib .
Unfortunately, while the “warn about unused return value” idea is quite good for many functions, it is not for all of them. And somehow the new compiler also ignores the old workaround to shut the warning up (that would be a cast to void of the value returned by the function); while, again, technically good to have those warnings, sometimes you just don’t care whether a call succeeded or not because either way you’re going to proceed in the same way, thus you don’t spend time checking the value (usually, because you check it somehow later on). What the “warn about unused return value” should really point out is if there are leaks due to allocation functions whose return value (the address of the newly-allocated memory area) is ignored, since there is no way you can just ignore that without having an error.
One quite stupid place where I have seen the new compiler to throw a totally useless warning is related to the nice() system call; this is a piece of code from xine-lib:
#ifndef WIN32
/* nice(-value) will fail silently for normal users.
* however when running as root this may provide smoother
* playback. follow the link for more information:
* [snip]
*/
nice(-1);
#endif /* WIN32 */(don’t get me started with the problems related to this function call, it’s not what I’m concerned with right now).
As you can see there is a comment about failing silently, which is exactly what the code wants; use it if it works, don’t if it doesn’t. Automagic, maybe, but I don’t see a problem with that. But with the new compiler this throws a warning because the return value is not checked. So it’s just a matter to check it and eventually log a warning so to make the compiler happy, no? It would be if it wasn’t for the special case of the nice() return value.
On success, the new nice value is returned (but see NOTES below). On error, –1 is returned, and errno is set appropriately.
[snip]
NOTES
[snip]
Since glibc 2.2.4,
nice()is implemented as a library function that callsgetpriority(2)to obtain the new nice value to be returned to the caller. With this implementation, a successful call can legitimately return –1. To reliably detect an error, seterrnoto 0 before the call, and check its value whennice()returns –1.
So basically the code would have to morph in something like the following:
#ifndef WIN32
/* nice(-value) will fail silently for normal users.
* however when running as root this may provide smoother
* playback. follow the link for more information:
* [snip]
*/
errno = 0;
res = nice(-1);
if ( res == -1 && errno != 0 )
lprintf("nice failed");
#endif /* WIN32 */And just for the sake of argument, the only error that may come out of the nice() function is a permission error. While it certainly isn’t an enormous amount of code needed for the check, it really is superfluous for software that is interested in just making use of it if they have the capability to do so. And it becomes even more critical to not bother with this when you consider that xine is a multithreaded program, and that the errno interface is … well… let’s just say it’s not the nicest way to deal with errors in multithreaded software.
What’s the bottom line of this post? Well, I think that the new warnings added with GCC 4.3.3 aren’t bad per-se, they actually are quite useful, but just dumping a whole load of new warnings on the developers is not going to help, especially if there are so many situations where you would just be adding error messages in the application that might never be read by the user. The amount of warnings raised by a compiler should never be so high that the output is filled with them, otherwise the good warnings will just get ignored causing software to not improve.
I think for xine-lib I’ll check out some of the warnings (I at least found some scary code in one situation) and then I’ll disable locally the warn-unused-return warning, or sed out the asprintf() and nice() return values ignoring. In either case I’m going to have to remove some true positive to avoid having to deal with a load of false positives I shouldn’t be caring about. Not nice.
Update (2017-04-28): I feel very sad to have found out over a year and a half later that Michael died. The links in this and other posts to his blog are now linked to the archive kindly provided and set up by Jan Kučera. Thank you, Jan. And thank you, Michael.
Update (2021-02-17): I've snipped away the broken URL to Miguel Freitas's page that is now not found. You can find an archived copy — courtesy of the Wayback Machine.
Why is errno a problem for multithreaded programs? Each thread has its own errno variable.
When TLS is supported, when it is not…
If errno is used properly, it’s thread-safe. “Properly” means that <errno.h> is included rather than errno being explicitly declared; this allows the system to refer to a per-thread errno (in <bits errno.h=””> system header)
You’re assuming glibc, I’m not ;)On modern glibc systems TLS (Thread-local Storage) is used so that each thread has its own errno variable. On most other modern system with TLS the same holds true, but the same does not old true with older systems or with systems where TLS is not supported.
Dude, errno is perfectly thread safe since POSIX 1c, i.e. realistically everywhere.The correct way to make the warning for nice() go away goes like this:(void) nice(-1);(i.e. cast the return value of the call explcitly to void and you are happy)
Hah! You wish!With _FORTIFY_SOURCE=2 and whatever other changes there are in GCC 4.3.3 (or the Gentoo version of it, I’m still not sure how much is upstream and how much is just ours), the void cast trick does not work any longer.Which is exactly why it’s driving me mad.
don’t blame upstream, they have nothing to do with it. we started forcing -Wformat-security and -DFORTIFY_SOURCE in 4.3.2-r2 on purpose.http://archives.gentoo.org/…