From owner-freebsd-bugs@FreeBSD.ORG Sat Mar 15 09:34:45 2008 Return-Path: Delivered-To: freebsd-bugs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E8A01106564A; Sat, 15 Mar 2008 09:34:45 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail11.syd.optusnet.com.au (mail11.syd.optusnet.com.au [211.29.132.192]) by mx1.freebsd.org (Postfix) with ESMTP id 72E498FC21; Sat, 15 Mar 2008 09:34:45 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c220-239-252-11.carlnfd3.nsw.optusnet.com.au (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail11.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m2F9YelG021487 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 15 Mar 2008 20:34:43 +1100 Date: Sat, 15 Mar 2008 20:34:40 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Dan Lukes In-Reply-To: <47DB7DA8.7050400@obluda.cz> Message-ID: <20080315192732.A38703@delplex.bde.org> References: <200803150200.m2F206rl078586@freefall.freebsd.org> <20080315151246.L35251@besplex.bde.org> <47DB7DA8.7050400@obluda.cz> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-bugs@FreeBSD.org, imp@FreeBSD.org, Bruce Evans Subject: Re: bin/71613: [PATCH] traceroute(8): cleanup of the usr.sbin/traceroute6 code X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 15 Mar 2008 09:34:46 -0000 On Sat, 15 Mar 2008, Dan Lukes wrote: > Bruce Evans napsal/wrote, On 03/15/08 06:06: >> gcc broke its guarantee about "const" stopping this warning, and even >> more importantly, of the variable not being removed, in gcc-4 or >> earlier. The #if 0 in the above was committed on 2003/04/30 with a > ... >> Using a volatile qualifier instead of a const qualifier gives inconsistent >> results. gcc-4.3 always removes "static char volatile copyright[] but >> doesn't warn about this removal at WARNS > 1. Not warning is clearly >> just a bug. gcc-3.3.3 always keeps this variable, and warns about it >> being unused at WARNS > 1. > > Hm. It seems the GCCs are not consistent between versions. In > advance, we didn't speak about intel's icc for now. > > I found something that make both proposed patches void. There is > defined __COPYRIGHT macro in sys/cdef.h exactly just for such purpose. The > cdefs.h logic tried to deal with icc/gcc and gcc.x/gcc.y differences. No, __COPYRIGHT() should never be used. In FreeBSD, it is even more broken than "static char const copyright[]", and this combined with its uglyness has prevented significant use. It is actually __IDSTRING() that is broken (__COPYRIGHT() just invokes __IDSTRING() with a fairly unique name.) __IDSTRING() is used a lot, e.g., for __FBSDID(), and its brokenness is mostly hamless except for __COPYRIGHT() since its main brokenness is for strings with newlines and such strings are only common for copyright strings. Bugs in __IDSTRING() include: For non-gcc non-intel compilers (and maybe once with an ifdef for very old versions of gcc, but support for very old versions of gcc is just broken here, while other parts of cdefs.h still have considerable uglyness to support such versions): - it uses the old "const" method. This might still work. - it uses the __unused qualifier bogusly. I think this has no effect, since __unused is #defined as nothing for non-gcc non-intel compilers. It is also illogical, and wouldn't work with gcc now, since gcc quite logically removes static unused variables if they are declared as __unused (__unused only suppresses the warning). It should use the __used qualifier or just nothing. For gcc and intel compilers: - if the string has any \n's in it, then __IDSTRING() reduces to an invalid asm directive with the \n's replaced by hard newlines. The desired object code is generated, but the assembler warns once for each hard newline and the build fails iff WERROR is enabled. These bugs have limited __COPYRIGHT() being used in only 7 places in /usr/src, all in code obtained from other systems where __IDSTRING() presumably isn't so broken: ./contrib/lukemftp/src/main.c ./contrib/lukemftpd/src/ftpd.c ./lib/libedit/TEST/test.c ./sbin/routed/main.c ./sbin/routed/rtquery/rtquery.c ./usr.bin/nl/nl.c ./usr.bin/gzip/gzip.c Not even one of these compiles cleanly -- they all have soft newlines in the sources, and none has been hacked to remove the newlines (but ISTR some sources being hacked to avoid this bug). Thus warning(s) are emitted for all of them. WARNS is undefined or 0 for most of them, but even when WARNS is 6 (for gzip), the build doesn't fail, since the warnings are generated by the assembler and -Werror only applies to the main compiler pass (another bug). > Even new gcc versions or brand new compiler can be handled in one > place. > > So all the 'copyright' places shall be patched using __COPYRIGHT > macro. Ugh. To use __IDSTRING() on strings with \n's in them, one ugly fix is to quote the quotes (\n -> \\n). Doing this at the source level would break the non-gcc non-intel case. __IDSTRING normally uses .ident. A more portable fix would be to always use "static const char __used UNIQUIZE(copyright)[]". This would lose putting of the string in a non-loaded section. Not too bad for just copyrights. At the source level, removing static declarations is not too bad. There are also the copyright[] declarations and COPYRIGHT_* macros in . These know better than to use __COPYRIGHT(). They avoid the portability problems by just not declaring copyright[] as static (or even as const). Bruce