From owner-svn-src-head@freebsd.org Fri Sep 27 21:20:12 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 B568D12D45C for ; Fri, 27 Sep 2019 21:20:12 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qk1-x72e.google.com (mail-qk1-x72e.google.com [IPv6:2607:f8b0:4864:20::72e]) (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 46g4Sq6lrDz4LPc for ; Fri, 27 Sep 2019 21:20:11 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qk1-x72e.google.com with SMTP id u186so3157169qkc.5 for ; Fri, 27 Sep 2019 14:20:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RkF39S2AyqlOQmS/S+c1RcnLVvc/XQRI2oQeXJudGGw=; b=qlkojCDM7gLyoQptYMj6+7akWw7GhNahjHtp6GVx1Mq1vREGF7nvok2do9j6j/7j/z 8fCmiM6P77v5NEAEjZy6Df/sCstXoPekqPOvpKVI1+LPTfxCW60mElqPwzPgH5JUo5QM cOt5/FHnZdsR5tRZG91sBA3XwK0y6HlWCidwMX+IYBvOE859zLFcfNwkfoBEFJ6NK/p0 s0uPDytJ/2esxI9yqeIMLzqMA2Hdb21sUjAPQsvovkh1165E2/QkQq6l7bzcw0ysvcGu 1cmGgsvHfD16jy5YKJAvRVjH1cWtSaQWLkj7MmNlG2bdsIpt6RzE0PP3626/CeJd3r3L upmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RkF39S2AyqlOQmS/S+c1RcnLVvc/XQRI2oQeXJudGGw=; b=HQcKN4hZWr8pYgHY6zqdHAZolUhbB+67CSlWeN0u21i1VLtOAk70CK/LoEz9ws9M0f m8N6MP1OSUhGNGHzKttAbw1jRVh47SQ/IvCBEX0XnG8CQ7J3WaKZdqeJALxSSwvzquod iwy66QKMUSGKlyppF93bnK1VRHzm5Wj9HSJTGKhLreNVUEK2l4gkE2+g+DrWk+QulEM6 wAzVp+6c6OApgSSsuAkaXKazitzT/9GKxp83353W7ySyFBJkvH1n2oM6hHeiPnG0QE6s 0WwwbUjx23tQZggoz2SxvimCLqWi9vpB36OzD5OvtHgygPRJ3dPB6gIhrPkBY4SLwydn ppfQ== X-Gm-Message-State: APjAAAXtADfKJJ+A4roI2NtH04ZDKYDqMPyJ3hnpaX5DCkPaWyzsHNWi lEyY9DF19fn2xtNAo7g1cSX9yA6/1hr3FEfuJbfLzQ== X-Google-Smtp-Source: APXvYqz/ub+j7bHCibvg5bDMPDfE+WnonUaNxT6k7ONJj0xbPbsGYoz/DISv3Du74xCPALegcpTLmQQlWfGa7hAviV4= X-Received: by 2002:a37:6787:: with SMTP id b129mr7229402qkc.60.1569619210599; Fri, 27 Sep 2019 14:20:10 -0700 (PDT) MIME-Version: 1.0 References: <201909271611.x8RGBl0H036116@repo.freebsd.org> <20190927184623.GM44691@kib.kiev.ua> In-Reply-To: From: Warner Losh Date: Fri, 27 Sep 2019 15:19:59 -0600 Message-ID: Subject: Re: svn commit: r352795 - head/lib/libc/sys To: Mateusz Guzik Cc: Konstantin Belousov , Warner Losh , src-committers , svn-src-all , svn-src-head X-Rspamd-Queue-Id: 46g4Sq6lrDz4LPc X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=bsdimp-com.20150623.gappssmtp.com header.s=20150623 header.b=qlkojCDM; dmarc=none; spf=none (mx1.freebsd.org: domain of wlosh@bsdimp.com has no SPF policy when checking 2607:f8b0:4864:20::72e) smtp.mailfrom=wlosh@bsdimp.com X-Spamd-Result: default: False [-4.83 / 15.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com:s=20150623]; FROM_HAS_DN(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-head@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; RCPT_COUNT_FIVE(0.00)[6]; TO_MATCH_ENVRCPT_SOME(0.00)[]; TO_DN_ALL(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; RCVD_IN_DNSWL_NONE(0.00)[e.2.7.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]; R_SPF_NA(0.00)[]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; FREEMAIL_TO(0.00)[gmail.com]; MIME_TRACE(0.00)[0:+,1:+,2:~]; IP_SCORE(-2.83)[ip: (-9.32), ipnet: 2607:f8b0::/32(-2.59), asn: 15169(-2.17), country: US(-0.05)]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; RCVD_TLS_ALL(0.00)[]; FREEMAIL_CC(0.00)[gmail.com] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Fri, 27 Sep 2019 21:20:12 -0000 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. > 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