From owner-svn-src-all@FreeBSD.ORG Mon Dec 19 00:12:10 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 175C5106564A; Mon, 19 Dec 2011 00:12:10 +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 7924B8FC12; Mon, 19 Dec 2011 00:12:08 +0000 (UTC) Received: from c211-28-227-231.carlnfd1.nsw.optusnet.com.au (c211-28-227-231.carlnfd1.nsw.optusnet.com.au [211.28.227.231]) by mail09.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id pBJ0C4vl018626 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 19 Dec 2011 11:12:06 +1100 Date: Mon, 19 Dec 2011 11:12:04 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dimitry Andric In-Reply-To: <4EEE136F.5080904@FreeBSD.org> Message-ID: <20111219080853.X877@besplex.bde.org> References: <201112172232.pBHMW1Bd079555@svn.freebsd.org> <4EED18B5.8000907@FreeBSD.org> <20111218013905.GA20867@zim.MIT.EDU> <4EEE136F.5080904@FreeBSD.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r228668 - head/usr.bin/netstat X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Dec 2011 00:12:10 -0000 On Sun, 18 Dec 2011, Dimitry Andric wrote: > On 2011-12-18 02:39, David Schultz wrote: > ... >>>> Log: >>>> Revert r228650, and work around the clang false positive with printf >>>> formats in usr.bin/netstat/atalk.c by conditionally adding NO_WFORMAT to >>>> the Makefile instead. > ... >> Have you been keeping track of the other hacks you've been >> sprinkling throughout the tree to work around clang bugs, e.g., >> the one in fsdb? It would be unfortunate if someone else has to >> waste their time later on figuring out what you did, when we could >> just as easily have waited a month for the clang bug to be fixed. > > Yes, I will revert the fsdb change too, and add a workaround in the > Makefile. Though after talking with a language lawyer type person, it > seems that clang was actually right with its original warning. > > Apparently, if you use ?: with two shorts, the end result always gets > promoted to an int, due to the Usual Arithmetic Conversions, so using > the "%hu" format is not correct. The following small program > demonstrates this: An expression starting with shorts indeed gets promoted by ?: (or almost any binary or unary operator), but that is irrelevant provided the result of the expression is representable as a short. It also gets promoted by passing it, and the compiler can't tell the difference at that point so it shouldn't warn (since this is the point at which printf() will see it and the purpose of the warning is to check to for things that will cause problems at the level of printf()). For the DIP() expression, as for any typical ?: expression, the result is representable in the same type as the operands if that type is common, since ?: just selects between the operands. (In most cases, the operands in DIP() have different types, but in cases where the ffs1 type has not been expanded, they have the same type, and for nlink_t this type is also (unsigned) short.) If the expesssion involved a more complicated operator like addition, this would not be obvious but might still be true. The compiler could determine this especially easily for constant expressions. It is important that compilers not warn for constant and other expressions that they know won't cause any problems for printf(). Passing sizeof(foo) for a field width is not quite an example, since the width of sizeof(foo) differs from the width of what printf expects (that is, sizeof(int)) on some machines, so you want it warned about always for portable code. The original non-problem could be checked for in a more refined class of warnings, starting with: - -Wbogus-cast[level]: warn if a cast is bogus at the given level. Some possible levels: - 0: bde doesn't like it - 1: compiler maintainer doesn't like it - 100-200: has no effect because it is: - 100: the identity conversion in the abstract machine - 101: the identity conversion in the current machine - 102: same as the default operator promotion in the abstract machine - 103: same as the default operator promotion in the current machine - 104: same as the default parameter promotion in the abstract machine - 105: same as the default parameter promotion in the current machine - 106: same as prototype parameter conversion in the abstract machine - 107: same as prototype parameter conversion in the current machine - 110-119: like 100-109, but changes the type, but then the next operation or cast or parameter type changes the type and also the value back it would have been without any cast at this level, so that the combination is null. This is useful mainly in conjunction with parameter-passing, as an extension of 106-107. We don't want to attach the warning to the final conversion given by a prototye, since the conversion given by prototypes is non-bogus in other contexts. - -Wsubtle-conversion[level]. Sort of the inverse of the above. If you add a cast to quiet a warning from this, then it should probably cause a warning from -Wbogus-cast[level]. I haven't thought this through, but in the present case -Wbogus-cast[114] would warn about casting down DIP() to u_short, since this has no effect because the default parameter promotion will turn it back into int. BTW, the printf format for di_nlink in fsdb is still broken in 2 ways. nlink_t is bogusly unsigned (uint16_t), but the format is %hd (signed short). First way: Using %h at all is broken, since it only works for 16-bit shorts, and nlink_t is a typedef to avoid hard-coding this. Second way: sign mismatch. The top bit of nlink_t is supposed to be unused (LINK_MAX = 32767), so this is relatively harmless, but fsdb is supposed to be able to work on current file systems, so an invalid i_nlink might be > LINK_MAX and then it will be misprinted. An invalid i_nlink of nearly 65535 probably means a "negative" value, and then printing it as signed is better. Both cases can't be handled, so the format should match the type. All of these problems and subtle non-problems can be avoided by not using %h. Just use the format that matches the arg type. This is just %d matching int after the default promotions (either inside DIP() or in parameter passing). This assumes that nlink_t is strictly smaller than int, which is a weaker (though not strictly weaker) assumption than the wrong assumption that it is short. Since it is strictly smaller, it the promotion is non-null and the type loses the signedness. The promoted value is always non-negative since the original type was unsigned, and the same as the original value since the conversion was a promotion. This value can be naturally printed using %d (%u works too, but doesn't match the parameter types, so there are minor subtleties to prove that it works). However, printing the value as non-negative is not bug for bug compatible with the original code, which was essentially: printf("LINKCNT=%hd", dp->di_nlink); This essentially abuses %h to corrupt the type from uint16_t to short (in FreeBSD-1, nlink_t was u_short, so only the sign was corrupted). If you want to pretend that your uint16_t's are actually signed, then the right way to do this is to replace them by checking the top bit in them and then force 2's complement arithmetic: /* XXX assume nlink_t is uint16_t: */ printf("LINKCNT=%d", (dp->di_nlink & 0x8000 ? -1 : 1) * dp->di_nlink); Next best is to assume that implementation-defined behaviour is 2's complement and just cast: /* XXX assume nlink_t is u_short: */ printf("LINKCNT=%d", (short)dp->di_nlink); Now with DIP(), we must cast the original di_nlink's, or equivalently cast DIP() down to u_short first: /* XXX assume nlink_t is u_short: */ printf("LINKCNT=%d", (short)(u_short)DIP(dp, di_nlink))); Here the double cast might be warned about by -Wbogus-cast[mumble], since it has the same effect as directly casting to short, but I left it in for clarity. But I think these complications are excessive here, and fsdb should just use %d on the non-negative DIP(). > #include > > int main(void) > { > printf("%zu\n", sizeof(1 > 2 ? (short)1 : (short)2)); > return 0; > } > > It will always print sizeof(int), e.g. 4 on most arches. This is not > what most programmers expect, I guess, at least I didn't. :) I've always expected this since I fixed type bugs in my compiler :-). I tried hundreds of expressions like the above in various compilers to see what is normal. Another interesting case is cpp expressions. C eventually standardized on only [u_]long arithmetic (now [u]intmax_t) in cpp expressions, but my K&R-ish compiler has fully typed cpp expressions including sizeof(), casts and floating point, so its cpp expressions give the same results as runtime expressions but different results from Standard cpp expressions. This was useful for determining runtime behaviour at compile time. > Since htons() and ntohs() are implemented in our tree with the __bswap > macros, which use ?: operators (at least when GNU inline asm and > __builtin_constant_p are supported), they will in fact return int, not > uint16_t. This is very implementation-dependent, and also broken if that is what they do now: Broken: htons(), etc., are now standardized, and any reasonable standard requires the optional macro versions of them to act as if they were functions. I don't know if POSIX is reasonable here. In the draft7 2001 version, I could only easily find: % 7321 The following shall either be declared as functions, defined as macros, or both. If functions are % 7322 declared, function prototypes shall be provided. % 7323 uint32_t htonl(uint32_t); % 7324 uint16_t htons(uint16_t); % 7325 uint32_t ntohl(uint32_t); % 7326 uint16_t ntohs(uint16_t); Thus if they are implemented as functions, they must return a non-promoted type. Any macro versions should return the same non-promoted type. Some recent implementations changed to using inline functions more. This might have changed the type. There are also the ones with a constant variant. A misimplementation could easily have the constant variant returning a different type to the variable version. And a ?: to select between the constant and variable versions gives the promoted type unless it is cast back down. Very implementation-dependent: Apart from the above, at least i386 used to do __byte_swap_word() entirely in asm except for a statement-expression to give a result of type u_short (later u_int16_t). ntohs was defined as __byte_swap_word in FreeBSD-4 and earlier, and there was no optimization for the constant case to give an ?: expression anywhere in this family of functions. The current implemention uses ?: consistently on i386 at least, so it consistently gives the broken version for __htons() and ntohs(). There used to be function versions of these interfaces. I got someone to clean up some of this. The cleanup was incomplete, but removed most of the function versions in libc. Grepping for ntohs in libc now shows the following vestiges: - most arches still have ntohs in a Symbol.map, although the function doesn't exist. This was apparently missed in the cleanup. - mips still has ntohs in a Makefile.inc and in a ntohs.S. This was apparently uncleaned after the cleanup. > A better fix is to add explicit casts to the __bswap macros, as per > attached patch, which I'm running through a make universe now. (Note > that some arches, such as arm and mips already add the explicit casts > for the expected __bswap return types.) Would that be OK to commit? It seems to be necessary, and should work since even if anyone wants to use ntohs in a cpp expression, it already won't work because it has casts and more in it. Some of the current mess is because jeff needed ntohs etc. to work in static initializers. This prevents using the same inline function version for the constant case as for the variable case (gcc now optimizes the general version nicely for the constant case). I've reported most of the style bugs and bugs in the mess before. The following are most obvious now on i386: - macro args are bogusly named with a leading underscores. Such underscores are only needed to protect namespaces in inline functions. They are just obfuscations in macros. - __bswap16_const() casts its return value back down to __uint16_t. This is useless, since __bswap16_const() is only usable in __bswap16() where the value is immediately promoted back to int. (Note that none of the __bswapN_const() functions is directly usable, since they all intentionally omit casting their arg for readability.) This was apparently an attempt to make the final type __uint16_t. Bruce