From owner-freebsd-current@FreeBSD.ORG Fri Feb 15 11:26:39 2008 Return-Path: Delivered-To: current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2AEC916A418 for ; Fri, 15 Feb 2008 11:26:39 +0000 (UTC) (envelope-from rermilov@team.vega.ru) Received: from mail.vega.ru (infra.dev.vega.ru [90.156.167.14]) by mx1.freebsd.org (Postfix) with ESMTP id 9F7D013C45B for ; Fri, 15 Feb 2008 11:26:38 +0000 (UTC) (envelope-from rermilov@team.vega.ru) Received: from [87.242.97.68] (port=53924 helo=edoofus.dev.vega.ru) by mail.vega.ru with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.68 (FreeBSD)) (envelope-from ) id 1JPyiB-0004Fz-AJ; Fri, 15 Feb 2008 11:26:35 +0000 Received: from edoofus.dev.vega.ru (localhost [127.0.0.1]) by edoofus.dev.vega.ru (8.14.2/8.14.2) with ESMTP id m1FBQSP9003221; Fri, 15 Feb 2008 14:26:28 +0300 (MSK) (envelope-from rermilov@team.vega.ru) Received: (from ru@localhost) by edoofus.dev.vega.ru (8.14.2/8.14.2/Submit) id m1FBQSxu003220; Fri, 15 Feb 2008 14:26:28 +0300 (MSK) (envelope-from rermilov@team.vega.ru) X-Authentication-Warning: edoofus.dev.vega.ru: ru set sender to rermilov@team.vega.ru using -f Date: Fri, 15 Feb 2008 14:26:28 +0300 From: Ruslan Ermilov To: Kostik Belousov Message-ID: <20080215112628.GA2047@team.vega.ru> References: <200802122009.m1CK94Y8026959@repoman.freebsd.org> <20080212200911.B49F416A51C@hub.freebsd.org> <20080212204803.GT57756@deviant.kiev.zoral.com.ua> <20080213113530.GB45243@team.vega.ru> <20080214173850.GB57756@deviant.kiev.zoral.com.ua> <20080214211744.GA80604@team.vega.ru> <20080215100243.GF57756@deviant.kiev.zoral.com.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080215100243.GF57756@deviant.kiev.zoral.com.ua> User-Agent: Mutt/1.5.17 (2007-11-01) Cc: current@freebsd.org Subject: Re: [src] cvs commit: src/include unistd.h src/lib/libc/sys readlink.2 src/sys/compat/freebsd32 syscalls.master src/sys/kern syscalls.master vfs_syscalls.c src/sys/sys syscallsubr.h X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Feb 2008 11:26:39 -0000 On Fri, Feb 15, 2008 at 12:02:43PM +0200, Kostik Belousov wrote: > On Fri, Feb 15, 2008 at 12:17:47AM +0300, Ruslan Ermilov wrote: > > On Thu, Feb 14, 2008 at 07:38:50PM +0200, Kostik Belousov wrote: > > > On Wed, Feb 13, 2008 at 02:35:30PM +0300, Ruslan Ermilov wrote: > > > > [ Replying to the list. ] > > > > > > > > On Tue, Feb 12, 2008 at 10:48:04PM +0200, Kostik Belousov wrote: > > > > > > -int readlink(const char *, char *, int); > > > > > > +ssize_t readlink(const char *, char *, size_t); > > > > > You do understand that this changes the ABI ? size_t and int have different > > > > > sizes on 64-bit arches, and now upper-half of the register used to pass > > > > > the third arg is used. Amd64, fortunately, makes very hard to load a > > > > > non-zero into the upper half, I am not so sure about IA64/sparc64. > > > > > > > > I considered that. I've tested locally on amd64 and sparc64, and > > > > ia64 on pluto2.freebsd.org. Since this is only a third argument, > > > > it's passed in a 64-bit register, and for any meaningful value of it > > > > (0 .. INT_MAX), there's no ABI change at all. I compared .s files. > > > > > > > > : // cc -S a.c ; mv a.s a.s~ ; cc -S -DNEW a.c ; diff -u a.s~ a.s > > > > : #include > > > > : #include > > > > : > > > > : #ifdef NEW > > > > : ssize_t readlink(const char *, char *, size_t); > > > > : #else > > > > : int readlink(const char *, char *, int); > > > > : #endif > > > > : > > > > : void > > > > : foo(void) > > > > : { > > > > : int i; > > > > : char buf[1024]; > > > > : > > > > : i = readlink("foo", buf, INT_MAX); > > > > : } > > > > > > > > > This change, IMHO, requires symbol version compat shims. > > > > > > > > I don't think so. > > > > > > > > > > The slightly contrived example below works on RELENG_7 amd64, relevant > > > output from the truss is > > > readlink("/usr/X11R6","l",1) = 1 (0x1) > > > on the CURRENT gives > > > readlink("/usr/X11R6","l",1) = -4294967295 (0xffffffff00000001) > > > [also please note wrong output for the third readlink arg; ktrace/kdump works > > > ok]. > > > > > > .text > > > .globl main > > > main: movq $0xffffffff00000001, %rax > > > movq %rax, %rdx > > > movq $buf, %rax > > > movq %rax, %rsi > > > movq $path, %rax > > > movq %rax, %rdi > > > call readlink > > > xorl %edi, %edi > > > call exit > > > > > > .section .rodata > > > path: .asciz "/usr/X11R6" > > > > > > .data > > > .comm buf, 0x80 > > > > This is because uio_resid is still "int". > > > > : int > > : kern_readlink(struct thread *td, char *path, enum uio_seg pathseg, char *buf, > > : enum uio_seg bufseg, size_t count) > > [...] > > : auio.uio_resid = count; > > [...] > > : td->td_retval[0] = count - auio.uio_resid; > > > > uio_resid gets the (truncated) value of "1", VOP_READLINK() > > reads 1 char, td_retval[0] gets the value 0xffffffff00000001. > > Any meaningful value of the third argument will work OTOH. > > > The point of the conversation I started is exactly this: the domain of the > _reasonable_ values for the third arg is changed after your commit. > The value that was perfectly acceptable before the commit now causes > wrong effects. I consider this to be the ABI change. Other than crafting this with an asm or a fake prototype, how the 64-bit value can even be passed to a syscall when the type of the third argument is declared to be 32-bit "int"? In any case, the behavior isn't changed for old binaries that expect "int" as a return type -- the returned 64-bit value will be truncated to fit into 32-bit int, giving the bug-to-bug compatible behavior, so there's no ABI change. The following code, compiled statically on the old system, : // The type of the 3rd arg is changed to allow for 64-bit. : int readlink(const char *path, char *buf, size_t bufsiz); : [...] : int r; : r = readlink("/usr/X11R6", buf, 0xffffffff00000005); gives, on old system: 93309 a CALL readlink(0x41a169,0x7fffffffeb00,0xffffffff00000005) 93309 a NAMI "/usr/X11R6" 93309 a RET readlink 5 and on a new system: 28748 a CALL readlink(0x41a169,0x7fffffffea90,0xffffffff00000005) 28748 a NAMI "/usr/X11R6" 28748 a RET readlink 5/0xffffffff00000005 I.e., it's still "int 5". The only change is in the undefined upper 32 bits of the 64-bit register holding a retval. So, where exactly is the ABI change here? When a similar change was made to read() and write() in 1998, the following checks were added (to work around uio_resid still being "int"): : if (uap->nbyte > INT_MAX) : return (EINVAL); We can add a similar check to kern_readlink(). Longer term, we should probably also consider changing the type of "struct uio.uio_resid" from "int" to size_t, as has already been done by our fellow BSDs. Cheers, -- Ruslan Ermilov ru@FreeBSD.org FreeBSD committer