On Sat, 24 Mar 2012, Dimitry Andric wrote:
Log: Fix the following clang warning in sys/dev/dcons/dcons.c, caused by the recent changes in sys/x86/include/endian.h:sys/dev/dcons/dcons.c:190:15: error: implicit conversion from '__uint32_t' (aka 'unsigned int') to '__uint16_t' (aka 'unsigned short') changes value from 1684238190 to 28526 [-Werror,-Wconstant-conversion] buf->magic = ntohl(DCONS_MAGIC); ^~~~~~~~~~~~~~~~~~ sys/sys/param.h:306:18: note: expanded from: #define ntohl(x) __ntohl(x) ^ ./x86/endian.h:128:20: note: expanded from: #define __ntohl(x) __bswap32(x) ^ ./x86/endian.h:78:20: note: expanded from: __bswap32_gen((__uint32_t)(x)) : __bswap32_var(x)) ^ ./x86/endian.h:68:26: note: expanded from: (((__uint32_t)__bswap16(x) << 16) | __bswap16((x) >> 16)) ^ ./x86/endian.h:75:53: note: expanded from: __bswap16_gen((__uint16_t)(x)) : __bswap16_var(x))) ~~~~~~~~~~~~~ ^
This warning was discussed before things were committed. gcc gives a similar warning, but according to tijl, it can't happen in normal use with either gcc or clang because -Wno-system-headers suppresses warnings in system headers. -Wnosystem-headers is the default in the kernel, where the above problem happens, so I don't see how it happens. However, bsd.sys.mk forces the non-default of -Wsystem-headers in userland at WARNS >= 1, so I don't see why there isn't a problem in userland if userland actually uses an expression like ntohl(DCONS_MAGIC).
This is because the __bswapXX_gen() macros (for x86) call the regular __bswapXX() macros. Since the __bswapXX_gen() variants are only called when their arguments are constant, there is no need to do that constancy check recursively. Also, it causes the above error with clang.
Not true. The __bswapXX_gen() are called with non-constant args from __bswap64_var() in the i386 case. As documented there, it is important for optimizations that __bswap64_gen() does do the constancy check recursively. This was discussed before things were committed, and again when jhb siggested breaking it similarly to this commit to avoid a problem on ia64.
Fix it by calling __bswap16_gen() from __bswap32_gen(), and similarly, __bswap32_gen() from __bswap64_gen().
This breaks it. DIring development, I had a correct fix involving casting down the arg.
While here, add extra parentheses around the __bswap16_gen() macro expansion, to prevent unexpected side effects.
These parentheses were intentionally left out for clarity. __bswap16_gen() is not user-serving, and all places in endian.h that use it supply enough parentheses.
Modified: head/sys/x86/include/endian.h Modified: head/sys/x86/include/endian.h ============================================================================== --- head/sys/x86/include/endian.h Sat Mar 24 06:40:41 2012 (r233418) +++ head/sys/x86/include/endian.h Sat Mar 24 10:07:21 2012 (r233419) @@ -63,11 +63,11 @@ #define BYTE_ORDER _BYTE_ORDER #endif -#define __bswap16_gen(x) (__uint16_t)((x) << 8 | (x) >> 8) +#define __bswap16_gen(x) ((__uint16_t)((x) << 8 | (x) >> 8)) #define __bswap32_gen(x) \ - (((__uint32_t)__bswap16(x) << 16) | __bswap16((x) >> 16)) + (((__uint32_t)__bswap16_gen(x) << 16) | __bswap16_gen((x) >> 16)) #define __bswap64_gen(x) \ - (((__uint64_t)__bswap32(x) << 32) | __bswap32((x) >> 32)) + (((__uint64_t)__bswap32_gen(x) << 32) | __bswap32_gen((x) >> 32)) #ifdef __GNUCLIKE_BUILTIN_CONSTANT_P #define __bswap16(x) \
Hmm, __bswap32_gen() and __bswap64_gen() were sloppy about leaving out the parentheses. They have unnecssary (?) parentheses around the whole expression. These parentheses are a bit more needed than for __bswap16_gen(), since '(cast)(expr)' rarely needs more while 'foo | bar' often does. These functions also have unnecessary and style-conflicting parentheses around the '<<' expression. Such parentheses are not used for either the '<<' or the '>>' expression for __bswap16_gen(). They are needed for parameter passing for the other 2 '>>' expressions. Bruce _______________________________________________ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
