Date: Tue, 1 Oct 2019 19:33:20 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Brooks Davis <brooks@freebsd.org> Cc: Warner Losh <imp@bsdimp.com>, Mateusz Guzik <mjguzik@gmail.com>, Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r352795 - head/lib/libc/sys Message-ID: <20191001163320.GX44691@kib.kiev.ua> In-Reply-To: <20191001162305.GM93439@spindle.one-eyed-alien.net> References: <201909271611.x8RGBl0H036116@repo.freebsd.org> <CAGudoHF454hyHeS_HayGRJYRpSfo9B9wwU4hVBxjcmAk0HEeJg@mail.gmail.com> <20190927184623.GM44691@kib.kiev.ua> <CAGudoHFhMRFm_qvXHZ4MsHGtv74w0_CLdOG3QojeC=ZxDTYZvQ@mail.gmail.com> <CANCZdfonqN9oRnqbjktW1Kc-oKMbcus-cL3x2Nh=dZDDWYdMDw@mail.gmail.com> <20190928072548.GN44691@kib.kiev.ua> <20191001162305.GM93439@spindle.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Oct 01, 2019 at 04:23:05PM +0000, 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 <mjguzik@gmail.com> wrote:
> > >
> > > > On 9/27/19, Konstantin Belousov <kostikbel@gmail.com> wrote:
> > > > > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote:
> > > > >> On 9/27/19, Warner Losh <imp@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.
>
> I think this would be a good solution.
>
>
> Note that the actual ABI constraint on variadic syscalls in FreeBSD is
> that they have exactly the same calling convention as if the argument is
> explicitly declared because we have on infrastructure to handle them any
> other way. Specifically, calling a function of type:
>
> int open(const char *path, int flags, ...);
>
> with a mode_t as a variadic argument must be identical to calling a
> function of type:
>
> int open(const char *path, int flags, mode_t mode);
It is not quite true, surprisingly, on amd64. At least the ABI requires
the caller to put the number of XMM registers used for param passing
into %eax for variadic functions. I think both gcc and clang can live
without this hint in modern times.
>
> This isn't true with CHERI and as a result I've moved the variadic
> argument handling (except for syscall() and __syscall()) into libc.
>
> -- Brooks
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191001163320.GX44691>