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?

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!

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?

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?

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... :)

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.

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.