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>