nonnull on epoll_wait(2) syscall wrappers?

nonnull on epoll_wait(2) syscall wrappers?

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2138 bytes --]

Hi Adhemerval,

I was checking the epoll_pwait2(2) man page to see if it needs some
updating for the wrapper you added recently.  It seems all's good.

<https://sourceware.org/bugzilla/show_bug.cgi?id=27359>

However, I noticed that there's a difference between the current
manual page and the glibc wrappers for the 3 related functions:


$ grepc -x /sys/epoll.h$ epoll_wait /usr/include/
/usr/include/x86_64-linux-gnu/sys/epoll.h:124:
extern int epoll_wait (int __epfd, struct epoll_event *__events,
		       int __maxevents, int __timeout)
	__attr_access ((__write_only__, 2, 3));
$ grepc -x /sys/epoll.h$ epoll_pwait /usr/include/
/usr/include/x86_64-linux-gnu/sys/epoll.h:134:
extern int epoll_pwait (int __epfd, struct epoll_event *__events,
			int __maxevents, int __timeout,
			const __sigset_t *__ss)
	__attr_access ((__write_only__, 2, 3));
$ grepc -x /sys/epoll.h$ epoll_pwait2 /usr/include/
/usr/include/x86_64-linux-gnu/sys/epoll.h:144:
extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
			 int __maxevents, const struct timespec *__timeout,
			 const __sigset_t *__ss)
	__attr_access ((__write_only__, 2, 3));


/usr/include/x86_64-linux-gnu/sys/epoll.h:157:
#  define epoll_pwait2 __epoll_pwait2_time64


The man page synopsis is:


SYNOPSIS
       #include <sys/epoll.h>

       int epoll_wait(int epfd, struct epoll_event *events,
                      int maxevents, int timeout);
       int epoll_pwait(int epfd, struct epoll_event *events,
                      int maxevents, int timeout,
                      const sigset_t *_Nullable sigmask);
       int epoll_pwait2(int epfd, struct epoll_event *events,
                      int maxevents, const struct timespec *_Nullable timeout,
                      const sigset_t *_Nullable sigmask);


I didn't use _Nullable in the events parameter, because I don't think
it can be NULL.  Does it make any sense in any case having a NULL
there?  Should we use nonnull in glibc?

Cheers,
Alex

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: nonnull on epoll_wait(2) syscall wrappers?

[Adhemerval Zanella Netto]

On 19/05/23 19:50, Alejandro Colomar wrote:
> Hi Adhemerval,
> 
> I was checking the epoll_pwait2(2) man page to see if it needs some
> updating for the wrapper you added recently.  It seems all's good.
> 
> <https://sourceware.org/bugzilla/show_bug.cgi?id=27359>
> 
> However, I noticed that there's a difference between the current
> manual page and the glibc wrappers for the 3 related functions:
> 
> 
> $ grepc -x /sys/epoll.h$ epoll_wait /usr/include/
> /usr/include/x86_64-linux-gnu/sys/epoll.h:124:
> extern int epoll_wait (int __epfd, struct epoll_event *__events,
> 		       int __maxevents, int __timeout)
> 	__attr_access ((__write_only__, 2, 3));
> $ grepc -x /sys/epoll.h$ epoll_pwait /usr/include/
> /usr/include/x86_64-linux-gnu/sys/epoll.h:134:
> extern int epoll_pwait (int __epfd, struct epoll_event *__events,
> 			int __maxevents, int __timeout,
> 			const __sigset_t *__ss)
> 	__attr_access ((__write_only__, 2, 3));
> $ grepc -x /sys/epoll.h$ epoll_pwait2 /usr/include/
> /usr/include/x86_64-linux-gnu/sys/epoll.h:144:
> extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
> 			 int __maxevents, const struct timespec *__timeout,
> 			 const __sigset_t *__ss)
> 	__attr_access ((__write_only__, 2, 3));
> 
> 
> /usr/include/x86_64-linux-gnu/sys/epoll.h:157:
> #  define epoll_pwait2 __epoll_pwait2_time64
> 
> 
> The man page synopsis is:
> 
> 
> SYNOPSIS
>        #include <sys/epoll.h>
> 
>        int epoll_wait(int epfd, struct epoll_event *events,
>                       int maxevents, int timeout);
>        int epoll_pwait(int epfd, struct epoll_event *events,
>                       int maxevents, int timeout,
>                       const sigset_t *_Nullable sigmask);
>        int epoll_pwait2(int epfd, struct epoll_event *events,
>                       int maxevents, const struct timespec *_Nullable timeout,
>                       const sigset_t *_Nullable sigmask);
> 
> 
> I didn't use _Nullable in the events parameter, because I don't think
> it can be NULL.  Does it make any sense in any case having a NULL
> there?  Should we use nonnull in glibc?

