Marking select(2) as restrict

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Marking select(2) as restrict

Eitan Adler-4
Hi all,

POSIX requires that the fd_set arguments in select(2) be marked as
restrict. This patch attempts to implement that.

(a) Am I missing anything?
(b) Anything in particular to watch out for?
(c) Assuming an exp-run passes any reason not to commit?


Index: lib/libc/sys/select.2
===================================================================
--- lib/libc/sys/select.2 (revision 329296)
+++ lib/libc/sys/select.2 (working copy)
@@ -39,7 +39,7 @@
 .Sh SYNOPSIS
 .In sys/select.h
 .Ft int
-.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
*exceptfds" "struct timeval *timeout"
+.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
 .Fn FD_SET fd &fdset
 .Fn FD_CLR fd &fdset
 .Fn FD_ISSET fd &fdset
Index: lib/libc/sys/select.c
===================================================================
--- lib/libc/sys/select.c (revision 329296)
+++ lib/libc/sys/select.c (working copy)
@@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);

 #pragma weak select
 int
-select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
+select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
restrict es, struct timeval *t)
 {

  return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval *))
Index: sys/sys/select.h
===================================================================
--- sys/sys/select.h (revision 329296)
+++ sys/sys/select.h (working copy)
@@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
  const struct timespec *__restrict, const sigset_t *__restrict);
 #ifndef _SELECT_DECLARED
 #define _SELECT_DECLARED
-/* XXX missing restrict type-qualifier */
-int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
+int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
*__restrict, struct timeval *);
 #endif
 __END_DECLS
 #endif /* !_KERNEL */


--
Eitan Adler
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Eitan Adler-4
I filed a request for a slightly modified version of this patch to be
exp-run. I'm planning on committing unless there is significant
fallout or objections on this list.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225981

On 15 February 2018 at 00:10, Eitan Adler <[hidden email]> wrote:

> Hi all,
>
> POSIX requires that the fd_set arguments in select(2) be marked as
> restrict. This patch attempts to implement that.
>
> (a) Am I missing anything?
> (b) Anything in particular to watch out for?
> (c) Assuming an exp-run passes any reason not to commit?
>
>
> Index: lib/libc/sys/select.2
> ===================================================================
> --- lib/libc/sys/select.2 (revision 329296)
> +++ lib/libc/sys/select.2 (working copy)
> @@ -39,7 +39,7 @@
>  .Sh SYNOPSIS
>  .In sys/select.h
>  .Ft int
> -.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
> *exceptfds" "struct timeval *timeout"
> +.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
> writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
>  .Fn FD_SET fd &fdset
>  .Fn FD_CLR fd &fdset
>  .Fn FD_ISSET fd &fdset
> Index: lib/libc/sys/select.c
> ===================================================================
> --- lib/libc/sys/select.c (revision 329296)
> +++ lib/libc/sys/select.c (working copy)
> @@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);
>
>  #pragma weak select
>  int
> -select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
> +select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
> restrict es, struct timeval *t)
>  {
>
>   return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval *))
> Index: sys/sys/select.h
> ===================================================================
> --- sys/sys/select.h (revision 329296)
> +++ sys/sys/select.h (working copy)
> @@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
>   const struct timespec *__restrict, const sigset_t *__restrict);
>  #ifndef _SELECT_DECLARED
>  #define _SELECT_DECLARED
> -/* XXX missing restrict type-qualifier */
> -int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
> +int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
> *__restrict, struct timeval *);
>  #endif
>  __END_DECLS
>  #endif /* !_KERNEL */
>
>
> --
> Eitan Adler



