svn commit: r352795 - head/lib/libc/sys

[Konstantin Belousov]

On Fri, Sep 27, 2019 at 03:19:59PM -0600, Warner Losh wrote:
> On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik <mjguzik at gmail.com> wrote:
> 
> > On 9/27/19, Konstantin Belousov <kostikbel at gmail.com> wrote:
> > > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> > >> On 9/27/19, Warner Losh <imp at freebsd.org> 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 reading, is specific to Rust.