Re: [PATCH] <semaphore.h>: Add nonnull attributes

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 18/03/2019 09:37, Yann Droneaud wrote:
> Except the following functions, NPTL implementation assume sem_t
> argument (or other arguments) are not NULL, so they would benefit
> from having the nonnull attribute.
> 
> - sem_close(): can cope with a NULL sem_t and return -1 with error EINVAL;
> - sem_destroy(): does nothing at all
> 
> 	* sysdeps/pthread/semaphore.h (sem_init): Add __nonnull attribute.
> 	(sem_destroy, sem_open, sem_close, sem_unlink): Likewise.
> 	(sem_wait, sem_timedwait, sem_trywait, sem_post): Likewise.
> 	(sem_getvalue): Likewise.

LGTM, although reading some gcc bug reports I am not sure how effective 
this attribute is in realword cases. For instance, gcc BZ#17308 [1] indicates
that gcc stills missing the warning for deduced values (and the bug was
closed with the premise of gcc fixing at least for some builtins like
print).

However it seems that for arbitrary function definition the attribute
still does not help much:

---
$ cat test.c
#include <sys/cdefs.h>

/* Arbitrary type just as an example.  */
typedef union
{
  char __size[16];
  long int __align;
} sem_t;

extern sem_t *sem_open (const char *__name, int __oflag, ...)
  __THROW __nonnull ((1));

sem_t * bar (void)
{
  char *name = 0;
  return sem_open (name, 0);
}
$ gcc -v
[...]
gcc version 9.0.1 20190315 (experimental) (GCC) 
$ gcc -Wall -Wnonnull test.c -c
$
---

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308

> ---
>  sysdeps/pthread/semaphore.h | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/sysdeps/pthread/semaphore.h b/sysdeps/pthread/semaphore.h
> index 41ff92765e32..87c054392c58 100644
> --- a/sysdeps/pthread/semaphore.h
> +++ b/sysdeps/pthread/semaphore.h
> @@ -33,24 +33,26 @@ __BEGIN_DECLS
>  /* Initialize semaphore object SEM to VALUE.  If PSHARED then share it
>     with other processes.  */
>  extern int sem_init (sem_t *__sem, int __pshared, unsigned int __value)
> -     __THROW;
> +  __THROW __nonnull ((1));
> +
>  /* Free resources associated with semaphore object SEM.  */
> -extern int sem_destroy (sem_t *__sem) __THROW;
> +extern int sem_destroy (sem_t *__sem) __THROW __nonnull ((1));
>  
>  /* Open a named semaphore NAME with open flags OFLAG.  */
> -extern sem_t *sem_open (const char *__name, int __oflag, ...) __THROW;
> +extern sem_t *sem_open (const char *__name, int __oflag, ...)
> +  __THROW __nonnull ((1));
>  
>  /* Close descriptor for named semaphore SEM.  */
> -extern int sem_close (sem_t *__sem) __THROW;
> +extern int sem_close (sem_t *__sem) __THROW __nonnull ((1));
>  
>  /* Remove named semaphore NAME.  */
> -extern int sem_unlink (const char *__name) __THROW;
> +extern int sem_unlink (const char *__name) __THROW __nonnull ((1));
>  
>  /* Wait for SEM being posted.
>  
>     This function is a cancellation point and therefore not marked with
>     __THROW.  */
> -extern int sem_wait (sem_t *__sem);
> +extern int sem_wait (sem_t *__sem) __nonnull ((1));
>  
>  #ifdef __USE_XOPEN2K
>  /* Similar to `sem_wait' but wait only until ABSTIME.
> @@ -58,18 +60,19 @@ extern int sem_wait (sem_t *__sem);
>     This function is a cancellation point and therefore not marked with
>     __THROW.  */
>  extern int sem_timedwait (sem_t *__restrict __sem,
> -			  const struct timespec *__restrict __abstime);
> +			  const struct timespec *__restrict __abstime)
> +  __nonnull ((1, 2));
>  #endif
>  
>  /* Test whether SEM is posted.  */
> -extern int sem_trywait (sem_t *__sem) __THROWNL;
> +extern int sem_trywait (sem_t *__sem) __THROWNL __nonnull ((1));
>  
>  /* Post SEM.  */
> -extern int sem_post (sem_t *__sem) __THROWNL;
> +extern int sem_post (sem_t *__sem) __THROWNL __nonnull ((1));
>  
>  /* Get current value of SEM and store it in *SVAL.  */
>  extern int sem_getvalue (sem_t *__restrict __sem, int *__restrict __sval)
> -     __THROW;
> +  __THROW __nonnull ((1, 2));
>  
>  
>  __END_DECLS
>