Addition of clang nullability attributes.

[pfg]

Summary

Add two new attributes for use in static checkers:

_Nonnull
The _Nonnull nullability qualifier indicates that null is not a meaningful
value for a value of the _Nonnull pointer type.

_Nullable
The _Nullable nullability qualifier indicates that a value of the
_Nullable pointer type can be null.

These seem to have been introduced in Clang 3.7:
http://releases.llvm.org/3.7.0/tools/clang/docs/AttributeReference.html

Test Plan

Currently building world with them.

[pfg]

Learn a bit more from Bionic's libc.

The cdefs.h definition in Android is a bit different, since it's known
to work and the license is consistent with our own headers, I will just
adopt it. Android also replaced all the __nonnull() attributes,
particularly in pthread.h, with _Nonnull.

Most of our uses of the __nonnull attribute were based on Bionic so
adopting this change is consistent.

The difference is:

"Unlike the nonnull attribute, this annotation indicated that a value
*should not* be null, not that it *cannot* be null, or even that the
behavior is undefined. The important distinction is that the optimizer will
perform surprising optimizations."
For an example, see here:
https://android.googlesource.com/platform/bionic/+/master/libc/include/sys/cdefs.h#120

More information at:
http://clang.llvm.org/docs/AttributeReference.html#nonnull

[pfg]

Minor updates.
While here I should mention, I did strip a bit the indentation in
pthread.h to make it more readable .

[pfg]

Ugh!

Use of these attributes breaks the build in ugly ways. For example:

