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

[Mateusz Guzik]

On 9/27/19, Warner Losh <imp at bsdimp.com> 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.
>
>

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>