From owner-svn-src-all@FreeBSD.ORG Sun Dec 18 16:23:13 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 7A2F6106566B; Sun, 18 Dec 2011 16:23:13 +0000 (UTC) (envelope-from dim@FreeBSD.org) Received: from tensor.andric.com (cl-327.ede-01.nl.sixxs.net [IPv6:2001:7b8:2ff:146::2]) by mx1.freebsd.org (Postfix) with ESMTP id EDBAA8FC08; Sun, 18 Dec 2011 16:23:12 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7:0:1c1b:9549:1b4:20d] (unknown [IPv6:2001:7b8:3a7:0:1c1b:9549:1b4:20d]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id 0976A5C37; Sun, 18 Dec 2011 17:23:12 +0100 (CET) Message-ID: <4EEE136F.5080904@FreeBSD.org> Date: Sun, 18 Dec 2011 17:23:11 +0100 From: Dimitry Andric Organization: The FreeBSD Project User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20111214 Thunderbird/9.0 MIME-Version: 1.0 To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201112172232.pBHMW1Bd079555@svn.freebsd.org> <4EED18B5.8000907@FreeBSD.org> <20111218013905.GA20867@zim.MIT.EDU> In-Reply-To: <20111218013905.GA20867@zim.MIT.EDU> X-Enigmail-Version: 1.3.4 Content-Type: multipart/mixed; boundary="------------050704080607050203000401" Cc: 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: Sun, 18 Dec 2011 16:23:13 -0000 This is a multi-part message in MIME format. --------------050704080607050203000401 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: #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. :) 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. 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? > Incidentally, the "bug" you fixed in telnet/utilities.c is also a > false positive; clang doesn't understand that an index into a > string constant is also a string constant. Yes, that is indeed a false positive, although this way of offsetting a format string is a bit too clever, to the point of being unreadable. :) I will create a PR for the clang guys, and revert this, adding NO_WFORMAT to the Makefile as a workaround. --------------050704080607050203000401 Content-Type: text/x-diff; name="bswap-ternary-1.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="bswap-ternary-1.diff" diff --git a/sys/amd64/include/endian.h b/sys/amd64/include/endian.h index de22c8b..60ed226 100644 --- a/sys/amd64/include/endian.h +++ b/sys/amd64/include/endian.h @@ -111,16 +111,16 @@ __bswap16_var(__uint16_t _x) } #define __bswap64(_x) \ - (__builtin_constant_p(_x) ? \ - __bswap64_const((__uint64_t)(_x)) : __bswap64_var(_x)) + ((__uint64_t)(__builtin_constant_p(_x) ? \ + __bswap64_const((__uint64_t)(_x)) : __bswap64_var(_x))) #define __bswap32(_x) \ - (__builtin_constant_p(_x) ? \ - __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x)) + ((__uint32_t)(__builtin_constant_p(_x) ? \ + __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x))) #define __bswap16(_x) \ - (__builtin_constant_p(_x) ? \ - __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x)) + ((__uint16_t)(__builtin_constant_p(_x) ? \ + __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x))) #define __htonl(x) __bswap32(x) #define __htons(x) __bswap16(x) diff --git a/sys/i386/include/endian.h b/sys/i386/include/endian.h index c09dfb1..0dbc6a5 100644 --- a/sys/i386/include/endian.h +++ b/sys/i386/include/endian.h @@ -111,16 +111,16 @@ __bswap16_var(__uint16_t _x) } #define __bswap64(_x) \ - (__builtin_constant_p(_x) ? \ - __bswap64_const((__uint64_t)(_x)) : __bswap64_var(_x)) + ((__uint64_t)(__builtin_constant_p(_x) ? \ + __bswap64_const((__uint64_t)(_x)) : __bswap64_var(_x))) #define __bswap32(_x) \ - (__builtin_constant_p(_x) ? \ - __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x)) + ((__uint32_t)(__builtin_constant_p(_x) ? \ + __bswap32_const((__uint32_t)(_x)) : __bswap32_var(_x))) #define __bswap16(_x) \ - (__builtin_constant_p(_x) ? \ - __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x)) + ((__uint16_t)(__builtin_constant_p(_x) ? \ + __bswap16_const((__uint16_t)(_x)) : __bswap16_var(_x))) #define __htonl(x) __bswap32(x) #define __htons(x) __bswap16(x) diff --git a/sys/powerpc/include/endian.h b/sys/powerpc/include/endian.h index 15dd7db..da0a0bb 100644 --- a/sys/powerpc/include/endian.h +++ b/sys/powerpc/include/endian.h @@ -124,12 +124,12 @@ __bswap64_var(__uint64_t _x) ((_x << 40) & ((__uint64_t)0xff << 48)) | ((_x << 56))); } -#define __bswap16(x) (__is_constant(x) ? __bswap16_const(x) : \ - __bswap16_var(x)) -#define __bswap32(x) (__is_constant(x) ? __bswap32_const(x) : \ - __bswap32_var(x)) -#define __bswap64(x) (__is_constant(x) ? __bswap64_const(x) : \ - __bswap64_var(x)) +#define __bswap16(x) ((__uint16_t)(__is_constant(x) ? __bswap16_const(x) : \ + __bswap16_var(x))) +#define __bswap32(x) ((__uint32_t)(__is_constant(x) ? __bswap32_const(x) : \ + __bswap32_var(x))) +#define __bswap64(x) ((__uint64_t)(__is_constant(x) ? __bswap64_const(x) : \ + __bswap64_var(x))) #define __htonl(x) ((__uint32_t)(x)) #define __htons(x) ((__uint16_t)(x)) diff --git a/sys/sparc64/include/endian.h b/sys/sparc64/include/endian.h index 2ca467e..d81ec1b 100644 --- a/sys/sparc64/include/endian.h +++ b/sys/sparc64/include/endian.h @@ -109,12 +109,12 @@ __bswap64_var(__uint64_t _x) ((_x << 40) & ((__uint64_t)0xff << 48)) | ((_x << 56))); } -#define __bswap16(x) (__is_constant(x) ? __bswap16_const(x) : \ - __bswap16_var(x)) -#define __bswap32(x) (__is_constant(x) ? __bswap32_const(x) : \ - __bswap32_var(x)) -#define __bswap64(x) (__is_constant(x) ? __bswap64_const(x) : \ - __bswap64_var(x)) +#define __bswap16(x) ((__uint16_t)(__is_constant(x) ? __bswap16_const(x) : \ + __bswap16_var(x))) +#define __bswap32(x) ((__uint32_t)(__is_constant(x) ? __bswap32_const(x) : \ + __bswap32_var(x))) +#define __bswap64(x) ((__uint64_t)(__is_constant(x) ? __bswap64_const(x) : \ + __bswap64_var(x))) #define __htonl(x) ((__uint32_t)(x)) #define __htons(x) ((__uint16_t)(x)) --------------050704080607050203000401--