--
Eitan Adler
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Stefan Blachmann
As the last C standard I know is ANSI, I was curious and read what
restrict does (https://en.wikipedia.org/wiki/Restrict).

To understand more about select() I read in the Linux tutorial
(http://man7.org/linux/man-pages/man2/select_tut.2.html) .
The section "Select Law" looks like that things could break easy.

Personally (in particular because I don't know the matter) I would be
afraid that making select pointers restrict could cause hard-to debug
misbehavior of a few programs that are already working borderline
(linux ports etc).

But as said, this is my unqualified guess.
And, few, almost no documents on select show it as restrict.

Always following Posix isn't necessarily good, as many don't care about it.
For example, with 11.0 the default Posix conform SHM setting was
changed to non-Posix-compliant, because posix conformity caused just
too many people troubles.
And, maybe the gain would be little anyway?

Just my worthless 2 cents.
Have a good day @hackers :)
Stefan


On 2/21/18, Eitan Adler <[hidden email]> wrote:

> I filed a request for a slightly modified version of this patch to be
> exp-run. I'm planning on committing unless there is significant
> fallout or objections on this list.
>
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225981
>
> On 15 February 2018 at 00:10, Eitan Adler <[hidden email]> wrote:
>> Hi all,
>>
>> POSIX requires that the fd_set arguments in select(2) be marked as
>> restrict. This patch attempts to implement that.
>>
>> (a) Am I missing anything?
>> (b) Anything in particular to watch out for?
>> (c) Assuming an exp-run passes any reason not to commit?
>>
>>
>> Index: lib/libc/sys/select.2
>> ===================================================================
>> --- lib/libc/sys/select.2 (revision 329296)
>> +++ lib/libc/sys/select.2 (working copy)
>> @@ -39,7 +39,7 @@
>>  .Sh SYNOPSIS
>>  .In sys/select.h
>>  .Ft int
>> -.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
>> *exceptfds" "struct timeval *timeout"
>> +.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
>> writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
>>  .Fn FD_SET fd &fdset
>>  .Fn FD_CLR fd &fdset
>>  .Fn FD_ISSET fd &fdset
>> Index: lib/libc/sys/select.c
>> ===================================================================
>> --- lib/libc/sys/select.c (revision 329296)
>> +++ lib/libc/sys/select.c (working copy)
>> @@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);
>>
>>  #pragma weak select
>>  int
>> -select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
>> +select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
>> restrict es, struct timeval *t)
>>  {
>>
>>   return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval *))
>> Index: sys/sys/select.h
>> ===================================================================
>> --- sys/sys/select.h (revision 329296)
>> +++ sys/sys/select.h (working copy)
>> @@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
>>   const struct timespec *__restrict, const sigset_t *__restrict);
>>  #ifndef _SELECT_DECLARED
>>  #define _SELECT_DECLARED
>> -/* XXX missing restrict type-qualifier */
>> -int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
>> +int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
>> *__restrict, struct timeval *);
>>  #endif
>>  __END_DECLS
>>  #endif /* !_KERNEL */
>>
>>
>> --
>> Eitan Adler
>
>
>
> --
> Eitan Adler
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Kevin Lo-2
In reply to this post by Eitan Adler-4
On Tue, Feb 20, 2018 at 04:29:59PM -0800, Eitan Adler wrote:
>
> I filed a request for a slightly modified version of this patch to be
> exp-run. I'm planning on committing unless there is significant
> fallout or objections on this list.
>
> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225981

Please send your patch to standards@.  The freebsd-standards mailing list
was created for precisely this purpose, thanks.

> On 15 February 2018 at 00:10, Eitan Adler <[hidden email]> wrote:
> > Hi all,
> >
> > POSIX requires that the fd_set arguments in select(2) be marked as
> > restrict. This patch attempts to implement that.
> >
> > (a) Am I missing anything?
> > (b) Anything in particular to watch out for?
> > (c) Assuming an exp-run passes any reason not to commit?
> >
> >
> > Index: lib/libc/sys/select.2
> > ===================================================================
> > --- lib/libc/sys/select.2 (revision 329296)
> > +++ lib/libc/sys/select.2 (working copy)
> > @@ -39,7 +39,7 @@
> >  .Sh SYNOPSIS
> >  .In sys/select.h
> >  .Ft int
> > -.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
> > *exceptfds" "struct timeval *timeout"
> > +.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
> > writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
> >  .Fn FD_SET fd &fdset
> >  .Fn FD_CLR fd &fdset
> >  .Fn FD_ISSET fd &fdset
> > Index: lib/libc/sys/select.c
> > ===================================================================
> > --- lib/libc/sys/select.c (revision 329296)
> > +++ lib/libc/sys/select.c (working copy)
> > @@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);
> >
> >  #pragma weak select
> >  int
> > -select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
> > +select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
> > restrict es, struct timeval *t)
> >  {
> >
> >   return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval *))
> > Index: sys/sys/select.h
> > ===================================================================
> > --- sys/sys/select.h (revision 329296)
> > +++ sys/sys/select.h (working copy)
> > @@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
> >   const struct timespec *__restrict, const sigset_t *__restrict);
> >  #ifndef _SELECT_DECLARED
> >  #define _SELECT_DECLARED
> > -/* XXX missing restrict type-qualifier */
> > -int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
> > +int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
> > *__restrict, struct timeval *);
> >  #endif
> >  __END_DECLS
> >  #endif /* !_KERNEL */
> >
> >
> > --
> > Eitan Adler
>
>
>
> --
> Eitan Adler
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Julian Elischer-5
In reply to this post by Stefan Blachmann

