From owner-svn-src-head@FreeBSD.ORG Thu Nov 29 23:39:29 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 06108A04; Thu, 29 Nov 2012 23:39:29 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail09.syd.optusnet.com.au (mail09.syd.optusnet.com.au [211.29.132.190]) by mx1.freebsd.org (Postfix) with ESMTP id 8BC958FC12; Thu, 29 Nov 2012 23:39:28 +0000 (UTC) Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qATNdNdZ017356 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 30 Nov 2012 10:39:24 +1100 Date: Fri, 30 Nov 2012 10:39:23 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov Subject: Re: svn commit: r243665 - head/sbin/dump In-Reply-To: <20121129205729.GN3013@kib.kiev.ua> Message-ID: <20121130100508.R1242@besplex.bde.org> References: <201211290516.qAT5GoT5098709@svn.freebsd.org> <20121129120147.GE3013@kib.kiev.ua> <20121129162412.GD29338@stack.nl> <20121129205729.GN3013@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Zr21sKHG c=1 sm=1 a=L02UnmsDQJIA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=rj3hlHQjMzkA:10 a=6I5d2MoRAAAA:8 a=mr-JSwSOzfl-SWpwChUA:9 a=CjuIK1q_8ugA:10 a=OJY-0xXhBfMA:10 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler , Jilles Tjoelker X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 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: Thu, 29 Nov 2012 23:39:29 -0000 On Thu, 29 Nov 2012, Konstantin Belousov wrote: > On Thu, Nov 29, 2012 at 05:24:12PM +0100, Jilles Tjoelker wrote: >> On Thu, Nov 29, 2012 at 02:01:47PM +0200, Konstantin Belousov wrote: >>> On Thu, Nov 29, 2012 at 05:16:50AM +0000, Eitan Adler wrote: >>>> Log: >>>> Mark non-returning function as such >> >>>> PR: bin/172978 >>>> Approved by: cperciva >>>> MFC after: 3 days >> >>>> Modified: >>>> head/sbin/dump/dump.h >> >>>> Modified: head/sbin/dump/dump.h >>>> ============================================================================== >>>> --- head/sbin/dump/dump.h Thu Nov 29 03:48:39 2012 (r243664) >>>> +++ head/sbin/dump/dump.h Thu Nov 29 05:16:50 2012 (r243665) >>>> @@ -121,7 +121,7 @@ void trewind(void); >>>> void writerec(char *dp, int isspcl); >>>> >>>> void Exit(int status) __dead2; >>>> -void dumpabort(int signo); >>>> +void dumpabort(int signo) __dead2; >>>> void dump_getfstab(void); >>>> >>>> char *rawname(char *cp); >> >>> What is the goal of this change ? It seems reasonable. The function is not static, so compilers can't always see that it is no-return. But I think most older uses of __dead2 for static functions are like uses of the register keyword. >> This has been discussed before in >> http://lists.freebsd.org/pipermail/freebsd-current/2011-January/022179.html >> and the rest of the thread. The result there seems that it is the least >> bad thing to add __dead2 even on static functions that do not return to >> reduce compiler/analyzer warnings and improve the generated code with >> GCC 4.2.1 and GCC 4.5. > But we are already in the shiny new world ? We should not care about so > antique and unsupported compilers any more ? > Or, at least, the commit message should have said that we do. > > I suppose that the worry about slightly unefficient code in the dump(8) would > be good April 1 subject. >> >>> It is arguably backward. There is absolutely no use to mark signal >>> handler as __dead2, and all other uses do not benefit from the >>> redundand declaration. >> >> Although most of the uses of dumpabort() are in the same file as its >> definition, GCC 4.2.1 is too dumb to detect its noreturn property >> automatically. >> >> Furthermore, some of the uses are in separate files. In particular, >> __dead2 on dumpabort() is required to mark or detect quit() as noreturn. >> >>> Also, being quite removed from the function definition, there is a >>> chance that some future modification would make the attribute a lie. >> >> Both GCC and Clang generate a warning by default if it appears possible >> for a noreturn function to return, so as long as -Werror remains enabled >> it should be safe enough. > > The annotation is very much like comment which rephrase the code and thus is > useless. But in this case, the attribute annotation does has a chance of > changing the compiler behaviour. > > IMO, such changes shall not be done, and the already accumulated nonsense > needs to be reverted. The only problem that I see is that the change obfuscates the actual bugs: - dumpabort() is a normal function that is used as a signal handler - the use of dumpabort() as a signal handler is not just a style bug, since it is very async-signal unsafe and thus cannot be used as a (non-broken) signal handler. Similar bugs were fixed in the less-critical application dd(1). Even in 4.4BSD, the signal handler summaryx() was safe (it used and still uses a combination of snprintf() and write()), and the only fix needed was to chane exit() to _exit() in the signal handler terminate(). dumpabort() does much more than exit() in some cases, so it is harder to fix. dd was presumably more correct since signals occur in normal operation so races in the signal handling would have been more obvious. I'm not happy with some other fixes for unsafe signal handlers. The ones in ping and top are still broken AFAIK. Just setting a flag in the signal and checking the in the main loop doesn't work if any syscall is restarted. In top, the read() syscall is restarted, so top is broken for signal that occur in its read() loop (e.g., for responding to the prompt for the 's' command). So SA_RESTART should be kept clear for most signals. But then every syscall has to check for EINTR. This is a burden and is not done by top, although it is probably manageable for top (there aren't any (?) syscalls other than the one in the read() loop to check). ping is more careful and keeps SA_RESTART clear, but it is still broken when the resolver library restarts. Libraries in general might not handle the EINTR's from !SA_RESTART properly, but in the resolver library the problem is sort of the opposite: at least in very old versions that use select(), EINTR is normal but the resolvier library doesn't want to return for EINTR, so it restarts at the library level and thus the main loop in ping is never reached in another way. Bruce