Introduce the gcc/clang nonnull attribute in the signal and pthread headers.

[pfg]

Summary

The `nonnull' attribute specifies that some function parameters
should be non-null pointers.
This is very useful as it helps the compiler generate warning on
suspicious code and can also enable some optimizations.
In clang this is also useful for the static analyzer.

The specific implementation was hinted by Android's bionic
libc. There are some function specific to FreeBSD that
miss similar treatment (contributions welcome).

Reference:
http://rachid.koucha.free.fr/tech_corner/nonnull_gcc_attribute.html

[theraven]

These look fine, but it would be nice to have some way of telling whether we've got all of them. For example, most of the sig* functions don't appear to define in their man pages what a null sigset_t means, so why do only some of them get the attribute?

[ed]

In D2101#4, @theraven wrote:

These look fine, but it would be nice to have some way of telling whether we've got all of them. For example, most of the sig* functions don't appear to define in their man pages what a null sigset_t means, so why do only some of them get the attribute?

Exactly. I would prefer it if we made these changes to headers at a time. That said, the changes look good so far. Thanks!

[dim]

LGTM, apart from the remarks already made by David and Ed. I'm curious whether this type of change will result in new warnings for e.g. buildworld or a ports exp-run. Did you try any ports builds with it?

[imp]

So what happens, despite the best flow analysis, if I pass a NULL pointer to these functions? Today we get nice, predictable core dumps. Will we somehow have something else if the optimizations this allows are enabled? If so, what?

[dim]

In D2101#7, @imp wrote:

So what happens, despite the best flow analysis, if I pass a NULL pointer to these functions? Today we get nice, predictable core dumps. Will we somehow have something else if the optimizations this allows are enabled? If so, what?

This attribute has some effects:

1. A warning whenever the compiler can determine at build time that you are calling the function with a NULL pointer anyway.
2. This obviously does not extend to runtime.
3. The compiler can further optimize the bodies of the functions, under the assumption the nonnull parameters will never be NULL.
4. Code that checks those parameters against NULL anyway is likely to be optimized away.
5. Code in the function body that accesses the parameters can segfault as usual when the parameters are NULL at runtime.

I think it is fair to say that 1) is the only real benefit of this attribute, e.g. having the compiler alert you whenever you call the function with a NULL parameter. There usually isn't much to gain from optimization in the function body itself.

E.g., if you write:

void my_memcpy(void *dest, const void *src, size_t len)
{
    if (dest == NULL || src == NULL)
        barf();
    size_t i;
    for (i = 0; i < len; ++i)
        *dest++ = *src++;
}

the effect of adding nonnull is that the part calling barf() can be optimized away. Now when you pass NULL to either or the parameters at runtime, by tricking the compiler or ignoring the warning, the loop that dereferences the pointer(s) will cause a segfault.

But you would probably not write the whole parameter check anyway, if you go through the trouble of adding nonnull to such a function... :)

[imp]

In D2101#8, @dim wrote:

But you would probably not write the whole parameter check anyway, if you go through the trouble of adding nonnull to such a function... :)

Perfect. That tells me what I need to know to review these changes.... More later.

[pfg]

In D2101#5, @ed wrote:

In D2101#4, @theraven wrote:

These look fine, but it would be nice to have some way of telling whether we've got all of them. For example, most of the sig* functions don't appear to define in their man pages what a null sigset_t means, so why do only some of them get the attribute?

Exactly. I would prefer it if we made these changes to headers at a time. That said, the changes look good so far. Thanks!

I don't really understand the concerns very well but I'll try to address them the best I can:

1. Concerning documentation: while it would make sense to document these, they are not standard issues and will not trigger an error (unless -Werror is specificied) and they will help a static analyzer (I think Coverity included) sniff suspicious code.
2. I don't have much experience with these functions and setting some of these to NULL may make sense so I started mimicking the bionic headers, which is actually good as it means we will detect the same issues as Android does. On the other hand, Bionic is a stripped down version of the BSD libc so some extra guidance on which them require such treatment would be appreciated.
3. Concerning new warnings: buildworld completed so I am not worried about base but I left running a tinderbox JIC. I don't think anyone keeps a warning count on the ports tree and those are not really under our control. I suspect that glibc does way more attribute setting than our libc.

[pfg]

I reviewed the bionic C library headers and caught some pthread functions
that were missing from the original revision.
Also note:

1. Initially Android also had the first set of sig* functions but

they were never carry attributes. Perhaps it happens that people
call those with NULL values but it doesn't cause harm. I'd prefer
we just keep the behaviour consistent with them.

2. Android doesn't have pthread_barrier_* pthread_spin_* .

There are a few pthread_*_np functions in FreeBSD. These
are all candidates for tagging by someone that knows better ;).

[pfg]

This has passed my build tests.
While it can still be enhanced, it should be valuable as-is to hint warnings on new code.
I will commit it but It will not be MFC'd since it is tangled partially with the thread safety annotations.

[pfg]

Closed by commit rS280458 (authored by @pfg).