From owner-svn-src-all@FreeBSD.ORG Sat Mar 24 17:48:37 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id ACFB11065675; Sat, 24 Mar 2012 17:48:37 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from fallbackmx06.syd.optusnet.com.au (fallbackmx06.syd.optusnet.com.au [211.29.132.8]) by mx1.freebsd.org (Postfix) with ESMTP id 370FD8FC25; Sat, 24 Mar 2012 17:48:36 +0000 (UTC) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by fallbackmx06.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q2OHmT8A001453; Sun, 25 Mar 2012 04:48:29 +1100 Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q2OHmKtn002178 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 25 Mar 2012 04:48:22 +1100 Date: Sun, 25 Mar 2012 04:48:20 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dimitry Andric In-Reply-To: <201203241007.q2OA7MtS024789@svn.freebsd.org> Message-ID: <20120325040224.Q2467@besplex.bde.org> References: <201203241007.q2OA7MtS024789@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed 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-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 24 Mar 2012 17:48:37 -0000 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