Date: Sun, 25 Mar 2012 04:48:20 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dim@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, tijl@FreeBSD.org Subject: Re: svn commit: r233419 - head/sys/x86/include Message-ID: <20120325040224.Q2467@besplex.bde.org> In-Reply-To: <201203241007.q2OA7MtS024789@svn.freebsd.org> References: <201203241007.q2OA7MtS024789@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120325040224.Q2467>