> On 2/21/18, Eitan Adler <[hidden email]> wrote:
>> I filed a request for a slightly modified version of this patch to be
>> exp-run. I'm planning on committing unless there is significant
>> fallout or objections on this list.
>>
>> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225981
>>
>> On 15 February 2018 at 00:10, Eitan Adler <[hidden email]> wrote:
>>> Hi all,
>>>
>>> POSIX requires that the fd_set arguments in select(2) be marked as
>>> restrict. This patch attempts to implement that.
>>>
>>> (a) Am I missing anything?
>>> (b) Anything in particular to watch out for?
>>> (c) Assuming an exp-run passes any reason not to commit?
>>>
>>>
>>> Index: lib/libc/sys/select.2
>>> ===================================================================
>>> --- lib/libc/sys/select.2 (revision 329296)
>>> +++ lib/libc/sys/select.2 (working copy)
>>> @@ -39,7 +39,7 @@
>>>   .Sh SYNOPSIS
>>>   .In sys/select.h
>>>   .Ft int
>>> -.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
>>> *exceptfds" "struct timeval *timeout"
>>> +.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
>>> writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
>>>   .Fn FD_SET fd &fdset
>>>   .Fn FD_CLR fd &fdset
>>>   .Fn FD_ISSET fd &fdset
>>> Index: lib/libc/sys/select.c
>>> ===================================================================
>>> --- lib/libc/sys/select.c (revision 329296)
>>> +++ lib/libc/sys/select.c (working copy)
>>> @@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);
>>>
>>>   #pragma weak select
>>>   int
>>> -select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
>>> +select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
>>> restrict es, struct timeval *t)
>>>   {
>>>
>>>    return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval *))
>>> Index: sys/sys/select.h
>>> ===================================================================
>>> --- sys/sys/select.h (revision 329296)
>>> +++ sys/sys/select.h (working copy)
>>> @@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
>>>    const struct timespec *__restrict, const sigset_t *__restrict);
>>>   #ifndef _SELECT_DECLARED
>>>   #define _SELECT_DECLARED
>>> -/* XXX missing restrict type-qualifier */
>>> -int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
>>> +int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
>>> *__restrict, struct timeval *);
>>>   #endif
>>>   __END_DECLS
>>>   #endif /* !_KERNEL */
>>>
>>>
>>> --
>>> Eitan Adler
>>
>>
>> --
>> Eitan Adler
>> _______________________________________________
>> [hidden email] mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
>> To unsubscribe, send any mail to "[hidden email]"
>>
So, you are saying that the bitmaps can not be shared..
I can not think of a reason they would be shared but...
  I can also say that I can not think of a proof that there does not
exist a case where it would make sense.

What is the potential gain?  and is it set so in other OS or standards?

Julian
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Julian Elischer-5
In reply to this post by Stefan Blachmann
On 21/2/18 9:52 am, Stefan Blachmann wrote:

> As the last C standard I know is ANSI, I was curious and read what
> restrict does (https://en.wikipedia.org/wiki/Restrict).
>
> To understand more about select() I read in the Linux tutorial
> (http://man7.org/linux/man-pages/man2/select_tut.2.html) .
> The section "Select Law" looks like that things could break easy.
>
> Personally (in particular because I don't know the matter) I would be
> afraid that making select pointers restrict could cause hard-to debug
> misbehavior of a few programs that are already working borderline
> (linux ports etc).
>
> But as said, this is my unqualified guess.
> And, few, almost no documents on select show it as restrict.
>
> Always following Posix isn't necessarily good, as many don't care about it.
> For example, with 11.0 the default Posix conform SHM setting was
> changed to non-Posix-compliant, because posix conformity caused just
> too many people troubles.
> And, maybe the gain would be little anyway?
>
> Just my worthless 2 cents.
> Have a good day @hackers :)
> Stefan
>
>
> On 2/21/18, Eitan Adler <[hidden email]> wrote:
>> I filed a request for a slightly modified version of this patch to be
>> exp-run. I'm planning on committing unless there is significant
>> fallout or objections on this list.
>>
>> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225981
>>
>> On 15 February 2018 at 00:10, Eitan Adler <[hidden email]> wrote:
>>> Hi all,
>>>
>>> POSIX requires that the fd_set arguments in select(2) be marked as
>>> restrict. This patch attempts to implement that.
>>>
>>> (a) Am I missing anything?
>>> (b) Anything in particular to watch out for?
>>> (c) Assuming an exp-run passes any reason not to commit?
>>>
>>>
>>> Index: lib/libc/sys/select.2
>>> ===================================================================
>>> --- lib/libc/sys/select.2 (revision 329296)
>>> +++ lib/libc/sys/select.2 (working copy)
>>> @@ -39,7 +39,7 @@
>>>   .Sh SYNOPSIS
>>>   .In sys/select.h
>>>   .Ft int
>>> -.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
>>> *exceptfds" "struct timeval *timeout"
>>> +.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
>>> writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
>>>   .Fn FD_SET fd &fdset
>>>   .Fn FD_CLR fd &fdset
>>>   .Fn FD_ISSET fd &fdset
>>> Index: lib/libc/sys/select.c
>>> ===================================================================
>>> --- lib/libc/sys/select.c (revision 329296)
>>> +++ lib/libc/sys/select.c (working copy)
>>> @@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);
>>>
>>>   #pragma weak select
>>>   int
>>> -select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
>>> +select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
>>> restrict es, struct timeval *t)
>>>   {
>>>
>>>    return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval *))
>>> Index: sys/sys/select.h
>>> ===================================================================
>>> --- sys/sys/select.h (revision 329296)
>>> +++ sys/sys/select.h (working copy)
>>> @@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
>>>    const struct timespec *__restrict, const sigset_t *__restrict);
>>>   #ifndef _SELECT_DECLARED
>>>   #define _SELECT_DECLARED
>>> -/* XXX missing restrict type-qualifier */
>>> -int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
>>> +int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
>>> *__restrict, struct timeval *);
>>>   #endif
>>>   __END_DECLS
>>>   #endif /* !_KERNEL */
>>>
>>>
>>> --
>>> Eitan Adler
>>
>>
>> --
>> Eitan Adler
>> _______________________________________________
>> [hidden email] mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
>> To unsubscribe, send any mail to "[hidden email]"
>>
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[hidden email]"
>

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Eitan Adler-4
In reply to this post by Kevin Lo-2
Adding standards mailing list

