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?

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.

I see, but you write

Turning off -Wnullability-completeness is not an option.

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

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.

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

Of course, very good point. Thank you.

Add kib per the pthread changes.

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.

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!

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.

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.

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?

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.

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.