From owner-svn-src-all@freebsd.org Tue Oct 1 16:23:14 2019 Return-Path: Delivered-To: svn-src-all@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 35628139DA4; Tue, 1 Oct 2019 16:23:14 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: from spindle.one-eyed-alien.net (spindle.one-eyed-alien.net [199.48.129.229]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 46jPhK004Dz42c5; Tue, 1 Oct 2019 16:23:12 +0000 (UTC) (envelope-from brooks@spindle.one-eyed-alien.net) Received: by spindle.one-eyed-alien.net (Postfix, from userid 3001) id 069B83C0199; Tue, 1 Oct 2019 16:23:06 +0000 (UTC) Date: Tue, 1 Oct 2019 16:23:05 +0000 From: Brooks Davis To: Konstantin Belousov Cc: Warner Losh , Mateusz Guzik , Warner Losh , src-committers , svn-src-all , svn-src-head Subject: Re: svn commit: r352795 - head/lib/libc/sys Message-ID: <20191001162305.GM93439@spindle.one-eyed-alien.net> References: <201909271611.x8RGBl0H036116@repo.freebsd.org> <20190927184623.GM44691@kib.kiev.ua> <20190928072548.GN44691@kib.kiev.ua> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="wTWi5aaYRw9ix9vO" Content-Disposition: inline In-Reply-To: <20190928072548.GN44691@kib.kiev.ua> User-Agent: Mutt/1.9.4 (2018-02-28) X-Rspamd-Queue-Id: 46jPhK004Dz42c5 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=none (mx1.freebsd.org: domain of brooks@spindle.one-eyed-alien.net has no SPF policy when checking 199.48.129.229) smtp.mailfrom=brooks@spindle.one-eyed-alien.net X-Spamd-Result: default: False [-6.49 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; FROM_HAS_DN(0.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.20)[multipart/signed,text/plain]; DMARC_NA(0.00)[freebsd.org]; AUTH_NA(1.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; RCPT_COUNT_SEVEN(0.00)[7]; R_SPF_NA(0.00)[]; SIGNED_PGP(-2.00)[]; FORGED_SENDER(0.30)[brooks@freebsd.org,brooks@spindle.one-eyed-alien.net]; RCVD_COUNT_ZERO(0.00)[0]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; ASN(0.00)[asn:36236, ipnet:199.48.128.0/22, country:US]; FROM_NEQ_ENVFROM(0.00)[brooks@freebsd.org,brooks@spindle.one-eyed-alien.net]; IP_SCORE(-3.59)[ip: (-9.38), ipnet: 199.48.128.0/22(-4.67), asn: 36236(-3.86), country: US(-0.05)] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Oct 2019 16:23:14 -0000 --wTWi5aaYRw9ix9vO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 wrote: > >=20 > > > 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 varad= ic > > > 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 hand= le > > > > 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. > > > > >=20 > > 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. > >=20 > >=20 > > > 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. > > > > >=20 > > That's not going to happen. And there's no need. It would cause more > > heartache than it's worth. > >=20 > >=20 > > > I have no interest in handling any of this, but the change committed > > > is definitely wrong. > > > > >=20 > > I tend to agree, but the manual was/is incomplete. The arg *IS* promote= d 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). > >=20 > > However, type purity aside, that's not how things are implemented. Open= is > > expecting an int (as is openat): > >=20 > > int > > open(const char *path, int flags, ...) > > { > > va_list ap; > > int mode; > >=20 > > if ((flags & O_CREAT) !=3D 0) { > > va_start(ap, flags); > > mode =3D va_arg(ap, int); > > va_end(ap); > > } else { > > mode =3D 0; > > } > > return (((int (*)(int, const char *, int, ...)) > > __libc_interposing[INTERPOS_openat])(fd, path, flags, mode)= ); > > } > >=20 > > 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 (he= nce > > my can of worms comment). And then we're passing it again through a var= adic > > function pointer... > >=20 > > 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. >=20 > > Or do we fix our implementation to > > match the docs? For all programs that don't pass in a 'long' or a point= er, > > 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. >=20 > >=20 > > To be honest, though, quibbling over how it should be implemented aside= , I > > think we should actually do the following: > >=20 > > 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 menti= on > 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); 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 --wTWi5aaYRw9ix9vO Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEcBAEBAgAGBQJdk31pAAoJEKzQXbSebgfArsAH/A3U2SdkTPQ8c3U5S96GTGZr gu5WTaBA30L96cdc42c5HzAcUBNPb2iqFa4uo/7VVfQTr81pYg6igJKvor4Db5yw BcRgruFreBl9sL559J2W0UPsu3G5UvuX7aZajMysx295LkEIf1nMg1n2wYdlL2Kf 83ZiGG3JlsYKXoLKhxiVHy0NvcUBrGC0x8s4L3diTrr0RBgKqoC3OqITXypuG9BD /E2wKByVkhDoRB71Cb0RKuib8SGH/464fM7SPX/0c1Ydc7PBp7UByI2w+yATREH8 AznNGbdpnefxtZJbEdmxdvayLWH9Lv/Shvp6yXznf+SucehNKqM891ty7/49uY0= =PX8t -----END PGP SIGNATURE----- --wTWi5aaYRw9ix9vO--