On Tuesday, 20 February 2018, Kevin Lo <[hidden email]> wrote:

> On Tue, Feb 20, 2018 at 04:29:59PM -0800, Eitan Adler wrote:
> >
> > I filed a request for a slightly modified version of this patch to be
> > exp-run. I'm planning on committing unless there is significant
> > fallout or objections on this list.
> >
> > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225981
>
> Please send your patch to standards@.  The freebsd-standards mailing list
> was created for precisely this purpose, thanks.
>
> > On 15 February 2018 at 00:10, Eitan Adler <[hidden email]> wrote:
> > > Hi all,
> > >
> > > POSIX requires that the fd_set arguments in select(2) be marked as
> > > restrict. This patch attempts to implement that.
> > >
> > > (a) Am I missing anything?
> > > (b) Anything in particular to watch out for?
> > > (c) Assuming an exp-run passes any reason not to commit?
> > >
> > >
> > > Index: lib/libc/sys/select.2
> > > ===================================================================
> > > --- lib/libc/sys/select.2 (revision 329296)
> > > +++ lib/libc/sys/select.2 (working copy)
> > > @@ -39,7 +39,7 @@
> > >  .Sh SYNOPSIS
> > >  .In sys/select.h
> > >  .Ft int
> > > -.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
> > > *exceptfds" "struct timeval *timeout"
> > > +.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
> > > writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
> > >  .Fn FD_SET fd &fdset
> > >  .Fn FD_CLR fd &fdset
> > >  .Fn FD_ISSET fd &fdset
> > > Index: lib/libc/sys/select.c
> > > ===================================================================
> > > --- lib/libc/sys/select.c (revision 329296)
> > > +++ lib/libc/sys/select.c (working copy)
> > > @@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);
> > >
> > >  #pragma weak select
> > >  int
> > > -select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
> > > +select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
> > > restrict es, struct timeval *t)
> > >  {
> > >
> > >   return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval
> *))
> > > Index: sys/sys/select.h
> > > ===================================================================
> > > --- sys/sys/select.h (revision 329296)
> > > +++ sys/sys/select.h (working copy)
> > > @@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
> > >   const struct timespec *__restrict, const sigset_t *__restrict);
> > >  #ifndef _SELECT_DECLARED
> > >  #define _SELECT_DECLARED
> > > -/* XXX missing restrict type-qualifier */
> > > -int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
> > > +int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
> > > *__restrict, struct timeval *);
> > >  #endif
> > >  __END_DECLS
> > >  #endif /* !_KERNEL */
> > >
> > >
> > > --
> > > Eitan Adler
> >
> >
> >
> > --
> > Eitan Adler
> > _______________________________________________
> > [hidden email] mailing list
> > https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> > To unsubscribe, send any mail to "
> [hidden email]"
> >
>


--
Sent from my Turing Machine
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Gary Jennejohn-6
In reply to this post by Julian Elischer-5
On Wed, 21 Feb 2018 11:28:44 +0800
Julian Elischer <[hidden email]> wrote:

