From owner-svn-src-all@freebsd.org Mon Aug 29 17:29:20 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 63C20BC71C0; Mon, 29 Aug 2016 17:29:20 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3B21219B; Mon, 29 Aug 2016 17:29:20 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id AFEBAB9A5; Mon, 29 Aug 2016 13:29:18 -0400 (EDT) From: John Baldwin To: Konstantin Belousov Cc: Andrey Chernov , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r304928 - in head/lib/libc: amd64/sys i386/sys sys Date: Mon, 29 Aug 2016 09:51:50 -0700 Message-ID: <18206408.GMLM77D01s@ralph.baldwin.cx> User-Agent: KMail/4.14.3 (FreeBSD/10.3-STABLE; KDE/4.14.3; amd64; ; ) In-Reply-To: <20160829065813.GP83214@kib.kiev.ua> References: <201608272303.u7RN3N0D078505@repo.freebsd.org> <1595604.93PBdSz0kX@ralph.baldwin.cx> <20160829065813.GP83214@kib.kiev.ua> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 29 Aug 2016 13:29:19 -0400 (EDT) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 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: Mon, 29 Aug 2016 17:29:20 -0000 On Monday, August 29, 2016 09:58:13 AM Konstantin Belousov wrote: > On Sun, Aug 28, 2016 at 04:09:51PM -0700, John Baldwin wrote: > > OTOH, given that we explicitly documented it as not being true, I suspect > > any applications that are using ptrace() are going off the documentation, not > > the implementation artifact. Note that Linux's ptrace() documents the same > > requirement as before this change (caller is required to clear errno), so I > > doubt there is any actual software out there that expects the > > FreeBSD-specific behavior. Given that and the extra maintenance overhead of > > having to dink with errno in assembly on X architectures, I'd rather we keep > > the old language in the manpage and remove the 'errno' frobbing in the system > > call wrappers. To be honest, my first response to this commit was one of > > surprise that we modify errno directly as that is inconsistent with other > > system calls. (I haven't looked to see if any other system call wrappers > > modify errno for non-error cases.) > > The problematic calls are PT_PEEK_I and PT_PEEK_D, as far as I understand. > > I dug into the ptrace(2) consumers, I found a lot of things using > it which I would not expect to use, besides usual suspects of gdb > lldb libunwind reptyr etc. Most surprising was that even high-profile > consumers including gdb sometimes fail to check errno after PT_PEEK. On > the other hand, I did not found a case in gdb where errno is checked > after PT_PEEK but not zeroed before the syscall. So the consumers are generally doing the right thing. > I almost agreed with you after the reading, but then I decided to look > into glibc just in case. What I found there is really fascinating. > From glibc/sysdeps/unix/sysv/linux: > res = INLINE_SYSCALL (ptrace, 4, request, pid, addr, data); > if (res >= 0 && request > 0 && request < 4) > { > __set_errno (0); > return ret; > } > #define PTRACE_PEEKTEXT 1 > #define PTRACE_PEEKDATA 2 > #define PTRACE_PEEKUSR 3 > > In the end, I might consider changing the ptrace wrappers into > consolidated C source, it would look like that > > int > ptrace(int request, pid_t pid, caddr_t addr, int data) > { > > errno = 0; > return (__sys_ptrace(request, pid, addr, data)); > } Certainly I think having a C wrapper like this makes more sense than doing it all in assembly N times. I would probably prefer to keep the manpage language the way it is though. If we are really worried about compat, we could bump the ptrace symver and use the function above as the FBSD_1.0 version perhaps. -- John Baldwin