Date: Sat, 24 Mar 2012 22:19:57 +0100 From: Dimitry Andric <dim@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> 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: <4F6E3A7D.9020709@FreeBSD.org> In-Reply-To: <20120325040224.Q2467@besplex.bde.org> References: <201203241007.q2OA7MtS024789@svn.freebsd.org> <20120325040224.Q2467@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 2012-03-24 18:48, Bruce Evans wrote: > 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. Then why was it committed without fixing the warning first? > 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. I don't see that option anywhere in either the world or kernel build. Why would you not want to warn about system headers? > 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). It only occurs in certain cases, and I have not been able to exactly determine when. It doesn't seem to be related to -Wsystem-headers. >> 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. Sure, but there it doesn't matter at all. > 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. Isn't it much easier, and less obtuse, to just jam in two bswaps for the i386? I never saw the reason for making the bswap macros *more* complicated instead of less. If you obfuscate them enough, no compiler will be able to optimize properly... :) >> 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. If it works correctly with clang, please post it. >> 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. Let's hope so, until a follow up commit breaks it again.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4F6E3A7D.9020709>