> > On 2/21/18, Eitan Adler <[hidden email]> wrote:  
> >> I filed a request for a slightly modified version of this patch to be
> >> exp-run. I'm planning on committing unless there is significant
> >> fallout or objections on this list.
> >>
> >> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=225981
> >>
> >> On 15 February 2018 at 00:10, Eitan Adler <[hidden email]> wrote:  
> >>> Hi all,
> >>>
> >>> POSIX requires that the fd_set arguments in select(2) be marked as
> >>> restrict. This patch attempts to implement that.
> >>>
> >>> (a) Am I missing anything?
> >>> (b) Anything in particular to watch out for?
> >>> (c) Assuming an exp-run passes any reason not to commit?
> >>>
> >>>
> >>> Index: lib/libc/sys/select.2
> >>> ===================================================================
> >>> --- lib/libc/sys/select.2 (revision 329296)
> >>> +++ lib/libc/sys/select.2 (working copy)
> >>> @@ -39,7 +39,7 @@
> >>>   .Sh SYNOPSIS
> >>>   .In sys/select.h
> >>>   .Ft int
> >>> -.Fn select "int nfds" "fd_set *readfds" "fd_set *writefds" "fd_set
> >>> *exceptfds" "struct timeval *timeout"
> >>> +.Fn select "int nfds" "fd_set * restrict readfds" "fd_set * restrict
> >>> writefds" "fd_set * restrict exceptfds" "struct timeval *timeout"
> >>>   .Fn FD_SET fd &fdset
> >>>   .Fn FD_CLR fd &fdset
> >>>   .Fn FD_ISSET fd &fdset
> >>> Index: lib/libc/sys/select.c
> >>> ===================================================================
> >>> --- lib/libc/sys/select.c (revision 329296)
> >>> +++ lib/libc/sys/select.c (working copy)
> >>> @@ -41,7 +41,7 @@ __weak_reference(__sys_select, __select);
> >>>
> >>>   #pragma weak select
> >>>   int
> >>> -select(int n, fd_set *rs, fd_set *ws, fd_set *es, struct timeval *t)
> >>> +select(int n, fd_set * restrict rs, fd_set * restrict ws, fd_set *
> >>> restrict es, struct timeval *t)
> >>>   {
> >>>
> >>>    return (((int (*)(int, fd_set *, fd_set *, fd_set *, struct timeval *))
> >>> Index: sys/sys/select.h
> >>> ===================================================================
> >>> --- sys/sys/select.h (revision 329296)
> >>> +++ sys/sys/select.h (working copy)
> >>> @@ -101,8 +101,7 @@ int pselect(int, fd_set *__restrict, fd_set *__res
> >>>    const struct timespec *__restrict, const sigset_t *__restrict);
> >>>   #ifndef _SELECT_DECLARED
> >>>   #define _SELECT_DECLARED
> >>> -/* XXX missing restrict type-qualifier */
> >>> -int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
> >>> +int select(int, fd_set *__restrict, fd_set *__restrict, fd_set
> >>> *__restrict, struct timeval *);
> >>>   #endif
> >>>   __END_DECLS
> >>>   #endif /* !_KERNEL */
> >>>
> >>>
> >>> --
> >>> Eitan Adler  
> >>
> >>
> >> --
> >> Eitan Adler
> >> _______________________________________________
> >> [hidden email] mailing list
> >> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> >> To unsubscribe, send any mail to "[hidden email]"
> >>  
> So, you are saying that the bitmaps can not be shared..
> I can not think of a reason they would be shared but...
>  __I can also say that I can not think of a proof that there does not
> exist a case where it would make sense.
>
> What is the potential gain?__ and is it set so in other OS or standards?
>

Strangely enough, the FreeBSD pselect(2) has restrict on all pointers.
pselect(2) is basically a variant of select(2) with a signal mask.

--
Gary Jennejohn
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Konstantin Belousov
In reply to this post by Eitan Adler-4
On Tue, Feb 20, 2018 at 10:14:05PM -0800, Eitan Adler wrote:
> On 20 February 2018 at 21:19, Warner Losh <[hidden email]> wrote:
> > Once upon a time, this would break a lot of code. Perhaps times have
> > changed.
>
> I've seen very little code that this would break though some of it
> certainly exists.
You certainly seen very little code, but the question was about the
existed code.

>
> > Does the state of the art give warnings,when restrict is violated?
>
> In the general case it can not since aliasing may occur through
> run-time warnings. Modern compilers can warn in specific sub-cases
> though:
What run-time warnings ?  Point out the code that issues them in the
case of restrict (or generic aliasing) violation.  In libc ?  In kernel ?

Did you even tried to estimate how many code around still requires disabling
alias analysis to run ?  How does restrict mix with no-aliasing option ?

Above question contains a hint how it is possible to detect existing calls
which violate the restrict specifier for select(2), but this requires
running of all select(2) consumers, on patched kernel or libc/libthr.

>
> restrict.c:10:6: warning: passing argument 1 to restrict-qualified
> parameter aliases with argument 2 [-Wrestrict]
>   meh(&a, &a);
>
> > If not,
> > how do you propose the ports broken subtlely be detected?
>
> My plan was to commit to current but not MFC. This would allow users
> to detect issues and report them.
And, how to do plan to classify the bugs appeared due the change ?
What specific indicators would allow you to find them out in the stream
of the bug reports ?

>
> Another option is to request that POSIX change the definition of
> select to not require restrict though I am doubtful this will happen.

I fail to find an explanation of the supposed benefit of the change
you proposing.  Please point it out.

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Tijl Coosemans
On Wed, 21 Feb 2018 12:44:00 +0200 Konstantin Belousov <[hidden email]> wrote:
> On Tue, Feb 20, 2018 at 10:14:05PM -0800, Eitan Adler wrote:
>> On 20 February 2018 at 21:19, Warner Losh <[hidden email]> wrote:  
>>> Once upon a time, this would break a lot of code. Perhaps times have
>>> changed.  
>>
>> I've seen very little code that this would break though some of it
>> certainly exists.  
> You certainly seen very little code, but the question was about the
> existed code.

FWIW, it seems that glibc uses restrict since 2000 so there's unlikely to
be much fallout:
https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/sys/select.h
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=98cbe360d947b59e7a5eda068581f4cfeb4b99b3
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Stefan Blachmann
In reply to this post by Konstantin Belousov
The Linux manual pages do not mention restrict for select().
glibc select() itself returns just ENOSYS(), if there is no alias for select().

So I guess what actually gets called is this:
https://github.com/udp/freebsd-libc/blob/master/sys/select.c#L48
Which in turn appears to call __sys_select:
https://github.com/udp/freebsd-libc/blob/master/include/libc_private.h#L346
See also:
https://lists.freebsd.org/pipermail/freebsd-questions/2007-August/154906.html

If I understand correctly, the *only* place that defines the
optimizations actually being done is the static functions itself:
https://github.com/freebsd/freebsd/blob/master/lib/libthr/thread/thr_syscalls.c#L487

So maybe the actual prototypes being used for the
functions for which the interpose array is used are irrelevant:
https://github.com/freebsd/freebsd/blob/master/lib/libthr/thread/thr_syscalls.c#L642

I not yet found out which functions are actually weakly aliased in,
but I could imagine that adding the restrict keyword to the prototypes
of the functions listed there, is possibly only
of cosmetical importance, without any actual effect.
If this is correct, one could be "Posix compliant" without causing any
disruptive  "optimization" :)

Have a nice Sunday!
Stefan


P.S.: Maybe it would be better to avoid adding the restrict keyword in the
__thr_select() function itself mentioned above, as Linux' select
function seems to have no restrict:
https://github.com/torvalds/linux/blob/master/fs/select.c#L1262
https://github.com/torvalds/linux/blob/master/fs/select.c#L599


On 2/25/18, Eitan Adler <[hidden email]> wrote:

> On 24 February 2018 at 10:55, Conrad Meyer <[hidden email]> wrote:
>> On Sat, Feb 24, 2018 at 10:35 AM, Eitan Adler <[hidden email]>
>> wrote:
>>> After this entire thread here is the summary. If I've misrepresented
>>> you here please let me know.
>>> ...
>>>
>>> kib@ - no benefit; concerned fallout could be hard to observe
>>> cem@ - concerned about warnings
>>
>> Consider me a +1 to kib@.  I did not voice those concerns explicitly
>> in earlier email because kib did already and I didn't anticipate you
>> would ignore him.
>
> I am not ignoring him. As I stated above I do not believe fallout is
> likely since most other major libc implementations have already done
> this:
>
> glibc: already done -
> https://github.com/bminor/glibc/blob/master/misc/sys/select.h#L101
> openbsd: already done
> https://github.com/openbsd/src/blob/master/sys/sys/select.h#L128
> dragonflyBSD: alredy done:
> https://github.com/dragonflybsd/dragonflybsd/blob/master/sys/sys/select.h#L50
> netbsd: already done:
> https://github.com/NetBSD/src/blob/trunk/sys/sys/select.h#L69
>
> As a further check I went through the search results on github for
> select() and did not see any failures in the top few pages.
>
> --
> Eitan Adler
> _______________________________________________
> [hidden email] mailing list
> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "[hidden email]"
>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

freebsd-hackers mailing list
In reply to this post by Konstantin Belousov


On 2018-Feb-24, at 5:28 PM, Bruce Evans <[hidden email]> wrote:

> On Sat, 24 Feb 2018, Warner Losh wrote:
>
>> On Sat, Feb 24, 2018 at 11:55 AM, Conrad Meyer <[hidden email]> wrote:
>>
>>> On Sat, Feb 24, 2018 at 10:35 AM, Eitan Adler <[hidden email]>
>>> wrote:
>>>> After this entire thread here is the summary. If I've misrepresented
>>>> you here please let me know.
>>>> ...
>>>>
>>>> kib@ - no benefit; concerned fallout could be hard to observe
>>>> cem@ - concerned about warnings
>>>
>>> Consider me a +1 to kib@.  I did not voice those concerns explicitly
>>> in earlier email because kib did already and I didn't anticipate you
>>> would ignore him.
>>
>> So there's no benefit to the change (we won't optimize better). It's hard
>> to observe breakage. No answer about how we'd even know if something broke
>> because a exp run sure as hell isn't going to tell us.
>>
>> All that militates against the change rather strongly. Your exp run will
>> change no minds because it is useless.
>
> Why not remove restrict from other APIs to be consistent with select()?  If
> might break their callers just as much.

This appears to be about the handling of non-conforming programs. Quoting
from C99's 6.7.3 "Type qualifiers" paragraph 7 about conforming programs:

"The intended use of the restrict qualifier (like the register storage
class) is to promote optimization, and deleting all instances of the
qualifier from all preprocessing units composing a conforming program
does not change its meaning (i.e. observable behavior)."

