From owner-svn-src-head@FreeBSD.ORG Sat Mar 24 21:19:54 2012 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 4FFA3106566B; Sat, 24 Mar 2012 21:19:54 +0000 (UTC) (envelope-from dim@FreeBSD.org) Received: from tensor.andric.com (cl-327.ede-01.nl.sixxs.net [IPv6:2001:7b8:2ff:146::2]) by mx1.freebsd.org (Postfix) with ESMTP id CF1FF8FC0A; Sat, 24 Mar 2012 21:19:53 +0000 (UTC) Received: from [IPv6:2001:7b8:3a7:0:c8dc:6e5:d0ce:fe36] (unknown [IPv6:2001:7b8:3a7:0:c8dc:6e5:d0ce:fe36]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tensor.andric.com (Postfix) with ESMTPSA id 084F15C37; Sat, 24 Mar 2012 22:19:53 +0100 (CET) Message-ID: <4F6E3A7D.9020709@FreeBSD.org> Date: Sat, 24 Mar 2012 22:19:57 +0100 From: Dimitry Andric Organization: The FreeBSD Project User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20120312 Thunderbird/11.0 MIME-Version: 1.0 To: Bruce Evans References: <201203241007.q2OA7MtS024789@svn.freebsd.org> <20120325040224.Q2467@besplex.bde.org> In-Reply-To: <20120325040224.Q2467@besplex.bde.org> X-Enigmail-Version: 1.4 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 24 Mar 2012 21:19:54 -0000 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.