Re: svn commit: r233684 - head/sys/x86/include
On Thu, Mar 29, 2012 at 11:31:48PM +, 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). #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) 0x) 16) | __bswap16((x) 16)) #define __bswap64_gen(x)\ - (((__uint64_t)__bswap32(x) 32) | __bswap32((x) 32)) + (((__uint64_t)__bswap32((x) 0x) 32) | __bswap32((x) 32)) #ifdef __GNUCLIKE_BUILTIN_CONSTANT_P #define __bswap16(x)\ -- http://ache.vniz.net/ ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r233684 - head/sys/x86/include
On 2012-03-30 10:25, Andrey Chernov wrote: On Thu, Mar 29, 2012 at 11:31:48PM +, 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). Yes, the arguments are all converted to unsigned types where necessary. The __bswapXX_gen() macros are only used internally by the __bswapXX() macros and the __bswapXX_var() inline functions. In case of the __bswapXX() macros, you can see that the argument to __bswapXX_gen() is first explicitly cast to an unsigned type, for example with __bswap32(): #define __bswap32(x)\ (__builtin_constant_p(x) ? \ __bswap32_gen((__uint32_t)(x)) : __bswap32_var(x)) Therefore, right shifting will not give any problems. For the call to __bswap32_var(), such casting is not needed, since the argument will be implicitly converted to __uint32_t. As Bruce has mentioned, you could add more explicit casts and additional parentheses, but those would be superfluous. ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r233684 - head/sys/x86/include
On Fri, Mar 30, 2012 at 02:11:21PM +0200, Dimitry Andric wrote: In case of the __bswapXX() macros, you can see that the argument to __bswapXX_gen() is first explicitly cast to an unsigned type, for example with __bswap32(): #define __bswap32(x)\ (__builtin_constant_p(x) ? \ __bswap32_gen((__uint32_t)(x)) : __bswap32_var(x)) Therefore, right shifting will not give any problems. For the call to __bswap32_var(), such casting is not needed, since the argument will be implicitly converted to __uint32_t. As Bruce has mentioned, you could add more explicit casts and additional parentheses, but those would be superfluous. I mention right shift just as potential problem. I really want to target casting vs. masking problem (masking provide more instructions). bde@ already suggest the same thing I generally mean in more details in his recent answer with the patch: % + __bswap16_gen((__uint16_t)(x)) : __bswap16_var((__uint16_t)(x -- http://ache.vniz.net/ ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
svn commit: r233684 - head/sys/x86/include
Author: dim Date: Thu Mar 29 23:31:48 2012 New Revision: 233684 URL: http://svn.freebsd.org/changeset/base/233684 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. 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. 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) 0x) 16) | __bswap16((x) 16)) #define__bswap64_gen(x)\ - (((__uint64_t)__bswap32(x) 32) | __bswap32((x) 32)) + (((__uint64_t)__bswap32((x) 0x) 32) | __bswap32((x) 32)) #ifdef __GNUCLIKE_BUILTIN_CONSTANT_P #define__bswap16(x)\ ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
