<semaphore.h>: Add nonnull attributes

  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.
---
 sysdeps/pthread/semaphore.h | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
  

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
>
  

Hi,

Le mardi 19 mars 2019 à 19:40 -0300, Adhemerval Zanella a écrit :
> 
> 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
> $
> 

Thanks for the review !

For the -Wnonnull warning to be effective you have to enable some
optimization. I don't know the exact one. At least with the ones
enabled by -O1:

$ gcc -Wnonnull -O test.c -c
test.c: In function 'bar':
test.c:16:10: warning: argument 1 null where non-null expected [-Wnonnull]
   16 |   return sem_open (name, 0);
      |          ^~~~~~~~~~~~~~~~~~
test.c:10:15: note: in a call to function 'sem_open' declared here
   10 | extern sem_t *sem_open (const char *__name, int __oflag, ...)
      |               ^~~~~~~~

$ gcc -v
[...]
gcc version 9.0.1 20190312 (Red Hat 9.0.1-0.10) (GCC) 

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

Regards.
  

* Yann Droneaud:

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

Yann, do you need help with isntalling this patch?

Thanks,
Florian
  

Le lundi 29 avril 2019 à 08:42 +0200, Florian Weimer a écrit :
> * Yann Droneaud:
> 
> > 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.
> 
> Yann, do you need help with isntalling this patch?
> 

Yes please: I have no account on any GNU server.

Also note I have not signed any copyright assignment. But I'm willing
to, for me and my own company.

Regards
  

* Yann Droneaud:

> Le lundi 29 avril 2019 à 08:42 +0200, Florian Weimer a écrit :
>> * Yann Droneaud:
>> 
>> > 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.
>> 
>> Yann, do you need help with isntalling this patch?
>> 
>
> Yes please: I have no account on any GNU server.

Okay, I will install this after testing.

> Also note I have not signed any copyright assignment. But I'm willing
> to, for me and my own company.

It seems that we have one previous contribution from you, in commit
a092b645f8 ("Fix DEBUG statements in resolv/res_send.c").  But it has
been removed by commit 09fbb56ad6 ("resolv: Remove DEBUG from
resolv/res_send.c"), so I think we can accept your new contribution
as-is, since it is below the 15 line limit in the FSF guidelines.

Nevertheless, please get the process going.  You should send this form

<http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future>

to <assign@gnu.org>.

Thanks,
Florian