From owner-svn-src-head@FreeBSD.ORG Fri Mar 30 12:12:01 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8F9D51065670; Fri, 30 Mar 2012 12:12:01 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id 05C608FC1F; Fri, 30 Mar 2012 12:12:00 +0000 (UTC) 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 q2UCBvbw025025 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 30 Mar 2012 23:11:58 +1100 Date: Fri, 30 Mar 2012 23:11:57 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dimitry Andric In-Reply-To: <201203292331.q2TNVmwN014920@svn.freebsd.org> Message-ID: <20120330212607.H1071@besplex.bde.org> References: <201203292331.q2TNVmwN014920@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: r233684 - 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: Fri, 30 Mar 2012 12:12:01 -0000 On Thu, 29 Mar 2012, Dimitry Andric wrote: > Log: > Fix an issue introduced in sys/x86/include/endian.h with r232721. In > that revision, the bswapXX_const() macros were renamed to bswapXX_gen(). > > Also, bswap64_gen() was implemented as two calls to bswap32(), and > similarly, bswap32_gen() as two calls to bswap16(). This mainly helps > our base gcc to produce more efficient assembly. > > However, the arguments are not properly masked, which results in the > wrong value being calculated in some instances. For example, > bswap32(0x12345678) returns 0x7c563412, and bswap64(0x123456789abcdef0) > returns 0xfcdefc9a7c563412. No, bswap32() and bswap64() were properly masked in r232721, and not only that, they were cast to the correct type, as required to prevent related problems when they are used. It was the next commit, rmumble, that broke this, by using uncast expressions sub-expressions for bswap32_gen() and bswap64_gen(). > Fix this by appropriately masking the arguments to bswap16() in > bswap32_gen(), and to bswap32() in bswap64_gen(). This should also > silence warnings from clang. Sorry, this is inappropriate. It has no effect except to silence the warnings from clang in a confusing way. > Submitted by: jh > > Modified: > head/sys/x86/include/endian.h > > Modified: head/sys/x86/include/endian.h > ============================================================================== > --- head/sys/x86/include/endian.h Thu Mar 29 23:30:17 2012 (r233683) > +++ head/sys/x86/include/endian.h Thu Mar 29 23:31:48 2012 (r233684) > @@ -65,9 +65,9 @@ > > #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((x) & 0xffff) << 16) | __bswap16((x) >> 16)) > #define __bswap64_gen(x) \ > - (((__uint64_t)__bswap32(x) << 32) | __bswap32((x) >> 32)) > + (((__uint64_t)__bswap32((x) & 0xffffffff) << 32) | __bswap32((x) >> 32)) This has no effect except to make it clear to clang (but not to humans) that the implicit conversions do the right thing. It would have worked around the bug in a previous version and accidentally fixed the warning in the same way as this version. For example, consider bswap32(0x12345678). This becomes __bswap32_gen( 0x12345678) after a null cast of the arg to __uint32_t. (We don't cast the result of __bswap32_gen(), and depend on minor magic (mainly on 32-bit ints) for this to be unnecessary.). The next reduction, for the correct version which has the above expression without the 0xfff mask, is: (((__uint32_t)__bswap16(0x12345678) << 16) | __bswap16(0x12345678 >> 16)) The __bswap16() macros work unsurprisingly (except for the conditional operator in them!) and give constants of type uint16_t which I denote by a US suffix. The result of evaluating them is: (((__uint32_t)0x7856US << 16) | 0x3412US) and the result is the unsurprising 0x78563412. Note that for the 'gen' part, everything is explicitly cast, as required for the result to be correct, and there is no reason for clang to warn, and it doesn't. However, for the `var' part, clang warns spuriously. This is a bug in clang, since the `var' part is not even used. 0x12345678 >> 16 is 0x1234 which is representable by __uint16_t, so there is no warning for it. However, __bswap16(0x12345678) expands to essentially: 1 ? __bswap16_gen(0x12345678) : __bswap16_var(0x12345678) Here __bswap16_var() takes a __uint16_t arg, so an implicit value-losing cast is required to call it. clang warns about this although __bswap16_var() is not actually called. Masking with 0xffff fixes this accidentally by converting 0x12345678 to 0x5678. Now it is representable as a __uint16_t, so clang doesn't warn about downcasting it when not-calling __bswap16_var() on it. A previous version broke the result by changing __bswap16() to __bswap16_gen(). In the critical expression: (((__uint32_t)__bswap16_gen(0x12345678) << 16) | __bswap16_gen(0x12345678 >> 16)), 0x12345678 >> 16 has only 16 bits, so there are no problems with it. But in __bswap16_gen(0x12345678), the value has extra bits which the macro is intentionally designed not to handle (callers are required to pass it only 16 bits. Its result is (0x12345678 << 8) | (0x12345678 >> 8) cast to __uint16_t. Compilers should warn about the overflow in this. If they just shift the bits out, the result is 0x34567800 | 0x00123456 = 0x34567c56 cast down = 0x7c56. It's surprising that this has only 1 bit incorrect. Masking with 0xffff fixes this by discarding the extra bits, which gives the same value although a different type than the unbroken version (the unbroken version casts the arg to __uint16_t before invoking __bswap16_gen() although not before invoking __bswap16_var() since it knows that the prototype will do an implicit cast for the latter and it doesn't want to know about the clang bug. > > #ifdef __GNUCLIKE_BUILTIN_CONSTANT_P > #define __bswap16(x) \ > The correct fix for working around the clang bugs is as I said before: cast the arg in the place where there is a problem: % Index: endian.h % =================================================================== % RCS file: /home/ncvs/src/sys/x86/include/endian.h,v % retrieving revision 1.7 % diff -u -2 -r1.7 endian.h % --- endian.h 29 Mar 2012 23:31:48 -0000 1.7 % +++ endian.h 30 Mar 2012 11:20:37 -0000 % @@ -66,18 +66,18 @@ % #define __bswap16_gen(x) (__uint16_t)((x) << 8 | (x) >> 8) % #define __bswap32_gen(x) \ % - (((__uint32_t)__bswap16((x) & 0xffff) << 16) | __bswap16((x) >> 16)) % + (((__uint32_t)__bswap16(x) << 16) | __bswap16((x) >> 16)) % #define __bswap64_gen(x) \ % - (((__uint64_t)__bswap32((x) & 0xffffffff) << 32) | __bswap32((x) >> 32)) % + (((__uint64_t)__bswap32(x) << 32) | __bswap32((x) >> 32)) Remove accidental fix. % % #ifdef __GNUCLIKE_BUILTIN_CONSTANT_P % #define __bswap16(x) \ % ((__uint16_t)(__builtin_constant_p(x) ? \ % - __bswap16_gen((__uint16_t)(x)) : __bswap16_var(x))) % + __bswap16_gen((__uint16_t)(x)) : __bswap16_var((__uint16_t)(x)))) Don't forget to fix __bswap16() too. To see the problem without this fix, try __bswap16(0x123456). The arg is weird and has more than 16-bits, so clang warns about downcasting it for not-calling __bswap16_var() on it. Other weird cases are supposed to be fixed too -- the arg is always cast down explicitly if it is weird and too wide to start. % #define __bswap32(x) \ % (__builtin_constant_p(x) ? \ % - __bswap32_gen((__uint32_t)(x)) : __bswap32_var(x)) % + __bswap32_gen((__uint32_t)(x)) : __bswap32_var((__uint32_t)(x))) % #define __bswap64(x) \ % (__builtin_constant_p(x) ? \ % - __bswap64_gen((__uint64_t)(x)) : __bswap64_var(x)) % + __bswap64_gen((__uint64_t)(x)) : __bswap64_var((__uint64_t)(x))) % #else % /* XXX these are broken for use in static initializers. */ I haven't lloked at tijl's larger rewrite yet. Bruce