Date: Fri, 30 Mar 2012 23:30:33 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Andrey Chernov <ache@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Dimitry Andric <dim@freebsd.org> Subject: Re: svn commit: r233684 - head/sys/x86/include Message-ID: <20120330231216.G1071@besplex.bde.org> In-Reply-To: <20120330082528.GA47173@vniz.net> References: <201203292331.q2TNVmwN014920@svn.freebsd.org> <20120330082528.GA47173@vniz.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 30 Mar 2012, Andrey Chernov wrote: > On Thu, Mar 29, 2012 at 11:31:48PM +0000, Dimitry Andric wrote: >> 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. > > Is sign extension considered in that place? Shifting any signed value to > ">>" direction (char, short, int, etc.) replicates sign bit, so cast to > corresponding unsigned value must be done first, which may take less > instructions, than masking (I am not sure about this part, just > guessing). Casting in that case applies to the argument (x) not to result > (x >> YY). There are lots of casts to uint* which are supposed to be sufficent, although some shortcuts are taken, especially in the 'gen' macros. The main thing to watch out for is C90's broken sign "value-preserving" promotion rule turning unsigned types into signed ones, so that sign extension bugs may occur later. >> #define __bswap16_gen(x) (__uint16_t)((x) << 8 | (x) >> 8) For example, this macro is private, and callers are required to know that its arg needs to be uint16_t or possibly smaller, and to not forget to cast to that if necessary. Then there are no problems evaluating ((x) << 8 | (x) >> 8), but it has type plain int. But we want the result to have type uint16_t and cast to that (this cast should probably be in callers too). So the plain int doesn't escape, but whenever the uint16_t is used, it gets promoted to plain int and its users should be careful with this. >> #define __bswap32_gen(x) \ >> - (((__uint32_t)__bswap16(x) << 16) | __bswap16((x) >> 16)) >> + (((__uint32_t)__bswap16((x) & 0xffff) << 16) | __bswap16((x) >> 16)) Here the cast to uint32_t is because the caller _is_ being careful with this. If the expression were plain __bswap16((x) << 16, then when __bswap16() returns 0x8000, the shift gives (plain int)0x80000000 = -0x7fffffff - 1 with 32-bit ints. This would work in practice on normal 2's complement machines, but is unportable. Note that the result of the whole expression is not cast to uint32_t. We depend on ints being precisely 32 bits, so that the the result of the expression, which is either plain int or unsigned int (provided that ints have at least 32 bits with no padding bits), is in fact precisely uint32_t. This is another reason why casting the result of the gen macros belongs in callers. (We mostly don't cast, but use one to cast down the result of the conditional expression in the 16-bit case after the default promotions cast up to plain int. Omitting the corresponding cast for the other widths again depends on ints being 32 bits.) >> #define __bswap64_gen(x) \ >> - (((__uint64_t)__bswap32(x) << 32) | __bswap32((x) >> 32)) >> + (((__uint64_t)__bswap32((x) & 0xffffffff) << 32) | __bswap32((x) >> 32)) Now we must cast up for the completely different reason that __bswap32() returns only uint32_t ints, and with 32 bit ints the implicit upwards conversion is null, but we need to shift to 64 bits, so we must start with at least 64 bits. >> >> #ifdef __GNUCLIKE_BUILTIN_CONSTANT_P >> #define __bswap16(x) \ Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120330231216.G1071>