Re: svn commit: r352795 - head/lib/libc/sys
On Tue, 1 Oct 2019, Ed Maste wrote: On Tue, 1 Oct 2019 at 12:23, Brooks Davis wrote: This isn't true with CHERI and as a result I've moved the variadic argument handling (except for syscall() and __syscall()) into libc. My grep found: open, openat, fcntl, semsys, msgsys, shmsys Is that the full list? I already wrote that this is quite broken for open and fcntl in POSIX. hecking some details shows that it is more fundamentally broken than I thought: - for open(), the type of the mode argument passed by the caller is unspecified. Whatever it is, it is "taken" as type mode_t, whatevr "taking" is. Since historical mode_t has only 16 bits, it can be represented by int even on systems with 16-bit ints, so the caller can start with either mode_t or int provided mode_t is no larger than historical mode_t and ints are either larger than 16 bits or 16 bits and not too exotic (the sign bit might cause problems if not 2's complement) - for fcntl() with F_SETOWN, the type of the pid argument passed by the caller is unspecified. Whatever it is, it is "taken" as type int. Thus if pid_t is larger than int, passing all possible values of pid_t is impossible. If also PID_MAX <= INT_MAX and all values of pid_t are actually <= PID_MAX, then all possible (positive) values can be passed, but the iplementation may have to do extra work to properly break a passed __default_promotion_of(pid_t) type by "taking" it as an int. This was discussed on the POSIX list recently. IMO it is too late and not useful to change the old specification to "take" the arg as anything except int. So pid_t might as well be specified as being a signed integer type whose default promotion is int. It is currently specified as being a signed integer type (with any size or exoticness). Bruce ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On Tue, 1 Oct 2019 at 14:13, Brooks Davis wrote: > > Also ioctl. I didn't handle the *sys() ones since they are internal only. Ah, yes - I didn't notice ioctl because syscalls.master lacked a XXX or ... comment. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On Tue, Oct 01, 2019 at 01:08:27PM -0400, Ed Maste wrote: > On Tue, 1 Oct 2019 at 12:23, Brooks Davis wrote: > > > > This isn't true with CHERI and as a result I've moved the variadic > > argument handling (except for syscall() and __syscall()) into libc. > > My grep found: open, openat, fcntl, semsys, msgsys, shmsys > Is that the full list? Also ioctl. I didn't handle the *sys() ones since they are internal only. -- Brooks signature.asc Description: PGP signature
Re: svn commit: r352795 - head/lib/libc/sys
On Tue, 1 Oct 2019 at 12:23, Brooks Davis wrote: > > This isn't true with CHERI and as a result I've moved the variadic > argument handling (except for syscall() and __syscall()) into libc. My grep found: open, openat, fcntl, semsys, msgsys, shmsys Is that the full list? ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On Tue, Oct 01, 2019 at 04:23:05PM +, Brooks Davis wrote:
> On Sat, Sep 28, 2019 at 10:25:48AM +0300, Konstantin Belousov wrote:
> > On Fri, Sep 27, 2019 at 03:19:59PM -0600, Warner Losh wrote:
> > > On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik wrote:
> > >
> > > > On 9/27/19, Konstantin Belousov wrote:
> > > > > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> > > > >> On 9/27/19, Warner Losh wrote:
> > > > >> > Document varadic args as int, since you can't have short varadic
> > > > args
> > > > >> > (they are
> > > > >> > promoted to ints).
> > > > >> >
> > > > >> > - `mode_t` is `uint16_t` (`sys/sys/_types.h`)
> > > > >> > - `openat` takes variadic args
> > > > >> > - variadic args cannot be 16-bit, and indeed the code uses int
> > > > >> > - the manpage currently kinda implies the argument is 16-bit by
> > > > >> > saying
> > > > >> > `mode_t`
> > > > >> >
> > > > >> But opengroup says it is mode_t. Perhaps it is mode_t which needs
> > > > >> to be changed?
> > > > >
> > > > > Yes, users must pass mode_t, and the man page is written for users.
> > > > > Implementation needs to be aware of the implicit promotion and handle
> > > > > it accordingly.
> > > > >
> > > > > In theory, mode_t might be wider than int.
> > > > >
> > > >
> > > > So I think the change should be reverted. Whatever workaround is being
> > > > in place in rust should remain for the current codebase.
> > > >
> > >
> > > Rust needs to understand that it's not C. It's mistake was assuming it was
> > > just like C and this is a case where the languages differ because C is so
> > > quirky.
> > >
> > >
> > > > If anyone is to fixed the problem they should bump mode_t to uint32_t,
> > > > to match Linux. This is ABI breakage, I don't know how that's handled.
> > > >
> > >
> > > That's not going to happen. And there's no need. It would cause more
> > > heartache than it's worth.
> > >
> > >
> > > > I have no interest in handling any of this, but the change committed
> > > > is definitely wrong.
> > > >
> > >
> > > I tend to agree, but the manual was/is incomplete. The arg *IS* promoted
> > > to
> > > an int, per normal C rules, so that part is right and there's no
> > > type-checking against truncation or the wrong type being used as would be
> > > the case if it weren't varadic (so don't pass a long here).
> > >
> > > However, type purity aside, that's not how things are implemented. Open is
> > > expecting an int (as is openat):
> > >
> > > int
> > > open(const char *path, int flags, ...)
> > > {
> > > va_list ap;
> > > int mode;
> > >
> > > if ((flags & O_CREAT) != 0) {
> > > va_start(ap, flags);
> > > mode = va_arg(ap, int);
> > > va_end(ap);
> > > } else {
> > > mode = 0;
> > > }
> > > return (((int (*)(int, const char *, int, ...))
> > > __libc_interposing[INTERPOS_openat])(fd, path, flags, mode));
> > > }
> > >
> > > so the change, from that perspective, actually documents the interface (so
> > > isn't definitely wrong, and my guarded 'tend to agree'). So if you did
> > > change the type of mode_t, the above code might be wrong afterwards (hence
> > > my can of worms comment). And then we're passing it again through a
> > > varadic
> > > function pointer...
> > >
> > > So while POSIX says one thing, we implement something else. Should we
> > > document POSIX or what we implement?
> > I do not see how did you come to this conclusion.
> >
> > > Or do we fix our implementation to
> > > match the docs? For all programs that don't pass in a 'long' or a pointer,
> > > the difference is zero, however.
> > ... on all supported architectures. On 32bit it actually does not matter
> > even
> > for long or pointers. But this is irrelevant, because correct programs
> > must only pass mode_t as the third arg, and then our libc does the right
> > thing on all currently supported platforms. More, I do not expect that
> > this fragment would need any revisions for future architectures.
> >
> > >
> > > To be honest, though, quibbling over how it should be implemented aside, I
> > > think we should actually do the following:
> > >
> > > diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2
> > > index a771461e2e49..aa912b797f74 100644
> > > --- a/lib/libc/sys/open.2
> > > +++ b/lib/libc/sys/open.2
> > > @@ -61,7 +61,7 @@ In this case
> > > and
> > > .Fn openat
> > > require an additional argument
> > > -.Fa "int mode" ,
> > > +.Fa "mode_t mode" ,
> > > and the file is created with mode
> > > .Fa mode
> > > as described in
> > > @@ -615,3 +615,8 @@ permits searches.
> > > The present implementation of the
> > > .Fa openat
> > > checks the current permissions of directory instead.
> > > +.Pp
> > > +The
> > > +.Fa mode
> > > +argument is varadic and may result in different calling conventions
> > > +than might otherwise be expected.
> > I do not see how this could
Re: svn commit: r352795 - head/lib/libc/sys
On Sat, Sep 28, 2019 at 10:25:48AM +0300, Konstantin Belousov wrote:
> On Fri, Sep 27, 2019 at 03:19:59PM -0600, Warner Losh wrote:
> > On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik wrote:
> >
> > > On 9/27/19, Konstantin Belousov wrote:
> > > > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> > > >> On 9/27/19, Warner Losh wrote:
> > > >> > Document varadic args as int, since you can't have short varadic
> > > args
> > > >> > (they are
> > > >> > promoted to ints).
> > > >> >
> > > >> > - `mode_t` is `uint16_t` (`sys/sys/_types.h`)
> > > >> > - `openat` takes variadic args
> > > >> > - variadic args cannot be 16-bit, and indeed the code uses int
> > > >> > - the manpage currently kinda implies the argument is 16-bit by
> > > >> > saying
> > > >> > `mode_t`
> > > >> >
> > > >> But opengroup says it is mode_t. Perhaps it is mode_t which needs
> > > >> to be changed?
> > > >
> > > > Yes, users must pass mode_t, and the man page is written for users.
> > > > Implementation needs to be aware of the implicit promotion and handle
> > > > it accordingly.
> > > >
> > > > In theory, mode_t might be wider than int.
> > > >
> > >
> > > So I think the change should be reverted. Whatever workaround is being
> > > in place in rust should remain for the current codebase.
> > >
> >
> > Rust needs to understand that it's not C. It's mistake was assuming it was
> > just like C and this is a case where the languages differ because C is so
> > quirky.
> >
> >
> > > If anyone is to fixed the problem they should bump mode_t to uint32_t,
> > > to match Linux. This is ABI breakage, I don't know how that's handled.
> > >
> >
> > That's not going to happen. And there's no need. It would cause more
> > heartache than it's worth.
> >
> >
> > > I have no interest in handling any of this, but the change committed
> > > is definitely wrong.
> > >
> >
> > I tend to agree, but the manual was/is incomplete. The arg *IS* promoted to
> > an int, per normal C rules, so that part is right and there's no
> > type-checking against truncation or the wrong type being used as would be
> > the case if it weren't varadic (so don't pass a long here).
> >
> > However, type purity aside, that's not how things are implemented. Open is
> > expecting an int (as is openat):
> >
> > int
> > open(const char *path, int flags, ...)
> > {
> > va_list ap;
> > int mode;
> >
> > if ((flags & O_CREAT) != 0) {
> > va_start(ap, flags);
> > mode = va_arg(ap, int);
> > va_end(ap);
> > } else {
> > mode = 0;
> > }
> > return (((int (*)(int, const char *, int, ...))
> > __libc_interposing[INTERPOS_openat])(fd, path, flags, mode));
> > }
> >
> > so the change, from that perspective, actually documents the interface (so
> > isn't definitely wrong, and my guarded 'tend to agree'). So if you did
> > change the type of mode_t, the above code might be wrong afterwards (hence
> > my can of worms comment). And then we're passing it again through a varadic
> > function pointer...
> >
> > So while POSIX says one thing, we implement something else. Should we
> > document POSIX or what we implement?
> I do not see how did you come to this conclusion.
>
> > Or do we fix our implementation to
> > match the docs? For all programs that don't pass in a 'long' or a pointer,
> > the difference is zero, however.
> ... on all supported architectures. On 32bit it actually does not matter even
> for long or pointers. But this is irrelevant, because correct programs
> must only pass mode_t as the third arg, and then our libc does the right
> thing on all currently supported platforms. More, I do not expect that
> this fragment would need any revisions for future architectures.
>
> >
> > To be honest, though, quibbling over how it should be implemented aside, I
> > think we should actually do the following:
> >
> > diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2
> > index a771461e2e49..aa912b797f74 100644
> > --- a/lib/libc/sys/open.2
> > +++ b/lib/libc/sys/open.2
> > @@ -61,7 +61,7 @@ In this case
> > and
> > .Fn openat
> > require an additional argument
> > -.Fa "int mode" ,
> > +.Fa "mode_t mode" ,
> > and the file is created with mode
> > .Fa mode
> > as described in
> > @@ -615,3 +615,8 @@ permits searches.
> > The present implementation of the
> > .Fa openat
> > checks the current permissions of directory instead.
> > +.Pp
> > +The
> > +.Fa mode
> > +argument is varadic and may result in different calling conventions
> > +than might otherwise be expected.
> I do not see how this could be useful for a user trying to call open(2).
> I think it would be much easier to understand and use if you simply mention
> that 'on all supported arches, mode_t is promoted to int by C rules for
> implicit conversions of arguments for variadic functions'. And perhaps
> put it somewhere else, not in the BUGS section.
I
Re: svn commit: r352795 - head/lib/libc/sys
On Fri, Sep 27, 2019 at 03:19:59PM -0600, Warner Losh wrote:
> On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik wrote:
>
> > On 9/27/19, Konstantin Belousov wrote:
> > > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> > >> On 9/27/19, Warner Losh wrote:
> > >> > Document varadic args as int, since you can't have short varadic
> > args
> > >> > (they are
> > >> > promoted to ints).
> > >> >
> > >> > - `mode_t` is `uint16_t` (`sys/sys/_types.h`)
> > >> > - `openat` takes variadic args
> > >> > - variadic args cannot be 16-bit, and indeed the code uses int
> > >> > - the manpage currently kinda implies the argument is 16-bit by
> > >> > saying
> > >> > `mode_t`
> > >> >
> > >> But opengroup says it is mode_t. Perhaps it is mode_t which needs
> > >> to be changed?
> > >
> > > Yes, users must pass mode_t, and the man page is written for users.
> > > Implementation needs to be aware of the implicit promotion and handle
> > > it accordingly.
> > >
> > > In theory, mode_t might be wider than int.
> > >
> >
> > So I think the change should be reverted. Whatever workaround is being
> > in place in rust should remain for the current codebase.
> >
>
> Rust needs to understand that it's not C. It's mistake was assuming it was
> just like C and this is a case where the languages differ because C is so
> quirky.
>
>
> > If anyone is to fixed the problem they should bump mode_t to uint32_t,
> > to match Linux. This is ABI breakage, I don't know how that's handled.
> >
>
> That's not going to happen. And there's no need. It would cause more
> heartache than it's worth.
>
>
> > I have no interest in handling any of this, but the change committed
> > is definitely wrong.
> >
>
> I tend to agree, but the manual was/is incomplete. The arg *IS* promoted to
> an int, per normal C rules, so that part is right and there's no
> type-checking against truncation or the wrong type being used as would be
> the case if it weren't varadic (so don't pass a long here).
>
> However, type purity aside, that's not how things are implemented. Open is
> expecting an int (as is openat):
>
> int
> open(const char *path, int flags, ...)
> {
> va_list ap;
> int mode;
>
> if ((flags & O_CREAT) != 0) {
> va_start(ap, flags);
> mode = va_arg(ap, int);
> va_end(ap);
> } else {
> mode = 0;
> }
> return (((int (*)(int, const char *, int, ...))
> __libc_interposing[INTERPOS_openat])(fd, path, flags, mode));
> }
>
> so the change, from that perspective, actually documents the interface (so
> isn't definitely wrong, and my guarded 'tend to agree'). So if you did
> change the type of mode_t, the above code might be wrong afterwards (hence
> my can of worms comment). And then we're passing it again through a varadic
> function pointer...
>
> So while POSIX says one thing, we implement something else. Should we
> document POSIX or what we implement?
I do not see how did you come to this conclusion.
> Or do we fix our implementation to
> match the docs? For all programs that don't pass in a 'long' or a pointer,
> the difference is zero, however.
... on all supported architectures. On 32bit it actually does not matter even
for long or pointers. But this is irrelevant, because correct programs
must only pass mode_t as the third arg, and then our libc does the right
thing on all currently supported platforms. More, I do not expect that
this fragment would need any revisions for future architectures.
>
> To be honest, though, quibbling over how it should be implemented aside, I
> think we should actually do the following:
>
> diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2
> index a771461e2e49..aa912b797f74 100644
> --- a/lib/libc/sys/open.2
> +++ b/lib/libc/sys/open.2
> @@ -61,7 +61,7 @@ In this case
> and
> .Fn openat
> require an additional argument
> -.Fa "int mode" ,
> +.Fa "mode_t mode" ,
> and the file is created with mode
> .Fa mode
> as described in
> @@ -615,3 +615,8 @@ permits searches.
> The present implementation of the
> .Fa openat
> checks the current permissions of directory instead.
> +.Pp
> +The
> +.Fa mode
> +argument is varadic and may result in different calling conventions
> +than might otherwise be expected.
I do not see how this could be useful for a user trying to call open(2).
I think it would be much easier to understand and use if you simply mention
that 'on all supported arches, mode_t is promoted to int by C rules for
implicit conversions of arguments for variadic functions'. And perhaps
put it somewhere else, not in the BUGS section.
>
> Is what I was thinking of committing instead. It's in the BUGS section, and
> is useful to know if you are debugging code that has this in the call path
> (since values may be on the stack instead of in registers, depending on the
> calling convention for the underlying architecture).
The Rust issue, from my
Re: svn commit: r352795 - head/lib/libc/sys
On 9/27/19, Warner Losh wrote:
> On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik wrote:
>
>> On 9/27/19, Konstantin Belousov wrote:
>> > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
>> >> On 9/27/19, Warner Losh wrote:
>> >> > Document varadic args as int, since you can't have short varadic
>> args
>> >> > (they are
>> >> > promoted to ints).
>> >> >
>> >> > - `mode_t` is `uint16_t` (`sys/sys/_types.h`)
>> >> > - `openat` takes variadic args
>> >> > - variadic args cannot be 16-bit, and indeed the code uses int
>> >> > - the manpage currently kinda implies the argument is 16-bit by
>> >> > saying
>> >> > `mode_t`
>> >> >
>> >> But opengroup says it is mode_t. Perhaps it is mode_t which needs
>> >> to be changed?
>> >
>> > Yes, users must pass mode_t, and the man page is written for users.
>> > Implementation needs to be aware of the implicit promotion and handle
>> > it accordingly.
>> >
>> > In theory, mode_t might be wider than int.
>> >
>>
>> So I think the change should be reverted. Whatever workaround is being
>> in place in rust should remain for the current codebase.
>>
>
> Rust needs to understand that it's not C. It's mistake was assuming it was
> just like C and this is a case where the languages differ because C is so
> quirky.
>
>
>> If anyone is to fixed the problem they should bump mode_t to uint32_t,
>> to match Linux. This is ABI breakage, I don't know how that's handled.
>>
>
> That's not going to happen. And there's no need. It would cause more
> heartache than it's worth.
>
>
In isolation, sure. Someone(tm) should do a type comprehensive type
check against Linux. There are probably many cases where something
has a different size, but software hardcodes what happens to work on
Linux (instead of using the type documented by opengroup or whatever
else is applicable).
>> I have no interest in handling any of this, but the change committed
>> is definitely wrong.
>>
>
> I tend to agree, but the manual was/is incomplete. The arg *IS* promoted to
> an int, per normal C rules, so that part is right and there's no
> type-checking against truncation or the wrong type being used as would be
> the case if it weren't varadic (so don't pass a long here).
>
But the fact there is any need for promotion in the first place is only
an implementation wart.
> However, type purity aside, that's not how things are implemented. Open is
> expecting an int (as is openat):
>
> int
> open(const char *path, int flags, ...)
> {
> va_list ap;
> int mode;
>
> if ((flags & O_CREAT) != 0) {
> va_start(ap, flags);
> mode = va_arg(ap, int);
> va_end(ap);
> } else {
> mode = 0;
> }
> return (((int (*)(int, const char *, int, ...))
> __libc_interposing[INTERPOS_openat])(fd, path, flags, mode));
> }
>
> so the change, from that perspective, actually documents the interface (so
> isn't definitely wrong, and my guarded 'tend to agree'). So if you did
> change the type of mode_t, the above code might be wrong afterwards (hence
> my can of worms comment). And then we're passing it again through a varadic
> function pointer...
>
> So while POSIX says one thing, we implement something else. Should we
> document POSIX or what we implement? Or do we fix our implementation to
> match the docs? For all programs that don't pass in a 'long' or a pointer,
> the difference is zero, however.
>
> To be honest, though, quibbling over how it should be implemented aside, I
> think we should actually do the following:
>
> diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2
> index a771461e2e49..aa912b797f74 100644
> --- a/lib/libc/sys/open.2
> +++ b/lib/libc/sys/open.2
> @@ -61,7 +61,7 @@ In this case
> and
> .Fn openat
> require an additional argument
> -.Fa "int mode" ,
> +.Fa "mode_t mode" ,
> and the file is created with mode
> .Fa mode
> as described in
> @@ -615,3 +615,8 @@ permits searches.
> The present implementation of the
> .Fa openat
> checks the current permissions of directory instead.
> +.Pp
> +The
> +.Fa mode
> +argument is varadic and may result in different calling conventions
> +than might otherwise be expected.
>
> Is what I was thinking of committing instead. It's in the BUGS section, and
> is useful to know if you are debugging code that has this in the call path
> (since values may be on the stack instead of in registers, depending on the
> calling convention for the underlying architecture).
>
I think this is fine. I mostly object to telling people to pass int instead of
mode_t.
--
Mateusz Guzik
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik wrote:
> On 9/27/19, Konstantin Belousov wrote:
> > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> >> On 9/27/19, Warner Losh wrote:
> >> > Document varadic args as int, since you can't have short varadic
> args
> >> > (they are
> >> > promoted to ints).
> >> >
> >> > - `mode_t` is `uint16_t` (`sys/sys/_types.h`)
> >> > - `openat` takes variadic args
> >> > - variadic args cannot be 16-bit, and indeed the code uses int
> >> > - the manpage currently kinda implies the argument is 16-bit by
> >> > saying
> >> > `mode_t`
> >> >
> >> But opengroup says it is mode_t. Perhaps it is mode_t which needs
> >> to be changed?
> >
> > Yes, users must pass mode_t, and the man page is written for users.
> > Implementation needs to be aware of the implicit promotion and handle
> > it accordingly.
> >
> > In theory, mode_t might be wider than int.
> >
>
> So I think the change should be reverted. Whatever workaround is being
> in place in rust should remain for the current codebase.
>
Rust needs to understand that it's not C. It's mistake was assuming it was
just like C and this is a case where the languages differ because C is so
quirky.
> If anyone is to fixed the problem they should bump mode_t to uint32_t,
> to match Linux. This is ABI breakage, I don't know how that's handled.
>
That's not going to happen. And there's no need. It would cause more
heartache than it's worth.
> I have no interest in handling any of this, but the change committed
> is definitely wrong.
>
I tend to agree, but the manual was/is incomplete. The arg *IS* promoted to
an int, per normal C rules, so that part is right and there's no
type-checking against truncation or the wrong type being used as would be
the case if it weren't varadic (so don't pass a long here).
However, type purity aside, that's not how things are implemented. Open is
expecting an int (as is openat):
int
open(const char *path, int flags, ...)
{
va_list ap;
int mode;
if ((flags & O_CREAT) != 0) {
va_start(ap, flags);
mode = va_arg(ap, int);
va_end(ap);
} else {
mode = 0;
}
return (((int (*)(int, const char *, int, ...))
__libc_interposing[INTERPOS_openat])(fd, path, flags, mode));
}
so the change, from that perspective, actually documents the interface (so
isn't definitely wrong, and my guarded 'tend to agree'). So if you did
change the type of mode_t, the above code might be wrong afterwards (hence
my can of worms comment). And then we're passing it again through a varadic
function pointer...
So while POSIX says one thing, we implement something else. Should we
document POSIX or what we implement? Or do we fix our implementation to
match the docs? For all programs that don't pass in a 'long' or a pointer,
the difference is zero, however.
To be honest, though, quibbling over how it should be implemented aside, I
think we should actually do the following:
diff --git a/lib/libc/sys/open.2 b/lib/libc/sys/open.2
index a771461e2e49..aa912b797f74 100644
--- a/lib/libc/sys/open.2
+++ b/lib/libc/sys/open.2
@@ -61,7 +61,7 @@ In this case
and
.Fn openat
require an additional argument
-.Fa "int mode" ,
+.Fa "mode_t mode" ,
and the file is created with mode
.Fa mode
as described in
@@ -615,3 +615,8 @@ permits searches.
The present implementation of the
.Fa openat
checks the current permissions of directory instead.
+.Pp
+The
+.Fa mode
+argument is varadic and may result in different calling conventions
+than might otherwise be expected.
Is what I was thinking of committing instead. It's in the BUGS section, and
is useful to know if you are debugging code that has this in the call path
(since values may be on the stack instead of in registers, depending on the
calling convention for the underlying architecture).
Warner
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On 9/27/19, Konstantin Belousov wrote: > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote: >> On 9/27/19, Warner Losh wrote: >> > Document varadic args as int, since you can't have short varadic args >> > (they are >> > promoted to ints). >> > >> > - `mode_t` is `uint16_t` (`sys/sys/_types.h`) >> > - `openat` takes variadic args >> > - variadic args cannot be 16-bit, and indeed the code uses int >> > - the manpage currently kinda implies the argument is 16-bit by >> > saying >> > `mode_t` >> > >> But opengroup says it is mode_t. Perhaps it is mode_t which needs >> to be changed? > > Yes, users must pass mode_t, and the man page is written for users. > Implementation needs to be aware of the implicit promotion and handle > it accordingly. > > In theory, mode_t might be wider than int. > So I think the change should be reverted. Whatever workaround is being in place in rust should remain for the current codebase. If anyone is to fixed the problem they should bump mode_t to uint32_t, to match Linux. This is ABI breakage, I don't know how that's handled. I have no interest in handling any of this, but the change committed is definitely wrong. -- Mateusz Guzik ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On Fri, 27 Sep 2019, Konstantin Belousov wrote: On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote: On 9/27/19, Warner Losh wrote: Author: imp Date: Fri Sep 27 16:11:47 2019 New Revision: 352795 URL: https://svnweb.freebsd.org/changeset/base/352795 Log: Document varadic args as int, since you can't have short varadic args (they are promoted to ints). - `mode_t` is `uint16_t` (`sys/sys/_types.h`) - `openat` takes variadic args - variadic args cannot be 16-bit, and indeed the code uses int - the manpage currently kinda implies the argument is 16-bit by saying `mode_t` Prompted by Rust things: https://github.com/tailhook/openat/issues/21 Submitted by: Greg V at unrelenting Differential Revision: https://reviews.freebsd.org/D21816 Modified: head/lib/libc/sys/mq_open.2 head/lib/libc/sys/open.2 Modified: head/lib/libc/sys/mq_open.2 == --- head/lib/libc/sys/mq_open.2 Fri Sep 27 15:28:30 2019(r352794) +++ head/lib/libc/sys/mq_open.2 Fri Sep 27 16:11:47 2019(r352795) @@ -133,7 +133,7 @@ Create a message queue. It requires two additional arguments: .Fa mode , which is of type -.Vt mode_t , +.Vt int , and .Fa attr , which is a pointer to an Modified: head/lib/libc/sys/open.2 == --- head/lib/libc/sys/open.2Fri Sep 27 15:28:30 2019(r352794) +++ head/lib/libc/sys/open.2Fri Sep 27 16:11:47 2019(r352795) @@ -61,7 +61,7 @@ In this case and .Fn openat require an additional argument -.Fa "mode_t mode" , +.Fa "int mode" , and the file is created with mode .Fa mode as described in But opengroup says it is mode_t. Perhaps it is mode_t which needs to be changed? POSIX needed to be changed before it became standard in 1988, but it is too late now. Types shorter than int shouldn't be used in APIs since they cause ABI and API problems. Especially in 1988 when non-prototyped functions were common. Non-prototyped functions use the default promotions much like variadic functions. open() is variadic, so its mode_t arg is always promoted, but 'int chmod(const char *path, mode_t mode)' is just wrong since in 1988 prototypes were not required and the prototype matching the K API was 'int chmod(const char *path, __default_promotion_of(mode_t) mode)'. Yes, users must pass mode_t, and the man page is written for users. Implementation needs to be aware of the implicit promotion and handle it accordingly. In theory, mode_t might be wider than int. Indeed. mode_t can also be the same size as int, but unsigned. This happens naturally if int is 16 bits, which POSIX allowed before 2001. In V7, mode_t was really a 16-bit u_int, but it was type-punned to int. When mode_t is u_int or wider than int, then the promotion is null, and variadic and K do it consistently (but the arg type before the call must be precisely mode_t, not int). The POSIX list recently discussed a related problem with variadic pid_t args for fcntl(). For F_SETOWN, the original arg type should be pid_t as for mode_t for open(). Howver, F_GETOWN only returns int, so pids larger than INT_MAX cannot work and making pid_t different from int is just foot shooting. Bruce ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote: > On 9/27/19, Warner Losh wrote: > > Author: imp > > Date: Fri Sep 27 16:11:47 2019 > > New Revision: 352795 > > URL: https://svnweb.freebsd.org/changeset/base/352795 > > > > Log: > > Document varadic args as int, since you can't have short varadic args > > (they are > > promoted to ints). > > > > - `mode_t` is `uint16_t` (`sys/sys/_types.h`) > > - `openat` takes variadic args > > - variadic args cannot be 16-bit, and indeed the code uses int > > - the manpage currently kinda implies the argument is 16-bit by saying > > `mode_t` > > > > Prompted by Rust things: https://github.com/tailhook/openat/issues/21 > > Submitted by: Greg V at unrelenting > > Differential Revision: https://reviews.freebsd.org/D21816 > > > > Modified: > > head/lib/libc/sys/mq_open.2 > > head/lib/libc/sys/open.2 > > > > Modified: head/lib/libc/sys/mq_open.2 > > == > > --- head/lib/libc/sys/mq_open.2 Fri Sep 27 15:28:30 2019 > > (r352794) > > +++ head/lib/libc/sys/mq_open.2 Fri Sep 27 16:11:47 2019 > > (r352795) > > @@ -133,7 +133,7 @@ Create a message queue. > > It requires two additional arguments: > > .Fa mode , > > which is of type > > -.Vt mode_t , > > +.Vt int , > > and > > .Fa attr , > > which is a pointer to an > > > > Modified: head/lib/libc/sys/open.2 > > == > > --- head/lib/libc/sys/open.2Fri Sep 27 15:28:30 2019 > > (r352794) > > +++ head/lib/libc/sys/open.2Fri Sep 27 16:11:47 2019 > > (r352795) > > @@ -61,7 +61,7 @@ In this case > > and > > .Fn openat > > require an additional argument > > -.Fa "mode_t mode" , > > +.Fa "int mode" , > > and the file is created with mode > > .Fa mode > > as described in > > > > But opengroup says it is mode_t. Perhaps it is mode_t which needs > to be changed? Yes, users must pass mode_t, and the man page is written for users. Implementation needs to be aware of the implicit promotion and handle it accordingly. In theory, mode_t might be wider than int. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r352795 - head/lib/libc/sys
On 9/27/19, Warner Losh wrote: > Author: imp > Date: Fri Sep 27 16:11:47 2019 > New Revision: 352795 > URL: https://svnweb.freebsd.org/changeset/base/352795 > > Log: > Document varadic args as int, since you can't have short varadic args > (they are > promoted to ints). > > - `mode_t` is `uint16_t` (`sys/sys/_types.h`) > - `openat` takes variadic args > - variadic args cannot be 16-bit, and indeed the code uses int > - the manpage currently kinda implies the argument is 16-bit by saying > `mode_t` > > Prompted by Rust things: https://github.com/tailhook/openat/issues/21 > Submitted by: Greg V at unrelenting > Differential Revision: https://reviews.freebsd.org/D21816 > > Modified: > head/lib/libc/sys/mq_open.2 > head/lib/libc/sys/open.2 > > Modified: head/lib/libc/sys/mq_open.2 > == > --- head/lib/libc/sys/mq_open.2 Fri Sep 27 15:28:30 2019 > (r352794) > +++ head/lib/libc/sys/mq_open.2 Fri Sep 27 16:11:47 2019 > (r352795) > @@ -133,7 +133,7 @@ Create a message queue. > It requires two additional arguments: > .Fa mode , > which is of type > -.Vt mode_t , > +.Vt int , > and > .Fa attr , > which is a pointer to an > > Modified: head/lib/libc/sys/open.2 > == > --- head/lib/libc/sys/open.2 Fri Sep 27 15:28:30 2019(r352794) > +++ head/lib/libc/sys/open.2 Fri Sep 27 16:11:47 2019(r352795) > @@ -61,7 +61,7 @@ In this case > and > .Fn openat > require an additional argument > -.Fa "mode_t mode" , > +.Fa "int mode" , > and the file is created with mode > .Fa mode > as described in > But opengroup says it is mode_t. Perhaps it is mode_t which needs to be changed? -- Mateusz Guzik ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r352795 - head/lib/libc/sys
Author: imp Date: Fri Sep 27 16:11:47 2019 New Revision: 352795 URL: https://svnweb.freebsd.org/changeset/base/352795 Log: Document varadic args as int, since you can't have short varadic args (they are promoted to ints). - `mode_t` is `uint16_t` (`sys/sys/_types.h`) - `openat` takes variadic args - variadic args cannot be 16-bit, and indeed the code uses int - the manpage currently kinda implies the argument is 16-bit by saying `mode_t` Prompted by Rust things: https://github.com/tailhook/openat/issues/21 Submitted by: Greg V at unrelenting Differential Revision: https://reviews.freebsd.org/D21816 Modified: head/lib/libc/sys/mq_open.2 head/lib/libc/sys/open.2 Modified: head/lib/libc/sys/mq_open.2 == --- head/lib/libc/sys/mq_open.2 Fri Sep 27 15:28:30 2019(r352794) +++ head/lib/libc/sys/mq_open.2 Fri Sep 27 16:11:47 2019(r352795) @@ -133,7 +133,7 @@ Create a message queue. It requires two additional arguments: .Fa mode , which is of type -.Vt mode_t , +.Vt int , and .Fa attr , which is a pointer to an Modified: head/lib/libc/sys/open.2 == --- head/lib/libc/sys/open.2Fri Sep 27 15:28:30 2019(r352794) +++ head/lib/libc/sys/open.2Fri Sep 27 16:11:47 2019(r352795) @@ -61,7 +61,7 @@ In this case and .Fn openat require an additional argument -.Fa "mode_t mode" , +.Fa "int mode" , and the file is created with mode .Fa mode as described in ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