It does make sense to mark the epoll_events argument as nonnull, since
either the kernel will return EINVAL for maxevents equal to 0 or
EFAULT for invalid events:

fs/eventpoll.c

2284  * Implement the event wait interface for the eventpoll file. It is the kernel
2285  * part of the user space epoll_wait(2).
2286  */
2287 static int do_epoll_wait(int epfd, struct epoll_event __user *events,
2288                          int maxevents, struct timespec64 *to)
2289 {
2290         int error;
2291         struct fd f;
2292         struct eventpoll *ep;
2293
2294         /* The maximum number of event must be greater than zero */
2295         if (maxevents <= 0 || maxevents > EP_MAX_EVENTS)
2296                 return -EINVAL;
2297
2298         /* Verify that the area passed by the user is writeable */
2299         if (!access_ok(events, maxevents * sizeof(struct epoll_event)))
2300                 return -EFAULT;

[PATCH] Use __nonnull for the epoll_wait(2) family of syscalls

[Alejandro Colomar]

Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 include/sys/epoll.h                 | 3 ++-
 sysdeps/unix/sysv/linux/sys/epoll.h | 8 ++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/sys/epoll.h b/include/sys/epoll.h
index 8049381a26..b23bc9c7c0 100644
--- a/include/sys/epoll.h
+++ b/include/sys/epoll.h
@@ -9,7 +9,8 @@ libc_hidden_proto (epoll_pwait)
 #else
 extern int __epoll_pwait2_time64 (int fd, struct epoll_event *ev, int maxev,
 				  const struct __timespec64 *tmo,
-				  const sigset_t *s);
+				  const sigset_t *s)
+       __nonnull ((2));
 libc_hidden_proto (__epoll_pwait2_time64)
 #endif
 
