Re: svn commit: r287217 - head/usr.sbin/syslogd
On Sun, 30 Aug 2015, Jilles Tjoelker wrote: On Sun, Aug 30, 2015 at 11:53:00AM +0200, Ed Schouten wrote: 2015-08-28 16:38 GMT+02:00 Joerg Sonnenberger : But the compiler can't tell if it is the *intention* that the function never returns. The warning behavior exists because that can easily change with macros etc. I think it's important to keep in mind what this keyword was designed for. The idea behind this attribute (and the C11 _Noreturn keyword) is to allow for propagation of optimisation state across compilation units. You use it to annotate functions in header files, so that the compiler does not need to handle function return at the call site. This knowledge can be automatically be inferred if the function is static. Although you are right philosophically, in practice there are some compilers and static analyzers that do benefit from noreturn attributes on static functions. In particular, gcc 4.2.1 generates better code and fewer warnings if static functions are annotated with noreturn if appropriate, and some versions of the Clang/LLVM static analyzer do not do any cross-procedural analysis at all and depend on noreturn attributes on static functions to avoid false positives. If this were important, then __dead2 would be used a lot for important functions, not for usage() and die(). It is in fact so important that __dead2 is used a whole 86 times in $(find /usr/src -name *.c): - 10 uses for non-static functions where there is actually a reason to use __dead2, but about half of these 10 are due to missing staticization - 30 uses for static usage(). All of these uses have the correct syntax with __dead2 at the end (some versions of gcc only allow attributes near the end) - 46 other uses. Mostly for functions like panic(), exit() or err(). 18 of these have syntax errors with __dead2 not at the end (or near the end, preceding other attributes). There are 10 'static __dead2 void's, 7 'static void __dead2's, and 1 '__dead2 static void'. Non-FreeBSD spellings of __dead2 are more popular. $(find /usr/src -name *.c) has 200 lines matching 'oreturn'. About 73 of these use the hard-coded gccism __attribute(()). The C11ish _Noreturn is used just 4 times (3 times in rlogin for static functions that don't need it and 1 time in libstdthreads). Outside of contrib, there are just 30 lines matching 'oreturn': 11 in crypto/heimdal, 2 in routed/rtqery, 8 in tools/regression for static usage(), 4 _Noreturns as above, 1 in xlint, 1 in pkill, 2 in comments and 1 unrelated match. In practice this means that many compiler warnings need to be disabled for the affected compilers. An example is the LLVM static checker in 2008, but hopefully not later versions if this or any compiler ever used in FreeBSD. errexit() in echo/echo.c always compiled at WARNS=6 with gcc, but was changed in 2008 to define (sic) it as __dead2 with the excuse that this reduces warnings with the static checker. The change is quite broken: - it has the syntax error 'static __dead2 void' - __dead2 is applied to the definition of the function. It doesn't take -funit-at-a-time to see when a static function which is defined before it is used doesn't return. This application point also makes the syntax problems larger. It is now natural to place the attribute declaration where it is, and there is a good chance that the old versions of gcc that didn't like it there also didn't like it being at the end. The errexit() function has many style bugs: - no prototype before the function. This is not a large style bug. The function is unsorted before main() so that it can be its own protoype. But this leaves no natural place to put the attribute. - initialization in declaration - missing blank line after declarations - garbage reason for existence of the function. 4.4BSD doesn't have the function or its style bugs, but just uses printf(). /bin/echo was "optimized" in FreeBSD to avoid using printf(). But with dynamic linkage, the space optimization is almost null, and with libc bloat for static linkage, the optimization doesn't work (/bin/echo has size 11K in my version of 5.2 where the bloat is smaller, but 415K in -current). The main() function in echo/echo.c has rotted similarly. To avoid using printf() in a loop, it uses complicated allocation and writev(). This optimization is about 5% faster for 1 args created by $(jot 1 0), but most uses of echo are with only a couple of args. With 10 args created by $(jot 10 0), the "optimization" is about 2% slower. Bruce ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r287217 - head/usr.sbin/syslogd
On Sun, Aug 30, 2015 at 11:53:00AM +0200, Ed Schouten wrote: > 2015-08-28 16:38 GMT+02:00 Joerg Sonnenberger : > > But the compiler can't tell if it is the *intention* that the function > > never returns. The warning behavior exists because that can easily > > change with macros etc. > I think it's important to keep in mind what this keyword was designed for. > The idea behind this attribute (and the C11 _Noreturn keyword) is to > allow for propagation of optimisation state across compilation units. > You use it to annotate functions in header files, so that the compiler > does not need to handle function return at the call site. This > knowledge can be automatically be inferred if the function is static. Although you are right philosophically, in practice there are some compilers and static analyzers that do benefit from noreturn attributes on static functions. In particular, gcc 4.2.1 generates better code and fewer warnings if static functions are annotated with noreturn if appropriate, and some versions of the Clang/LLVM static analyzer do not do any cross-procedural analysis at all and depend on noreturn attributes on static functions to avoid false positives. > Whether the compiler can throw additional warnings or not based on > whether this keyword is present or what the intent of the programmer > is, is completely irrelevant. In practice this means that many compiler warnings need to be disabled for the affected compilers. > I agree with Bruce that this change makes little sense. I would even > go as far as to say that GCC/Clang should just throw warnings if > _Noreturn is used on a function that is static or used on a definition > instead of a declaration. _Noreturn on a global definition makes no sense but on a static function it may still be needed. -- Jilles Tjoelker ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r287217 - head/usr.sbin/syslogd
2015-08-28 16:38 GMT+02:00 Joerg Sonnenberger : > But the compiler can't tell if it is the *intention* that the function > never returns. The warning behavior exists because that can easily > change with macros etc. I think it's important to keep in mind what this keyword was designed for. The idea behind this attribute (and the C11 _Noreturn keyword) is to allow for propagation of optimisation state across compilation units. You use it to annotate functions in header files, so that the compiler does not need to handle function return at the call site. This knowledge can be automatically be inferred if the function is static. Whether the compiler can throw additional warnings or not based on whether this keyword is present or what the intent of the programmer is, is completely irrelevant. I agree with Bruce that this change makes little sense. I would even go as far as to say that GCC/Clang should just throw warnings if _Noreturn is used on a function that is static or used on a definition instead of a declaration. -- Ed Schouten Nuxi, 's-Hertogenbosch, the Netherlands KvK-nr.: 62051717 ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r287217 - head/usr.sbin/syslogd
On Fri, 28 Aug 2015, Joerg Sonnenberger wrote: On Fri, Aug 28, 2015 at 10:17:56PM +1000, Bruce Evans wrote: -static voiddie(int); +static voiddie(int) __dead2; Since the function is static, it is very easy for the compiler to see that it doesn't return. But the compiler can't tell if it is the *intention* that the function never returns. The warning behavior exists because that can easily change with macros etc. The compiler should trust the programmer to write correct functions. Even gcc-4.2.1 does this by default, since -O implies -funit-at-a-time for gcc-4.2.1. For clang, there is no way to prevent this (except possibly -O0) since, since -fno-unit-at-a-time is broken in clang. It is not broken. It is loadly ignored as unsupported. The very existance of the option in GCC has always been a concession to broken and badly written code, including of course GCC's own CRT. Unsupported == incompatible == broken. My use of this option can probably be reduced to -fno-toplevel-reorder, but that is even more broken in clang (it and -ftoplevel-reorder are "unknown arguments", while -fno-unit-at-a-time is an "unsupported optimization", and -funit-at-a-time works). Bruce ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r287217 - head/usr.sbin/syslogd
On Fri, Aug 28, 2015 at 10:17:56PM +1000, Bruce Evans wrote: > >-static void die(int); > >+static void die(int) __dead2; > > Since the function is static, it is very easy for the compiler to see > that it doesn't return. But the compiler can't tell if it is the *intention* that the function never returns. The warning behavior exists because that can easily change with macros etc. > Even gcc-4.2.1 does this by default, since > -O implies -funit-at-a-time for gcc-4.2.1. For clang, there is no way > to prevent this (except possibly -O0) since, since -fno-unit-at-a-time > is broken in clang. It is not broken. It is loadly ignored as unsupported. The very existance of the option in GCC has always been a concession to broken and badly written code, including of course GCC's own CRT. Joerg ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r287217 - head/usr.sbin/syslogd
On Thu, 27 Aug 2015, Xin LI wrote: Log: die() would never return, mark it as so. Why? (Except to add a style bug.) Modified: head/usr.sbin/syslogd/syslogd.c == --- head/usr.sbin/syslogd/syslogd.c Thu Aug 27 17:16:18 2015 (r287216) +++ head/usr.sbin/syslogd/syslogd.c Thu Aug 27 18:11:00 2015 (r287217) @@ -324,7 +324,7 @@ static const char *cvthname(struct socka static void deadq_enter(pid_t, const char *); static int deadq_remove(pid_t); static int decode(const char *, const CODE *); -static voiddie(int); +static voiddie(int) __dead2; Since the function is static, it is very easy for the compiler to see that it doesn't return. Even gcc-4.2.1 does this by default, since -O implies -funit-at-a-time for gcc-4.2.1. For clang, there is no way to prevent this (except possibly -O0) since, since -fno-unit-at-a-time is broken in clang. Several other functions in the same file are still missing this style bug: - usage(). The style bug is clearly documented for usage() by its absence in style(9) and in most files that have usage(). Howvever, about 30 declarations of usage() in /usr/src have the style bug. - timedout(). This is a signal handler, so doesn't really need it. This is broken signal handler. It carefully uses _exit() so as to not use stdio in a signal handler, but defeats this by also using errx(). Bruce ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r287217 - head/usr.sbin/syslogd
Author: delphij Date: Thu Aug 27 18:11:00 2015 New Revision: 287217 URL: https://svnweb.freebsd.org/changeset/base/287217 Log: die() would never return, mark it as so. MFC after:2 weeks Modified: head/usr.sbin/syslogd/syslogd.c Modified: head/usr.sbin/syslogd/syslogd.c == --- head/usr.sbin/syslogd/syslogd.c Thu Aug 27 17:16:18 2015 (r287216) +++ head/usr.sbin/syslogd/syslogd.c Thu Aug 27 18:11:00 2015 (r287217) @@ -324,7 +324,7 @@ static const char *cvthname(struct socka static voiddeadq_enter(pid_t, const char *); static int deadq_remove(pid_t); static int decode(const char *, const CODE *); -static voiddie(int); +static voiddie(int) __dead2; static voiddodie(int); static voiddofsync(void); static voiddomark(int); ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
