Date: Sat, 28 Sep 2019 03:20:14 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Warner Losh <imp@bsdimp.com> Cc: Konstantin Belousov <kostikbel@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: <CAGudoHFWO%2BBYyNTpzBiORSone4REjMTAT7dX3mmXrNR47BEn0A@mail.gmail.com> In-Reply-To: <CANCZdfonqN9oRnqbjktW1Kc-oKMbcus-cL3x2Nh=dZDDWYdMDw@mail.gmail.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
On 9/27/19, Warner Losh <imp@bsdimp.com> 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.
>
>
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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHFWO%2BBYyNTpzBiORSone4REjMTAT7dX3mmXrNR47BEn0A>