diff --git a/sysdeps/unix/sysv/linux/sys/epoll.h b/sysdeps/unix/sysv/linux/sys/epoll.h
index b17d344e79..23872c9438 100644
--- a/sysdeps/unix/sysv/linux/sys/epoll.h
+++ b/sysdeps/unix/sysv/linux/sys/epoll.h
@@ -123,7 +123,7 @@ extern int epoll_ctl (int __epfd, int __op, int __fd,
    __THROW.  */
 extern int epoll_wait (int __epfd, struct epoll_event *__events,
 		       int __maxevents, int __timeout)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 
 
 /* Same as epoll_wait, but the thread's signal mask is temporarily
@@ -134,7 +134,7 @@ extern int epoll_wait (int __epfd, struct epoll_event *__events,
 extern int epoll_pwait (int __epfd, struct epoll_event *__events,
 			int __maxevents, int __timeout,
 			const __sigset_t *__ss)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 
 /* Same as epoll_pwait, but the timeout as a timespec.
 
@@ -144,7 +144,7 @@ extern int epoll_pwait (int __epfd, struct epoll_event *__events,
 extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
 			 int __maxevents, const struct timespec *__timeout,
 			 const __sigset_t *__ss)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 #else
 # ifdef __REDIRECT
 extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
@@ -152,7 +152,7 @@ extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
 				      const struct timespec *__timeout,
 				      const __sigset_t *__ss),
 		       __epoll_pwait2_time64)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 # else
 #  define epoll_pwait2 __epoll_pwait2_time64
 # endif
-- 
2.40.1

Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls

[Adhemerval Zanella Netto]

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

On 22/05/23 19:01, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
>  include/sys/epoll.h                 | 3 ++-
>  sysdeps/unix/sysv/linux/sys/epoll.h | 8 ++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sys/epoll.h b/include/sys/epoll.h
> index 8049381a26..b23bc9c7c0 100644
> --- a/include/sys/epoll.h
> +++ b/include/sys/epoll.h
> @@ -9,7 +9,8 @@ libc_hidden_proto (epoll_pwait)
>  #else
>  extern int __epoll_pwait2_time64 (int fd, struct epoll_event *ev, int maxev,
>  				  const struct __timespec64 *tmo,
> -				  const sigset_t *s);
> +				  const sigset_t *s)
> +       __nonnull ((2));
>  libc_hidden_proto (__epoll_pwait2_time64)
>  #endif
>  
> diff --git a/sysdeps/unix/sysv/linux/sys/epoll.h b/sysdeps/unix/sysv/linux/sys/epoll.h
> index b17d344e79..23872c9438 100644
> --- a/sysdeps/unix/sysv/linux/sys/epoll.h
> +++ b/sysdeps/unix/sysv/linux/sys/epoll.h
> @@ -123,7 +123,7 @@ extern int epoll_ctl (int __epfd, int __op, int __fd,
>     __THROW.  */
>  extern int epoll_wait (int __epfd, struct epoll_event *__events,
>  		       int __maxevents, int __timeout)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  
>  
>  /* Same as epoll_wait, but the thread's signal mask is temporarily
> @@ -134,7 +134,7 @@ extern int epoll_wait (int __epfd, struct epoll_event *__events,
>  extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>  			int __maxevents, int __timeout,
>  			const __sigset_t *__ss)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  
>  /* Same as epoll_pwait, but the timeout as a timespec.
>  
> @@ -144,7 +144,7 @@ extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>  extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
>  			 int __maxevents, const struct timespec *__timeout,
>  			 const __sigset_t *__ss)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  #else
>  # ifdef __REDIRECT
>  extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
> @@ -152,7 +152,7 @@ extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
>  				      const struct timespec *__timeout,
>  				      const __sigset_t *__ss),
>  		       __epoll_pwait2_time64)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  # else
>  #  define epoll_pwait2 __epoll_pwait2_time64
>  # endif

Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls

[Adhemerval Zanella Netto]

In fact, checking I am seeing a regression:

../sysdeps/unix/sysv/linux/tst-epoll.c: In function ‘do_test’:
../sysdeps/unix/sysv/linux/tst-epoll.c:194:11: error: argument 2 null where non-null expected [-Werror=nonnull]
  194 |   int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
      |           ^~~~~~~~~~~~
In file included from ../include/sys/epoll.h:2,
                 from ../sysdeps/unix/sysv/linux/tst-epoll.c:27:
../sysdeps/unix/sysv/linux/sys/epoll.h:144:12: note: in a call to function ‘epoll_pwait2’ declared ‘nonnull’
  144 | extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
      |            ^~~~~~~~~~~~
cc1: all warnings being treated as errors

And I am not sure why it was not caught by buildbots.

The check is only for test for epoll_pwait2 support, so I think it would be simpler to
just suppress the warning:

diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
index 66f091c202..e2fd34e0e6 100644
--- a/sysdeps/unix/sysv/linux/tst-epoll.c
+++ b/sysdeps/unix/sysv/linux/tst-epoll.c
@@ -18,12 +18,13 @@

 #include <errno.h>
 #include <intprops.h>
+#include <libc-diag.h>
+#include <stdlib.h>
 #include <support/check.h>
 #include <support/support.h>
 #include <support/xsignal.h>
-#include <support/xunistd.h>
 #include <support/xtime.h>
-#include <stdlib.h>
+#include <support/xunistd.h>
 #include <sys/epoll.h>

 /* The test focus on checking if the timeout argument is correctly handled
@@ -191,7 +192,12 @@ do_test (void)
     xsigaction (SIGCHLD, &sa, NULL);
   }

+  /* The NULL tests here is only to check if epoll_pwait2 is supported by the
+     kernel and to simplify the rest of test.  */
+  DIAG_PUSH_NEEDS_COMMENT;
+  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
   int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
+  DIAG_POP_NEEDS_COMMENT;
   TEST_COMPARE (r, -1);
   bool pwait2_supported = errno != ENOSYS;

Could you send a v3 with the change?  Another possibility is to remove the
pwait2_supported and handle it on the test itself (it would require more
extensive changes).

On 23/05/23 09:27, Adhemerval Zanella Netto wrote:
> LGTM, thanks.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
> 
> On 22/05/23 19:01, Alejandro Colomar wrote:
>> Signed-off-by: Alejandro Colomar <alx@kernel.org>
>> ---
>>  include/sys/epoll.h                 | 3 ++-
>>  sysdeps/unix/sysv/linux/sys/epoll.h | 8 ++++----
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/sys/epoll.h b/include/sys/epoll.h
>> index 8049381a26..b23bc9c7c0 100644
>> --- a/include/sys/epoll.h
>> +++ b/include/sys/epoll.h
>> @@ -9,7 +9,8 @@ libc_hidden_proto (epoll_pwait)
>>  #else
>>  extern int __epoll_pwait2_time64 (int fd, struct epoll_event *ev, int maxev,
>>  				  const struct __timespec64 *tmo,
>> -				  const sigset_t *s);
>> +				  const sigset_t *s)
>> +       __nonnull ((2));
>>  libc_hidden_proto (__epoll_pwait2_time64)
>>  #endif
>>  
>> diff --git a/sysdeps/unix/sysv/linux/sys/epoll.h b/sysdeps/unix/sysv/linux/sys/epoll.h
>> index b17d344e79..23872c9438 100644
>> --- a/sysdeps/unix/sysv/linux/sys/epoll.h
>> +++ b/sysdeps/unix/sysv/linux/sys/epoll.h
>> @@ -123,7 +123,7 @@ extern int epoll_ctl (int __epfd, int __op, int __fd,
>>     __THROW.  */
>>  extern int epoll_wait (int __epfd, struct epoll_event *__events,
>>  		       int __maxevents, int __timeout)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  
>>  
>>  /* Same as epoll_wait, but the thread's signal mask is temporarily
>> @@ -134,7 +134,7 @@ extern int epoll_wait (int __epfd, struct epoll_event *__events,
>>  extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>>  			int __maxevents, int __timeout,
>>  			const __sigset_t *__ss)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  
>>  /* Same as epoll_pwait, but the timeout as a timespec.
>>  
>> @@ -144,7 +144,7 @@ extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>>  extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
>>  			 int __maxevents, const struct timespec *__timeout,
>>  			 const __sigset_t *__ss)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  #else
>>  # ifdef __REDIRECT
>>  extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
>> @@ -152,7 +152,7 @@ extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
>>  				      const struct timespec *__timeout,
>>  				      const __sigset_t *__ss),
>>  		       __epoll_pwait2_time64)
>> -	__attr_access ((__write_only__, 2, 3));
>> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>>  # else
>>  #  define epoll_pwait2 __epoll_pwait2_time64
>>  # endif

Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 3574 bytes --]

Hi Adhemerval!

On 5/29/23 19:39, Adhemerval Zanella Netto wrote:
> In fact, checking I am seeing a regression:
> 
> ../sysdeps/unix/sysv/linux/tst-epoll.c: In function ‘do_test’:
> ../sysdeps/unix/sysv/linux/tst-epoll.c:194:11: error: argument 2 null where non-null expected [-Werror=nonnull]
>   194 |   int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
>       |           ^~~~~~~~~~~~
> In file included from ../include/sys/epoll.h:2,
>                  from ../sysdeps/unix/sysv/linux/tst-epoll.c:27:
> ../sysdeps/unix/sysv/linux/sys/epoll.h:144:12: note: in a call to function ‘epoll_pwait2’ declared ‘nonnull’
>   144 | extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
>       |            ^~~~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> And I am not sure why it was not caught by buildbots.

I didn't catch it either.  I didn't run tests, since I thought if anything failed, it
would probably be at compilation stage, so I only compiled.  Not sure about the
buildbots, though; those should have caught it.

> 
> The check is only for test for epoll_pwait2 support, so I think it would be simpler to
> just suppress the warning:
> 
> diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
> index 66f091c202..e2fd34e0e6 100644
> --- a/sysdeps/unix/sysv/linux/tst-epoll.c
> +++ b/sysdeps/unix/sysv/linux/tst-epoll.c
> @@ -18,12 +18,13 @@
> 
>  #include <errno.h>
>  #include <intprops.h>
> +#include <libc-diag.h>
> +#include <stdlib.h>
>  #include <support/check.h>
>  #include <support/support.h>
>  #include <support/xsignal.h>
> -#include <support/xunistd.h>
>  #include <support/xtime.h>
> -#include <stdlib.h>
> +#include <support/xunistd.h>
>  #include <sys/epoll.h>
> 
>  /* The test focus on checking if the timeout argument is correctly handled
> @@ -191,7 +192,12 @@ do_test (void)
>      xsigaction (SIGCHLD, &sa, NULL);
>    }
> 
> +  /* The NULL tests here is only to check if epoll_pwait2 is supported by the
> +     kernel and to simplify the rest of test.  */
> +  DIAG_PUSH_NEEDS_COMMENT;
> +  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>    int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
> +  DIAG_POP_NEEDS_COMMENT;
>    TEST_COMPARE (r, -1);
>    bool pwait2_supported = errno != ENOSYS;
> 
> Could you send a v3 with the change?  Another possibility is to remove the
> pwait2_supported and handle it on the test itself (it would require more
> extensive changes).

I suggest a slightly different approach: passing the address of a dummy variable.
It is even simpler.  See the suggested diff below.  If you like it, I'll send a
revision with it (which IIRC, should be v2).

Cheers,
Alex

$ git diff
diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
index 66f091c202..abda45c427 100644
--- a/sysdeps/unix/sysv/linux/tst-epoll.c
+++ b/sysdeps/unix/sysv/linux/tst-epoll.c
@@ -180,6 +180,8 @@ epoll_pwait2_check (int epfd, struct epoll_event *ev, int maxev, int tmo,
 static int
 do_test (void)
 {
+  struct epoll_event ev;
+
   {
     struct sigaction sa;
     sa.sa_handler = handler;
@@ -191,7 +193,7 @@ do_test (void)
     xsigaction (SIGCHLD, &sa, NULL);
   }
 
-  int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
+  int r = epoll_pwait2 (-1, &ev, 0, NULL, NULL);
   TEST_COMPARE (r, -1);
   bool pwait2_supported = errno != ENOSYS;
 

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] Use __nonnull for the epoll_wait(2) family of syscalls

[Adhemerval Zanella Netto]

On 29/05/23 20:17, Alejandro Colomar wrote:
> Hi Adhemerval!
> 
> On 5/29/23 19:39, Adhemerval Zanella Netto wrote:
>> In fact, checking I am seeing a regression:
>>
>> ../sysdeps/unix/sysv/linux/tst-epoll.c: In function ‘do_test’:
>> ../sysdeps/unix/sysv/linux/tst-epoll.c:194:11: error: argument 2 null where non-null expected [-Werror=nonnull]
>>   194 |   int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
>>       |           ^~~~~~~~~~~~
>> In file included from ../include/sys/epoll.h:2,
>>                  from ../sysdeps/unix/sysv/linux/tst-epoll.c:27:
>> ../sysdeps/unix/sysv/linux/sys/epoll.h:144:12: note: in a call to function ‘epoll_pwait2’ declared ‘nonnull’
>>   144 | extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
>>       |            ^~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>>
>> And I am not sure why it was not caught by buildbots.
> 
> I didn't catch it either.  I didn't run tests, since I thought if anything failed, it
> would probably be at compilation stage, so I only compiled.  Not sure about the
> buildbots, though; those should have caught it.

We consider tests build failures as regression as well, at least the fix is
catching bogus usages.  You can just build the tests without running by
issuing 'make check run-built-tests=no', which speed up tests for this
kind of changes.

> 
>>
>> The check is only for test for epoll_pwait2 support, so I think it would be simpler to
>> just suppress the warning:
>>
>> diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
>> index 66f091c202..e2fd34e0e6 100644
>> --- a/sysdeps/unix/sysv/linux/tst-epoll.c
>> +++ b/sysdeps/unix/sysv/linux/tst-epoll.c
>> @@ -18,12 +18,13 @@
>>
>>  #include <errno.h>
>>  #include <intprops.h>
>> +#include <libc-diag.h>
>> +#include <stdlib.h>
>>  #include <support/check.h>
>>  #include <support/support.h>
>>  #include <support/xsignal.h>
>> -#include <support/xunistd.h>
>>  #include <support/xtime.h>
>> -#include <stdlib.h>
>> +#include <support/xunistd.h>
>>  #include <sys/epoll.h>
>>
>>  /* The test focus on checking if the timeout argument is correctly handled
>> @@ -191,7 +192,12 @@ do_test (void)
>>      xsigaction (SIGCHLD, &sa, NULL);
>>    }
>>
>> +  /* The NULL tests here is only to check if epoll_pwait2 is supported by the
>> +     kernel and to simplify the rest of test.  */
>> +  DIAG_PUSH_NEEDS_COMMENT;
>> +  DIAG_IGNORE_NEEDS_COMMENT (7, "-Wnonnull");
>>    int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
>> +  DIAG_POP_NEEDS_COMMENT;
>>    TEST_COMPARE (r, -1);
>>    bool pwait2_supported = errno != ENOSYS;
>>
>> Could you send a v3 with the change?  Another possibility is to remove the
>> pwait2_supported and handle it on the test itself (it would require more
>> extensive changes).
> 
> I suggest a slightly different approach: passing the address of a dummy variable.
> It is even simpler.  See the suggested diff below.  If you like it, I'll send a
> revision with it (which IIRC, should be v2).

Either version works for me, thanks!

> 
> Cheers,
> Alex
> 
> $ git diff
> diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
> index 66f091c202..abda45c427 100644
> --- a/sysdeps/unix/sysv/linux/tst-epoll.c
> +++ b/sysdeps/unix/sysv/linux/tst-epoll.c
> @@ -180,6 +180,8 @@ epoll_pwait2_check (int epfd, struct epoll_event *ev, int maxev, int tmo,
>  static int
>  do_test (void)
>  {
> +  struct epoll_event ev;
> +
>    {
>      struct sigaction sa;
>      sa.sa_handler = handler;
> @@ -191,7 +193,7 @@ do_test (void)
>      xsigaction (SIGCHLD, &sa, NULL);
>    }
>  
> -  int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
> +  int r = epoll_pwait2 (-1, &ev, 0, NULL, NULL);
>    TEST_COMPARE (r, -1);
>    bool pwait2_supported = errno != ENOSYS;
>  
> 

[PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test

[Alejandro Colomar]

epoll_pwait2(2)'s second argument should be nonnull.  We're going to add
__nonnull to the prototype, so let's fix the test accordingly.  We can
use a dummy variable to avoid passing NULL.

Reported-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 sysdeps/unix/sysv/linux/tst-epoll.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
index 66f091c202..abda45c427 100644
--- a/sysdeps/unix/sysv/linux/tst-epoll.c
+++ b/sysdeps/unix/sysv/linux/tst-epoll.c
@@ -180,6 +180,8 @@ epoll_pwait2_check (int epfd, struct epoll_event *ev, int maxev, int tmo,
 static int
 do_test (void)
 {
+  struct epoll_event ev;
+
   {
     struct sigaction sa;
     sa.sa_handler = handler;
@@ -191,7 +193,7 @@ do_test (void)
     xsigaction (SIGCHLD, &sa, NULL);
   }
 
-  int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
+  int r = epoll_pwait2 (-1, &ev, 0, NULL, NULL);
   TEST_COMPARE (r, -1);
   bool pwait2_supported = errno != ENOSYS;
 
-- 
2.40.1

Re: [PATCH v2 1/2] Fix invalid use of NULL in epoll_pwait2(2) test

[Adhemerval Zanella Netto]

On 31/05/23 17:44, Alejandro Colomar wrote:
> epoll_pwait2(2)'s second argument should be nonnull.  We're going to add
> __nonnull to the prototype, so let's fix the test accordingly.  We can
> use a dummy variable to avoid passing NULL.
> 
> Reported-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/unix/sysv/linux/tst-epoll.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/tst-epoll.c b/sysdeps/unix/sysv/linux/tst-epoll.c
> index 66f091c202..abda45c427 100644
> --- a/sysdeps/unix/sysv/linux/tst-epoll.c
> +++ b/sysdeps/unix/sysv/linux/tst-epoll.c
> @@ -180,6 +180,8 @@ epoll_pwait2_check (int epfd, struct epoll_event *ev, int maxev, int tmo,
>  static int
>  do_test (void)
>  {
> +  struct epoll_event ev;
> +
>    {
>      struct sigaction sa;
>      sa.sa_handler = handler;
> @@ -191,7 +193,7 @@ do_test (void)
>      xsigaction (SIGCHLD, &sa, NULL);
>    }
>  
> -  int r = epoll_pwait2 (-1, NULL, 0, NULL, NULL);
> +  int r = epoll_pwait2 (-1, &ev, 0, NULL, NULL);
>    TEST_COMPARE (r, -1);
>    bool pwait2_supported = errno != ENOSYS;
>  

[PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls

[Alejandro Colomar]

Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 include/sys/epoll.h                 | 3 ++-
 sysdeps/unix/sysv/linux/sys/epoll.h | 8 ++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/sys/epoll.h b/include/sys/epoll.h
index 8049381a26..b23bc9c7c0 100644
--- a/include/sys/epoll.h
+++ b/include/sys/epoll.h
@@ -9,7 +9,8 @@ libc_hidden_proto (epoll_pwait)
 #else
 extern int __epoll_pwait2_time64 (int fd, struct epoll_event *ev, int maxev,
 				  const struct __timespec64 *tmo,
-				  const sigset_t *s);
+				  const sigset_t *s)
+       __nonnull ((2));
 libc_hidden_proto (__epoll_pwait2_time64)
 #endif
 
diff --git a/sysdeps/unix/sysv/linux/sys/epoll.h b/sysdeps/unix/sysv/linux/sys/epoll.h
index b17d344e79..23872c9438 100644
--- a/sysdeps/unix/sysv/linux/sys/epoll.h
+++ b/sysdeps/unix/sysv/linux/sys/epoll.h
@@ -123,7 +123,7 @@ extern int epoll_ctl (int __epfd, int __op, int __fd,
    __THROW.  */
 extern int epoll_wait (int __epfd, struct epoll_event *__events,
 		       int __maxevents, int __timeout)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 
 
 /* Same as epoll_wait, but the thread's signal mask is temporarily
@@ -134,7 +134,7 @@ extern int epoll_wait (int __epfd, struct epoll_event *__events,
 extern int epoll_pwait (int __epfd, struct epoll_event *__events,
 			int __maxevents, int __timeout,
 			const __sigset_t *__ss)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 
 /* Same as epoll_pwait, but the timeout as a timespec.
 
@@ -144,7 +144,7 @@ extern int epoll_pwait (int __epfd, struct epoll_event *__events,
 extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
 			 int __maxevents, const struct timespec *__timeout,
 			 const __sigset_t *__ss)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 #else
 # ifdef __REDIRECT
 extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
@@ -152,7 +152,7 @@ extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
 				      const struct timespec *__timeout,
 				      const __sigset_t *__ss),
 		       __epoll_pwait2_time64)
-	__attr_access ((__write_only__, 2, 3));
+	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
 # else
 #  define epoll_pwait2 __epoll_pwait2_time64
 # endif
-- 
2.40.1

Re: [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls

[Adhemerval Zanella Netto]

On 31/05/23 17:44, Alejandro Colomar wrote:
> Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>


LGTM, although the buildbot failed to apply the patch for some reason [1].
It does apply if I use git-pw.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

[1] https://patchwork.sourceware.org/project/glibc/patch/20230531204422.18052-2-alx@kernel.org/

> ---
>  include/sys/epoll.h                 | 3 ++-
>  sysdeps/unix/sysv/linux/sys/epoll.h | 8 ++++----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/sys/epoll.h b/include/sys/epoll.h
> index 8049381a26..b23bc9c7c0 100644
> --- a/include/sys/epoll.h
> +++ b/include/sys/epoll.h
> @@ -9,7 +9,8 @@ libc_hidden_proto (epoll_pwait)
>  #else
>  extern int __epoll_pwait2_time64 (int fd, struct epoll_event *ev, int maxev,
>  				  const struct __timespec64 *tmo,
> -				  const sigset_t *s);
> +				  const sigset_t *s)
> +       __nonnull ((2));
>  libc_hidden_proto (__epoll_pwait2_time64)
>  #endif
>  
> diff --git a/sysdeps/unix/sysv/linux/sys/epoll.h b/sysdeps/unix/sysv/linux/sys/epoll.h
> index b17d344e79..23872c9438 100644
> --- a/sysdeps/unix/sysv/linux/sys/epoll.h
> +++ b/sysdeps/unix/sysv/linux/sys/epoll.h
> @@ -123,7 +123,7 @@ extern int epoll_ctl (int __epfd, int __op, int __fd,
>     __THROW.  */
>  extern int epoll_wait (int __epfd, struct epoll_event *__events,
>  		       int __maxevents, int __timeout)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  
>  
>  /* Same as epoll_wait, but the thread's signal mask is temporarily
> @@ -134,7 +134,7 @@ extern int epoll_wait (int __epfd, struct epoll_event *__events,
>  extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>  			int __maxevents, int __timeout,
>  			const __sigset_t *__ss)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  
>  /* Same as epoll_pwait, but the timeout as a timespec.
>  
> @@ -144,7 +144,7 @@ extern int epoll_pwait (int __epfd, struct epoll_event *__events,
>  extern int epoll_pwait2 (int __epfd, struct epoll_event *__events,
>  			 int __maxevents, const struct timespec *__timeout,
>  			 const __sigset_t *__ss)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  #else
>  # ifdef __REDIRECT
>  extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
> @@ -152,7 +152,7 @@ extern int __REDIRECT (epoll_pwait2, (int __epfd, struct epoll_event *__ev,
>  				      const struct timespec *__timeout,
>  				      const __sigset_t *__ss),
>  		       __epoll_pwait2_time64)
> -	__attr_access ((__write_only__, 2, 3));
> +	__attr_access ((__write_only__, 2, 3)) __nonnull ((2));
>  # else
>  #  define epoll_pwait2 __epoll_pwait2_time64
>  # endif

Re: [PATCH v2 2/2] Use __nonnull for the epoll_wait(2) family of syscalls

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 894 bytes --]

On 6/1/23 20:00, Adhemerval Zanella Netto wrote:
> 
> 
> On 31/05/23 17:44, Alejandro Colomar wrote:
>> Cc: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
>> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> 
> 
> LGTM, although the buildbot failed to apply the patch for some reason [1].

Don't know; maybe I forgot to rebase to the latest master, and some context
changed, and it applied with fuzz... git can apply it, but the buildbot is
probably more cautious.

> It does apply if I use git-pw.
> 
> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

Thank you!

(you forgot to actually put this tag in 1/2 :)

Cheers,
Alex

> 
> [1] https://patchwork.sourceware.org/project/glibc/patch/20230531204422.18052-2-alx@kernel.org/
> 

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]