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

[Warner Losh]

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? 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