Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 18 Dec 2011 17:23:11 +0100
From:      Dimitry Andric <dim@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r228668 - head/usr.bin/netstat
Message-ID:  <4EEE136F.5080904@FreeBSD.org>
In-Reply-To: <20111218013905.GA20867@zim.MIT.EDU>
References:  <201112172232.pBHMW1Bd079555@svn.freebsd.org> <4EED18B5.8000907@FreeBSD.org> <20111218013905.GA20867@zim.MIT.EDU>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <stdio.h>

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--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4EEE136F.5080904>