So: If removing restrict results in a conforming program's observable
behavior changing, then that would be evidence that the environment is
incorrectly implemented relative to C99.

Of course, removing restrict can make it harder to detect non-conforming
programs if the compiler(s) involved are helpful when restrict is
present.


Going the other way:

6.7.3.1 "Formal definition of restrict" also says in its paragraph 6:

"A translator is free to ignore any or all aliasing implications of
uses of restrict."

===
Mark Millard
marklmi at yahoo.com
( markmi at dsl-only.net is
going away in 2018-Feb, late)

_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Chris Torek-3
In reply to this post by Eitan Adler-4
If we can back up a bit, there are (at least) two mostly-separate
issues:

 * What types and qualfiiers to use for various parameters:
   These are dictated by standards, and we should just conform
   to whatever is in the standard unless there's some major
   reason to do otherwise.

   POSIX says that we *should* use "restrict" here:
   http://pubs.opengroup.org/onlinepubs/009696699/functions/pselect.html

   so if no one has a major reason to do otherwise (such as
   "no conforming program can tell that we didn't and all it will
   achieve is nothing" :-) ) I'd say add the restrict.

   (As kib and others have noted, it achieves a lot of nothing, so
   the priority for this particular conformance seems low.)

 * Select itself: it makes no sense *in the caller* to pass
   the same &fd_set argument to more than one parameter unless
   they are all input-only.  The kernel is going to write on them,
   and there is no guarantee that the kernel will write them in any
   particular order.  Hence if you write:

        ret = select(nfds, &a, &a, &a, timeout);

   you are nearly guaranteed to have a bug: this call can only
   be correct if the only value you intend to inspect is "ret".

   (That, in fact, is what devd was doing: if ret==0, daemonize,
   and in any case throw away the returned results in the by-pointer
   arguments.  Hence not a bug -- unless ... well, read on.)

   The remaining question is: can even that be correct?  In
   particular, should the kernel be required to use copy-in/copy-out
   semantics, or can it do the equivalent (minus error checking
   and a lot of other important details) of:

        int select(int nfds, fd_set *in, fd_set *out, fd_set *ex,
            struct timeval *tvp) {
                some_complicated_type v_in, v_out, v_ex;

                v1 = op(*in);
                *in = function(v1);
                v2 = op(*out);
                ...
        }

   ?  If the latter is *permitted*, then the call passing "&a"
   three times is *always* wrong, since *in might have been
   clobbered before *out is ever loaded.  If a program breaks
   because it was doing this, well, it was already broken, we were
   just lucky that it worked anyway.  (Was this good luck, or bad
   luck? :-) )

   Currently, the kernel does in fact copy them all in,
   then futz with them, then copy them all out (for good reasons).
   So the devd usage always works.  But will the kernel *always*
   do this, or is that an accident of the implementation?

   If the kernel *is* always going to do this, the documentation
   should say so, at which point the POSIX requirement for the
   restrict on the parameter is itself a bug (as far as BSD is
   concerned).  That would call for feature test macros around
   the declaration.

