Date: Fri, 11 May 2018 19:42:11 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Eitan Adler <eadler@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r333482 - head/usr.bin/expand Message-ID: <20180511182952.B2874@besplex.bde.org> In-Reply-To: <201805110655.w4B6t3sH032476@repo.freebsd.org> References: <201805110655.w4B6t3sH032476@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 11 May 2018, Eitan Adler wrote: > Log: > [expand] add __dead2 annotation to usage Declaring static functions as __dead2 has no good effect, but reduces portability and is a style bug. Compilers have always been able to see inside static functions and determine if they return. Primitive compilers might not see forward functions, and now clang doesn't even support the -fno-unit-at-a-time flag which is needed for turning off excessive forwarding. __dead2 might be useful for primitive code analysis tools like lint, but lint in FreeBSD never understood any attributes, and even lint always read full files, so it only needed special help with extern functions whose source is not visible. In /usr/src/bin, in 4.4BSD, there are no instances of this style bug for static functions. Howver, 4.4BSD is not careful to declare functions as static, and it has the worse style bugs for the usage() in the following programs: - sbin/route/route.c: this declares usage() as implicit-extern and bogusly declares the function as __dead, and also helps lint by /* NOTREACH */ after the function usages exit() to not return. 4.4BSD also isn't careful about prototypes. No function in route.c has a prototype. Most functions in route.c are forward-declared, but usage() has the style bug of being placed before main() so that it doesn't need a forward declaration. So its __dead declaration has no effect even with -fno-unit-at-a-time. __dead is the gcc-1 spelling of __dead2. gcc-1 doesn't need it provided exit() is declared as __dead, which it is in 4.4BSD. - usr.bin/rlogin/rlogin.c: usage() is implicit-extern and forward-declared, and even has a prototype using __P(()). Its definition is also declared as __dead. In gcc-1, __dead isn't an attribute (since gcc-1 doesn't support attributes), but is the volatile keyword invalidly overloaded. Perhaps the function definition needs to have the same qualifiers as the forward declaration, but in general C has the opposite problem with types for functions -- function declarations can be built up from incomplete declarations to obfuscate what the combined qualifiers/types are are. - usr.bin/touch/touch.c: like rlogin except usage() is forward-declared with a prototype using __P(()) and the function definition doesn't have /* NOTREACHED */. __dead is bogusly attached to the function definition where it has no effect. That is all instances of __dead for usage() in /usr/*bin in 4.4BSD. There are only about 10 other commands that use __dead for other functions. Some of these uses are actually correct, since they are for extern functions in header files. FreeBSD has expanded this style bug only a little. In /usr/src/bin, there about 39 programs, but there are only 19 instances of __dead2 and only 3 instances of the style bug for static usage() (for cat, hostname and realpath). stty has an example of correct use of __dead2 usage() (when usage() is extern and prototyped in a header file. Of the other 15 instances, 7 are correct (for extern functions) and 8 are style bugs (for static functions). Most of the bugs are in bin/sh. Some of these instances expand the bugs by misplacing __dead2 before the function name. This is a style bug at best, and might be a syntax error for old versions of gcc. gcc-1 had to place __dead first since it was a qualifier, and this couldn't just be changed to an attribute since gcc-2 didn't allow attributes there, so __dead2 was placed at the end of the declaration. Some of these instances also add other attributes like __printflike() to forward declarations. I don't know if compilers look deeply enough into function definitions to see such attributes there. I guess that they don't for __printflike() and other complicated attributes. Even after inlining everything, it is hard to see if the inlined block implements attributes as specified by _printflike(). Parts of the block might inherit attributes by calling vfprintf(), but other parts might do things like 'switch (*fmt)' as in the implementation of vfprintf(), and understanding that requires a deep analysis. FreeBSD has expanded on this style bug by sometimes spelling __dead2 as _Noreturn and misplacing this before the function name. This is used mainly to add style bugs to header files. 3 style bugs in most places: - rename __dead2 to _Noreturn - place it before function names where it is syntactically incorrect for old compilers - misformat it there. In /usr/src/bin, there is only 1 instance of a case-sensitive noreturn. This is in pkill, where FreeBSD macros to hide unportabilities are not even used. Instead, the attribute is hard-coded as __attribute__((__noreturn__)). This is the only instance in src/bin of the style and portability bug of using hard-coded __attribute__(()). Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180511182952.B2874>