RFC: Enable nonnull and warn_unused_result attributes by default

Clang Frontend
1

Hi,

GCC has "nonnull" and "warn_unused_result" attributes that are really great for finding bugs, assuming they're used everywhere. The problem is, they would have to be used for >99% of the functions, which makes them way too much trouble to be worth it for normal code. So wouldn't it be better to just make that the default behavior, and add "null" and "nowarn_unused_result" attributes for the few exceptions where the opposite behavior is needed?

I've implemented both of those myself, although only the "null" attribute code is made public in http://llvm.org/bugs/show_bug.cgi?id=6786 I'm now wondering (hoping!) if there's a way to make these changes part of the standard clang. I know the current patch isn't good enough, but before trying to improving it, I'd like to know:

* How realistic is it to get this feature included at all? I guess I could just keep patching clang myself, but it's rather annoying. I think this feature would be very useful for anyone who wants to write high quality C code, since it forces dealing with return values and specifying if NULL pointers are valid as parameters. I found maybe 20 bugs (mainly missing error handling) from my own code with these patches.

* What would be the correct parameter names to enable these? -fattribute-nonnull-default & -fattribute-warn-unused-result-default or -Wattribute-nonnull-default & -Wattribute-warn-unused-result-default or ..?

* Getting these warnings from libraries you can't modify (libc, etc.) is rather pointless. So the patch I wrote enables these only for functions found from non-system headers. Is this too much of a special case? Should there be separate settings to enable these also for system headers? Although libc already adds the nonnull attributes for almost everything, so it could also add the null attributes for the rest of them to make both behaviors work..

* For the "null" attribute we could follow either the current "nonnull" attribute behavior by giving a list of parameter numbers which can be NULL (which is rather ugly) or the "unused" attribute behavior where you simply mark the parameter with null attribute. Since GCC doesn't have this new "null" attribute and with the default change there's really no need to use "nonnull" at all, I think perhaps it would be better to use the unused-like behavior?

2

Maybe you're very disciplined about this, but legally null parameters are pervasive in many C code bases. There is no way we could create such a warning and enable it by default; our users would hate us.

Functions with results that are typically ignored are less common but still quite idiomatic. This warning would be possible, but it'd take a lot of tweaking.

John.

3

I didn't mean that clang would enable these by default for all users. I meant adding a new clang parameter that can be used to optionally enable them for all functions in that code base. That wouldn't cause any trouble for existing users, but would be very helpful for me and maybe for others.

I've already implemented these to both clang and to my 300k line code base (http://hg.dovecot.org/dovecot-2.2/). I get zero warnings with them enabled, so it's definitely something that can be very useful for code bases that are disciplined enough.

4

It's a bad idea. Many lint implementations have allows this since pretty
much forever, it tends to create way more noise than it helps. It is
also very easy to create bogus warn_unused_result attributions. Modern
glibc is a prime example. Pretty much all of stdio allows two different
kinds of error checking -- doing it per function call or once at the end
with ferror. Guess what is broken with glibc...

Joerg

5

The problem here is that *very* few users care to be this disciplined. Among those who care, for many users it isn't even possible: most applications include a number of third-party headers that would certainly not have the appropriate annotations, and which can't realistically be updated to have all of these annotations.

It's clear that this will never be a "default-on" warning, and because the potential user base is not small, I also don't think that it should go into the main Clang tree at all.

  - Doug