So, if we declare that select(nfds, &a, &a, &a, timeout) *is*
legal and meaningful in BSD, *don't* just put in the restrict:
use feature test macros as appropriate to handle any conformance
issues.  But please also update select(2) to call out when this
is OK (e.g., in devd's particular use case).

Chris
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Gary Jennejohn-6
On Tue, 27 Feb 2018 14:30:24 -0800 (PST)
Chris Torek <[hidden email]> wrote:

> If we can back up a bit, there are (at least) two mostly-separate
> issues:
>
>  * What types and qualfiiers to use for various parameters:
>    These are dictated by standards, and we should just conform
>    to whatever is in the standard unless there's some major
>    reason to do otherwise.
>
>    POSIX says that we *should* use "restrict" here:
>    http://pubs.opengroup.org/onlinepubs/009696699/functions/pselect.html
>
>    so if no one has a major reason to do otherwise (such as
>    "no conforming program can tell that we didn't and all it will
>    achieve is nothing" :-) ) I'd say add the restrict.
>
>    (As kib and others have noted, it achieves a lot of nothing, so
>    the priority for this particular conformance seems low.)
>
>  * Select itself: it makes no sense *in the caller* to pass
>    the same &fd_set argument to more than one parameter unless
>    they are all input-only.  The kernel is going to write on them,
>    and there is no guarantee that the kernel will write them in any
>    particular order.  Hence if you write:
>
>         ret = select(nfds, &a, &a, &a, timeout);
>
>    you are nearly guaranteed to have a bug: this call can only
>    be correct if the only value you intend to inspect is "ret".
>
>    (That, in fact, is what devd was doing: if ret==0, daemonize,
>    and in any case throw away the returned results in the by-pointer
>    arguments.  Hence not a bug -- unless ... well, read on.)
>
>    The remaining question is: can even that be correct?  In
>    particular, should the kernel be required to use copy-in/copy-out
>    semantics, or can it do the equivalent (minus error checking
>    and a lot of other important details) of:
>
>         int select(int nfds, fd_set *in, fd_set *out, fd_set *ex,
>             struct timeval *tvp) {
>                 some_complicated_type v_in, v_out, v_ex;
>
>                 v1 = op(*in);
>                 *in = function(v1);
>                 v2 = op(*out);
>                 ...
>         }
>
>    ?  If the latter is *permitted*, then the call passing "&a"
>    three times is *always* wrong, since *in might have been
>    clobbered before *out is ever loaded.  If a program breaks
>    because it was doing this, well, it was already broken, we were
>    just lucky that it worked anyway.  (Was this good luck, or bad
>    luck? :-) )
>
>    Currently, the kernel does in fact copy them all in,
>    then futz with them, then copy them all out (for good reasons).
>    So the devd usage always works.  But will the kernel *always*
>    do this, or is that an accident of the implementation?
>

Linux also does a copyin (copy_from_user) of each FD_SET.  Linux
also does not use restrict in select.  It would be a very bad
design decision to not copy each FD_SET separately into the
kernel.

>    If the kernel *is* always going to do this, the documentation
>    should say so, at which point the POSIX requirement for the
>    restrict on the parameter is itself a bug (as far as BSD is
>    concerned).  That would call for feature test macros around
>    the declaration.
>
> So, if we declare that select(nfds, &a, &a, &a, timeout) *is*
> legal and meaningful in BSD, *don't* just put in the restrict:
> use feature test macros as appropriate to handle any conformance
> issues.  But please also update select(2) to call out when this
> is OK (e.g., in devd's particular use case).
>

It would be good to explicitly state that it is only safe to
use pointers to a single FD_SET when only the error return is of
interest and the results in the FD_SET will be ignored.

--
Gary Jennejohn
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Marking select(2) as restrict

Dimitry Andric-4
On 28 Feb 2018, at 10:40, Gary Jennejohn <[hidden email]> wrote:

>
> On Tue, 27 Feb 2018 14:30:24 -0800 (PST)
> Chris Torek <[hidden email]> wrote:
>
>> If we can back up a bit, there are (at least) two mostly-separate
>> issues:
>>
>> * What types and qualfiiers to use for various parameters:
>>   These are dictated by standards, and we should just conform
>>   to whatever is in the standard unless there's some major
>>   reason to do otherwise.
>>
>>   POSIX says that we *should* use "restrict" here:
>>   http://pubs.opengroup.org/onlinepubs/009696699/functions/pselect.html
>>
>>   so if no one has a major reason to do otherwise (such as
>>   "no conforming program can tell that we didn't and all it will
>>   achieve is nothing" :-) ) I'd say add the restrict.
>>
>>   (As kib and others have noted, it achieves a lot of nothing, so
>>   the priority for this particular conformance seems low.)
>>
>> * Select itself: it makes no sense *in the caller* to pass
>>   the same &fd_set argument to more than one parameter unless
>>   they are all input-only.  The kernel is going to write on them,
>>   and there is no guarantee that the kernel will write them in any
>>   particular order.  Hence if you write:
>>
>>        ret = select(nfds, &a, &a, &a, timeout);
>>
>>   you are nearly guaranteed to have a bug: this call can only
>>   be correct if the only value you intend to inspect is "ret".
>>
>>   (That, in fact, is what devd was doing: if ret==0, daemonize,
>>   and in any case throw away the returned results in the by-pointer
>>   arguments.  Hence not a bug -- unless ... well, read on.)
>>
>>   The remaining question is: can even that be correct?  In
>>   particular, should the kernel be required to use copy-in/copy-out
>>   semantics, or can it do the equivalent (minus error checking
>>   and a lot of other important details) of:
>>
>>        int select(int nfds, fd_set *in, fd_set *out, fd_set *ex,
>>            struct timeval *tvp) {
>>                some_complicated_type v_in, v_out, v_ex;
>>
>>                v1 = op(*in);
>>                *in = function(v1);
>>                v2 = op(*out);
>>                ...
>>        }
>>
>>   ?  If the latter is *permitted*, then the call passing "&a"
>>   three times is *always* wrong, since *in might have been
>>   clobbered before *out is ever loaded.  If a program breaks
>>   because it was doing this, well, it was already broken, we were
>>   just lucky that it worked anyway.  (Was this good luck, or bad
>>   luck? :-) )
>>
>>   Currently, the kernel does in fact copy them all in,
>>   then futz with them, then copy them all out (for good reasons).
>>   So the devd usage always works.  But will the kernel *always*
>>   do this, or is that an accident of the implementation?
>>
>
> Linux also does a copyin (copy_from_user) of each FD_SET.  Linux
> also does not use restrict in select.
Not in the kernel, where it is effectively declared as:

asmlinkage long sys_select(int n, fd_set __user *inp, fd_set __user *outp,
                        fd_set __user *exp, struct timeval __user *tvp);

but definitely in userland, where glibc has:

extern int __select (int __nfds, fd_set *__restrict __readfds,
                     fd_set *__restrict __writefds,
                     fd_set *__restrict __exceptfds,
                     struct timeval *__restrict __timeout);

So for any normal program the entry point to select() is certainly
using restrict.


> It would be a very bad
> design decision to not copy each FD_SET separately into the
> kernel.

Indeed, Linux's sys_select allocates 6 separate bitmaps, 3 for the
incoming fd_sets, and 3 for the outgoing ones.

If the outgoing ones would all use the same pointer, it is just going
to end up calling copy_to_user() 3 times on the same area, overwriting
the previous results.  That would still be unsafe for some situations.


> It would be good to explicitly state that it is only safe to
> use pointers to a single FD_SET when only the error return is of
> interest and the results in the FD_SET will be ignored.

Agreed.

-Dimitry


signature.asc (230 bytes) Download Attachment