From owner-svn-src-head@FreeBSD.ORG Mon Apr 29 13:10:43 2013 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id C911DF8C; Mon, 29 Apr 2013 13:10:43 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail109.syd.optusnet.com.au (mail109.syd.optusnet.com.au [211.29.132.80]) by mx1.freebsd.org (Postfix) with ESMTP id 5C5201397; Mon, 29 Apr 2013 13:10:43 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail109.syd.optusnet.com.au (Postfix) with ESMTPS id A2FC2121426; Mon, 29 Apr 2013 22:43:32 +1000 (EST) Date: Mon, 29 Apr 2013 22:43:28 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler Subject: Re: svn commit: r250037 - head/bin/hostname In-Reply-To: <201304282252.r3SMqiHH009205@svn.freebsd.org> Message-ID: <20130429212227.R1043@besplex.bde.org> References: <201304282252.r3SMqiHH009205@svn.freebsd.org> 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=Tre+H0rh c=1 sm=1 a=HGXDrXWPGKwA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=S8Dq8nDLOMQA:10 a=L-e4rMQgvTWKUpiW9zgA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org 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: Mon, 29 Apr 2013 13:10:43 -0000 On Sun, 28 Apr 2013, Eitan Adler wrote: > Log: > Mark usage() __dead2 This just adds a style bug. > Modified: head/bin/hostname/hostname.c > ============================================================================== > --- head/bin/hostname/hostname.c Sun Apr 28 22:12:40 2013 (r250036) > +++ head/bin/hostname/hostname.c Sun Apr 28 22:52:43 2013 (r250037) > @@ -49,7 +49,7 @@ __FBSDID("$FreeBSD$"); > #include > #include > > -static void usage(void); > +static void usage(void) __dead2; Static functions should never be marked __dead2. The purpose of the markup is to inform the compiler that the function doesn't return. But for static functions, the compiler can see this for itself, at least with -funit-at-a-time in CFLAGS. (-funit-at-a-time is not required (doesn't exist) in C standards, but is now the default. Compilers even use it to look to closely at static functions and automatically inline them even when they are declared after they are used, as is common for usage(), provided they are only used once and -fno-inline-functions-called-once is not used. This mainly breaks debugging and profiling of non-inline functions. The breakage is largest for clang, collaterally with the bug that clang doesn't support either of these flags, so it seems to have no way to turn off this excessive inlining.) The style bug is missing in the example of using usage() in style(9). This style bug is missing in about 259 out of 284 declarations of static void usage(void) in /usr/src/*bin (.c files). Except: - bin/pkill/pkill.c has it in a worse form, with __dead2 hard-coded using __attribute(()) - usr.bin/rlogin/rlogin.c has it in a gratuitously different form, as 'static _Noreturn void usage(void);'. This is bogus since _Noreturn is a wrapper for a new C++ feature, but rlogin is an old C application; for C, it is defined as __dead2, but it isn't clear that it does the same as a normal use of __dead2 since it is used in a syntactically different way. __dead2 only exist because this syntax, which was used for the old __dead (== volatile return type instead of __attribute((__noreturn__))) didn't work. Apparently the syntax of __attribute__() has become less strict, so the more natural (old) syntax works again, and the use of the old syntax here is just a style bug and not a syntax error. I counted these style bugs by grepping for "static.*usage" (318 lines) and then filtering out most that didn't match the normal usage of usage(). There are too many gratuitous differences like taking a 'char *' arg or returning an int. Another popular style bug is a gnu-style declaration with a space after the function name. One declaration is even missing the arg type, so it isn't a prototype. Not all instances of the __dead[2] style bug are new. In 4.4BSD-Lite2, there were 17 instances of __dead in /usr/src/*bin. Only 4 of these were on static functions. Only 2 of them were on usage(), and usage() wasn't declared static in these 2. These 2 were rlogin and mount_nfs. The bug in rlogin persists in a worse form, and the bug in mount_nfs was cloned to many other mount utilities. In FreeBSD-~5.2, there were 43 instances of __dead2 in /usr/src/*bin. 30 of these were on static functions. 22 of these were on usage(). Almost half of these 22 were in mount utilities. Further statistics for -current in /usr/src/*bin: - 60 instances of __dead2 - 57 of these on static functions - at least 26 of these on static.*usage.* - some false/misleading hits for the sloppy patterns: - 1 nonsense declaration in echo/echo.c. __dead2 is placed on the function definition of errexit(), where it ican have no effect except possibly to cause a warning when the function does return. The function actually ends with exit(), so the compiler can see that it doesn't return. There is no forward prototype for this function, but it is placed before it is used, so the compiler can see that it doesn't return even without -funit-at-a-time. - same nonsense declaration in sh/arith_yacc.c:yyerror(). It returns with error() instad of exit(), so its non-returningness is given by __dead2 on error(). - same nonsense declaration in killall/killall.c:usage(). usage() is unsorted at the beginning of the file, so it doesn't need the usual forward prototype, but it has the __dead2 declaration which is not even needed for the prototype. - i2c/i2c.c:usage(). Like killall, except the unsorting is not so close to the beginning of the file. - gbde/gbde.c:usage(). Like i2c. - hastd/primary.c. Like sh (3 functions). - hastctl/hastctl.c. Like i2c. These false hits were only harder to understand because the function definitions are formatted normally, so the function name isn't on the same line as __dead2 so grepping with simple patterns doesn't show what it is. The syntax for all the nonsense declarations is 'static __dead2 \n'. This would have been a syntax error for the original implementation of __dead2. __dead2 is named as it is (with '2' meaning gcc version2 instead of 1) because simply changing the definition of __dead wouldn't have worked due to syntactical problems. The above statistics are all for .c files. __dead2 is used a further 28 times in .h files in /usr/src/*bin in -current. Now all the uses are hopefully useful. But there aren't enough of them to make much difference. System headers are hopefully more careful about this. Bruce