in file included from 
/scratch/tmp/pfg/head/contrib/llvm/projects/libunwind/src/libunwind.cpp:27:
In file included from 
/scratch/tmp/pfg/head/contrib/llvm/projects/libunwind/src/UnwindCursor.hpp:19:
/scratch/tmp/pfg/obj/i386.i386/scratch/tmp/pfg/head/tmp/usr/include/pthread.h:149:26: 
error: pointer is missing a nullability type specifier (_Nonnull, 
_Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
int     pthread_atfork(void (*)(void), void (*)(void), void (*)(void));
                             ^
/scratch/tmp/pfg/obj/i386.i386/scratch/tmp/pfg/head/tmp/usr/include/pthread.h:151:33: 
error: pointer is missing a nullability type specifier (_Nonnull, 
_Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
int     pthread_attr_getstack(const pthread_attr_t * _Nonnull __restrict, 
                                    ^
/scratch/tmp/pfg/obj/i386.i386/scratch/tmp/pfg/head/tmp/usr/include/pthread.h:152:8: 
error: pointer is missing a nullability type specifier (_Nonnull, 
_Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
                void ** _Nonnull __restrict, size_t * _Nonnull 
__restrict);
                     ^
/scratch/tmp/pfg/obj/i386.i386/scratch/tmp/pfg/head/tmp/usr/include/pthread.h:153:37: 
error: pointer is missing a nullability type specifier (_Nonnull, 
_Nullable, or _Null_unspecified) [-Werror,-Wnullability-completeness]
...

More investigation will be required :(.

[pfg]

The '__nonnull()' attribute is rather dangerous, it seems reasonable to replace it with the '_Nonnull' qualifier. Unfortunately it seems like a lot more changes than those from either bionic or Apple are required to build cleanly. Turning off -Wnullability-completeness is not an option.

I will abandon this revision and will be back when/if I get something working :(.

[pfg]

I figured it out:

We need to add pragmas to the headers where we use nullablity specifiers,
otherwise the compiler will ask for *all* pointers to be annotated.

While this approach is the same used in Android, Apple appears to build
everything with -Wno-nullability-completeness.

[pfg]

Update with small cleanups that were breaking world.

[pfg]

More cleanups: the pragmas have to appear before *any* pointer is
declared.

This passes a tinderbox build.

[emaste]

I suspect this is fine (haven't yet looked in detail). Is turning off -Wnullability-completeness a nonstarter because we have some headers that are already completely annotated and want to prevent new, unannotated declarations from appearing?

[pfg]

In D9004#186583, @emaste wrote:

I suspect this is fine (haven't yet looked in detail). Is turning off -Wnullability-completeness a nonstarter because we have some headers that are already completely annotated and want to prevent new, unannotated declarations from appearing?

Sort of: we can'r really annotate everything without adding a lot of annotations. As soon as the compiler sees *one* annotation, it complains about all other pointers that haven't been annotated, therefore we need to turn off the warnings.

For reference, the patch uses this inspiration:
sys/systm.h: replace preexiting nonnull attribute (with some hinting fron Android)
stdio.h, stdlib.h: MacOX
pthread.h err.h, signal., syslog.h: Android (we had the pthread nonnull checks there)

I haven't yet added nonnull checks in the stdlib string functions that are in Glibc and that Android carries. the new _Nonnull qualifier is safe enough that we should probably consider those as well.

[emaste]

I see, but you write

Turning off -Wnullability-completeness is not an option.

I'm curious why it's not an option.

[pfg]

In D9004#186600, @emaste wrote:

I see, but you write

Turning off -Wnullability-completeness is not an option.

I'm curious why it's not an option.

Adding it as a global option would certain solve any buildworld issues cleanly.
The problem, I think, would be for third-party software, like ports, using system headers.

[emaste]

The problem, I think, would be for third-party software, like ports, using system headers.

Of course, very good point. Thank you.

[pfg]

Update some of the threading functions.
Move the pragmas consistently to the top of the header.
This is a commit candidate.

[pfg]

Add kib per the pthread changes.

[kib]

Why is this useful at all ? IMO it adds enormous and useless noise to the headers, while providing a doubtful value, if any.

If only referencing pthread*(3) functions, POSIX recommends that implementations detect invalid or uninitialized pointer to pthread_t and return EINVAL in that case. Our libthr usually checks for the pthread pointer == NULL and returns EINVAL in this case. More, apps might reasonably rely on this behavior and knowingly use it.

sys/sys/syslog.h

37 ↗

(On Diff #23792)

If there is the push, there must be a pop somewhere. I believe you contaminate the user environment right now.

[kib]

sys/sys/systm.h

261 ↗

(On Diff #23792)

Why do you disable user address 0 ?

404 ↗

(On Diff #23792)

This is complete nonsense, wait channel is just a token.

[pfg]

In D9004#188426, @kib wrote:

Why is this useful at all ? IMO it adds enormous and useless noise to the headers, while providing a doubtful value, if any.

Note that I am only replacing the very dangerous GCC __nonnull__ attribute with the much more benign clang _Nonnull. Most cases were pre-existing, except for some functions where we are followinh Apple Darwin, in the hope of being more firendly to swift (and clang in the Mac)

If only referencing pthread*(3) functions, POSIX recommends that implementations detect invalid or uninitialized pointer to pthread_t and return EINVAL in that case. Our libthr usually checks for the pthread pointer == NULL and returns EINVAL in this case. More, apps might reasonably rely on this behavior and knowingly use it.

We introduced those after review in D2101 following Android. The idea is that it is very easy to get threading wrong so the extra warnings are useful. The previously-existing attributes actually "optimized" away the NULL checks. Android since then have acknowledged that such attributes are wrong and replaced them with the corresponding clang qualifier (which doesn't do stupid things).

I will fix the issues you found and update the revision .. Thanks!

sys/sys/syslog.h

37 ↗

(On Diff #23792)

Absolutely right! I had meant to check this but for some reason the Android headers don't have it I will add them.

sys/sys/systm.h

261 ↗

(On Diff #23792)

This was my error, thanks.

404 ↗

(On Diff #23792)

This was just copied form the pre-existing _nonnull() invocation.
I will remove it.

[pfg]

sys/sys/systm.h

261 ↗

(On Diff #23792)

Actually ... it was not my error: __nonnull(1) __nonnull(2); translate into _Nonnull for the first and second arguments.

[pfg]

Update to bring the pop pragmas.

I left the copyinstr(9) as it is for consistency with the previous
declaration. FWIW, according to the manpage the first argument should be a
"NUL-terminated string", not sure conceptually a NULL pointer would
classify.

[kib]

In D9004#188458, @pfg wrote:

Update to bring the pop pragmas.

I left the copyinstr(9) as it is for consistency with the previous
declaration. FWIW, according to the manpage the first argument should be a
"NUL-terminated string", not sure conceptually a NULL pointer would
classify.

0 is absolutely (potentially) valid pointer to a userspace string. More, in past there were ABIs where there was a zero-length string located at address zero.

[pfg]

In D9004#188501, @kib wrote:

In D9004#188458, @pfg wrote:

Update to bring the pop pragmas.

I left the copyinstr(9) as it is for consistency with the previous
declaration. FWIW, according to the manpage the first argument should be a
"NUL-terminated string", not sure conceptually a NULL pointer would
classify.

0 is absolutely (potentially) valid pointer to a userspace string. More, in past there were ABIs where there was a zero-length string located at address zero.

OK, so these are wrong since r117837, although I see it wouldn't make much sense to call this with an explicit NULL. I will remove it and I guess I should do some partial MFC to avoid completely the GCC __nonnull__ attribute.

Furthermore I think __nonnull() should be removed from cdefs.h, at least from current.

[pfg]

Update copyinstr().

[pfg]

Looking at deprecating the __nonnull() attribute completely, I found a
couple of occurrences in lib/libthr/thread/thr_private.h.

There are also a couple of uses in
sys/x86/x86/mca.c
which I am addressing independently.

[pfg]

Add check for nullability feature before using the pragmas.
We still have many older versions of clang in the ports tree.

I am currently touching many critical headers so I will request an
exp-run before committing this.

[ed]

I like the idea, but I think it would make more sense to do this a bit more slowly. For example, we could use this change to add stubs for these keywords for old compilers to sys/cdefs.h. Then we could decide to add these annotations header by header and not add those #pragma directives at all. Simply annotate a header file entirely or don't do it at all.

Thoughts?

[pfg]

In D9004#189480, @ed wrote:

I like the idea, but I think it would make more sense to do this a bit more slowly. For example, we could use this change to add stubs for these keywords for old compilers to sys/cdefs.h.

The stubs are in place already since r310977, this had to be done so that the GCC ports have ample time to catch up.. MFC's are a problem due to the way GCC forks our C headers. My plan was to remove the __nonnull() in a different commit.

Then we could decide to add these annotations header by header and not add those #pragma directives at all. Simply annotate a header file entirely or don't do it at all.

Thoughts?

Annotating all the headers would make them pretty much unreadable: we would have to add _Null_unspecified to almost every pointer.

I would feel happier if there were a nice macro to deal with the pragmas: something like we do for __BEGIN_DECLS and __END_DECLS.

[pfg]

In D9004#189482, @pfg wrote:

I would feel happier if there were a nice macro to deal with the pragmas: something like we do for __BEGIN_DECLS and __END_DECLS.

Something like this:

/*
 * Nullability qualifiers: currently only supported by Clang.
 */
#if !(defined(__clang__) && __has_feature(nullability))
#define	_Nonnull
#define	_Nullable
#define __BEGIN_NULLABILITY
#define __END_NULLABILITY
#else
#define __BEGIN_NULLABILITY _Pragma("clang diagnostic push")	\
	_Pragma("clang diagnostic ignored \"-Wnullability-completeness\"")
#define	__END_NULLABILITY _Pragma("clang diagnostic pop")
#endif

However _Pragma was introduced in C99, so bde@ would probably object.

[pfg]

Trying the _Pragma suddenly doesn't look like a bad idea after all: if GCC adopts
the attributes it would be easier to adapt them in just one place instead of chasing
all the headers.

Any comments?

[pfg]

I was not very happy about the old naming for the macros so I have new
names which I think are more descriptive:

NULLABILITY_PRAGMA_PUSH
NULLABILITY_PRAGMA_POP

At least now we know what the macros do.