Page MenuHomeFreeBSD

Addition of clang nullability attributes.
ClosedPublic

Authored by pfg on Dec 30 2016, 9:55 PM.

Details

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.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pfg updated this revision to Diff 23477.Dec 30 2016, 9:55 PM
pfg retitled this revision from to Addition of clang nullability attributes..
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)
pfg added reviewers: theraven, dim, ed, emaste, jilles.
pfg updated this revision to Diff 23483.Dec 31 2016, 3:22 AM

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 updated this revision to Diff 23484.Dec 31 2016, 3:31 AM

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

pfg updated this revision to Diff 23505.EditedJan 1 2017, 4:15 AM

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 abandoned this revision.Jan 1 2017, 4:24 AM

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 updated this revision to Diff 23519.Jan 1 2017, 8:19 PM

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 updated this revision to Diff 23558.Jan 2 2017, 7:30 PM

Update with small cleanups that were breaking world.

pfg updated this revision to Diff 23575.Jan 3 2017, 3:52 PM

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

This passes a tinderbox build.

emaste edited edge metadata.Jan 3 2017, 3:55 PM

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 added a comment.Jan 3 2017, 4:15 PM

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 added a comment.Jan 3 2017, 4:57 PM

I see, but you write

Turning off -Wnullability-completeness is not an option.

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

pfg added a comment.Jan 3 2017, 6:07 PM

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 added a comment.Jan 3 2017, 6:23 PM

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

Of course, very good point. Thank you.

pfg updated this revision to Diff 23792.Jan 9 2017, 8:05 PM
pfg edited edge metadata.

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

pfg added a reviewer: kib.Jan 9 2017, 8:10 PM

Add kib per the pthread changes.

kib edited edge metadata.Jan 10 2017, 11:25 AM

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 added inline comments.Jan 10 2017, 11:27 AM
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 added a comment.Jan 10 2017, 3:29 PM
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 added inline comments.Jan 10 2017, 3:41 PM
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 updated this revision to Diff 23832.Jan 10 2017, 3:59 PM
pfg edited edge metadata.

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 added a comment.Jan 10 2017, 4:39 PM
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 added a comment.Jan 10 2017, 6:23 PM
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 updated this revision to Diff 23839.Jan 10 2017, 6:31 PM
pfg marked 3 inline comments as done.

Update copyinstr().

pfg marked 2 inline comments as done.Jan 10 2017, 6:32 PM
pfg updated this revision to Diff 23856.EditedJan 11 2017, 12:49 AM

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 updated this revision to Diff 23965.Jan 13 2017, 3:33 PM

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 edited edge metadata.Jan 13 2017, 9:19 PM

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 added a comment.Jan 13 2017, 9:33 PM
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 added a comment.EditedJan 14 2017, 2:55 AM
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 updated this revision to Diff 24057.Jan 16 2017, 3:20 AM
pfg edited edge metadata.

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 updated this revision to Diff 24159.Jan 18 2017, 3:41 PM

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.

This revision was automatically updated to reflect the committed changes.