From owner-svn-src-head@freebsd.org Sat Sep 28 01:20:18 2019 Return-Path: Delivered-To: svn-src-head@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 11E43F2DD0; Sat, 28 Sep 2019 01:20:18 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-oi1-x241.google.com (mail-oi1-x241.google.com [IPv6:2607:f8b0:4864:20::241]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 46g9nr3G5Rz4ZdM; Sat, 28 Sep 2019 01:20:16 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-oi1-x241.google.com with SMTP id o205so6709325oib.12; Fri, 27 Sep 2019 18:20:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=3TF5zcgfSmB+0WhMc/nH1tWr3vA0c786h/0LYR8rdqc=; b=hzUjGJDOltd2X7m4/MS4qfvPEIu7xvOY9PznjTdnjYGGRPEVbyRi2Q6d4dLxJ3JhkV r89ngStTGEYu5aSBOfmleb93ERG3hHHaTtGlu4GSOg9lM+N1Oen/X+siT9V++hXtK3U5 9cSRbZAGFIEbmQ+1M87lTjjM+AUHfkkqr4bGPKkG7J9pKCgvi+yFFJHxmNLE6/quSDrX cCQjYy+jH5vbnp2vdr8BSn34Kvrgcd0t9htLpvz+EYAIfYssVrWbGgE8py5hGYwSHuJx vNumdMxiGZSX1Gr6y+zCDk78ti3y+5tiqOxXR5UjmVVR/sDax/7CLDDMjT+vSOqUxfpS 9y9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=3TF5zcgfSmB+0WhMc/nH1tWr3vA0c786h/0LYR8rdqc=; b=Egqbf5JhDpmNs+50Tg3RL0zE8iQpXQIJHNLpVsFeTeCIMgeskXWDe10SZlijf0Au1M zalu8x+7Sgdx3hApdZSwl4HSvtbyI2dNfPPuCaRTq7o4jPCtwE3h6lNT+o6VNEXDgJMM lDYKVMP6PbqyFGgiEH09YfdNtsWM/4UFT902FIyT3gznbvt/c4B52APUkwTSyUBKcMJZ MG7Y56UrrecpVbte63Z7IKu43Un+rADzNvFn22Ek0Ea7T6Xk3Po6LdKL+Mpr3KuENaAW CO87N4ismjBNx2ZNU3puvY0gqJCi11jl+zKDpyFh4aVmS4kmvhBWVKdI5v/uzdJgIzvL wJ/A== X-Gm-Message-State: APjAAAU7CnonINEVae9FtxAMdNV9WEaKGgNZUWEfd8HzlxK8wgWlhl14 6LObdp5RxKX4z2jvoYzDGZJYFbqrzGBftfMmAUNLrg== X-Google-Smtp-Source: APXvYqysHSDeWaaNVJUlqvNdffZQgHd7oqKARwO+qNCfk59rjEQ1zZE9Zy7vjIH8ncOo3N3DcEjx/CIM7nmfnh1xlVk= X-Received: by 2002:a54:4f1b:: with SMTP id e27mr3997019oiy.85.1569633614920; Fri, 27 Sep 2019 18:20:14 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a4a:458d:0:0:0:0:0 with HTTP; Fri, 27 Sep 2019 18:20:14 -0700 (PDT) In-Reply-To: References: <201909271611.x8RGBl0H036116@repo.freebsd.org> <20190927184623.GM44691@kib.kiev.ua> From: Mateusz Guzik Date: Sat, 28 Sep 2019 03:20:14 +0200 Message-ID: Subject: Re: svn commit: r352795 - head/lib/libc/sys To: Warner Losh Cc: Konstantin Belousov , Warner Losh , src-committers , svn-src-all , svn-src-head Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 46g9nr3G5Rz4ZdM X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=hzUjGJDO; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2607:f8b0:4864:20::241 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-3.00 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20161025]; FROM_HAS_DN(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com.dwl.dnswl.org : 127.0.5.0]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36]; FREEMAIL_FROM(0.00)[gmail.com]; MIME_GOOD(-0.10)[text/plain]; IP_SCORE(0.00)[ip: (2.12), ipnet: 2607:f8b0::/32(-2.59), asn: 15169(-2.17), country: US(-0.05)]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; IP_SCORE_FREEMAIL(0.00)[]; RCPT_COUNT_FIVE(0.00)[6]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; RCVD_IN_DNSWL_NONE(0.00)[1.4.2.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FREEMAIL_CC(0.00)[gmail.com]; RCVD_TLS_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Sep 2019 01:20:18 -0000 On 9/27/19, Warner Losh wrote: > On Fri, Sep 27, 2019 at 2:38 PM Mateusz Guzik wrote: > >> On 9/27/19, Konstantin Belousov wrote: >> > On Fri, Sep 27, 2019 at 08:32:20PM +0200, Mateusz Guzik wrote: >> >> On 9/27/19, Warner Losh 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