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.
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))
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
