[PATCH 1/6] make byte swapping macros less opaque to the optimizer

[Matt Turner]

On Thu, Apr 28, 2011 at 4:36 PM, Siarhei Siamashka
<siarhei.siamashka at gmail.com> wrote:
> On Thu, Apr 28, 2011 at 2:33 AM, Matt Turner <mattst88 at gmail.com> wrote:
>> diff --git a/include/misc.h b/include/misc.h
>> index 803f5ba..b7a3fd2 100644
>> --- a/include/misc.h
>> +++ b/include/misc.h
>> @@ -240,32 +240,17 @@ xstrtokenize(const char *str, const char* separators);
>>  #define SwapRestL(stuff) \
>>     SwapLongs((CARD32 *)(stuff + 1), LengthRestL(stuff))
>>
>> -/* byte swap a 32-bit value */
>> -#define swapl(x, n) { \
>> -                n = ((char *) (x))[0];\
>> -                ((char *) (x))[0] = ((char *) (x))[3];\
>> -                ((char *) (x))[3] = n;\
>> -                n = ((char *) (x))[1];\
>> -                ((char *) (x))[1] = ((char *) (x))[2];\
>> -                ((char *) (x))[2] = n; }
>> -
>> -/* byte swap a short */
>> -#define swaps(x, n) { \
>> -                n = ((char *) (x))[0];\
>> -                ((char *) (x))[0] = ((char *) (x))[1];\
>> -                ((char *) (x))[1] = n; }
>> -
>>  /* copy 32-bit value from src to dst byteswapping on the way */
>> -#define cpswapl(src, dst) { \
>> -                 ((char *)&(dst))[0] = ((char *) &(src))[3];\
>> -                 ((char *)&(dst))[1] = ((char *) &(src))[2];\
>> -                 ((char *)&(dst))[2] = ((char *) &(src))[1];\
>> -                 ((char *)&(dst))[3] = ((char *) &(src))[0]; }
>> +#define cpswapl(src, dst) (dst) = lswapl((src))
>>
>>  /* copy short from src to dst byteswapping on the way */
>> -#define cpswaps(src, dst) { \
>> -                ((char *) &(dst))[0] = ((char *) &(src))[1];\
>> -                ((char *) &(dst))[1] = ((char *) &(src))[0]; }
>> +#define cpswaps(src, dst) (dst) = lswaps((src))
>> +
>> +/* byte swap a 32-bit value */
>> +#define swapl(x, n) (*(x)) = lswapl(*(x))
>> +
>> +/* byte swap a short */
>> +#define swaps(x, n) (*(x)) = lswaps(*(x))
>
> This is not an equal replacement for the previous code. The difference
> is that the older variant of 'swapl' could accept any pointer having
> any alignment, but still do byteswap for exactly 4 bytes at that
> memory location.
>
> Just to perform an additional sanity check, the following test code can be used:
>
> -#define swapl(x, n) (*(x)) = lswapl(*(x))
> +#define swapl(x, n) do {                    \
> +        char dummytmp1[4 - sizeof(*(x))];   \
> +        char dummytmp2[sizeof(*(x)) - 4];   \
> +        (*(x)) = lswapl(*(x));              \
> +    } while (0)
>
>  /* byte swap a short */
> -#define swaps(x, n) (*(x)) = lswaps(*(x))
> +#define swaps(x, n) do {                    \
> +        char dummytmp1[2 - sizeof(*(x))];   \
> +        char dummytmp2[sizeof(*(x)) - 2];   \
> +        (*(x)) = lswaps(*(x));              \
> +    } while (0)
>
> And this extra guard check already spots at least one issue:
>
> swaprep.c:436:2: error: size of array 'dummytmp2' is too large
>
> http://cgit.freedesktop.org/xorg/xserver/tree/dix/swaprep.c?id=xorg-server-1.10.1#n423
>
> One more potential pitfall is data alignment. The older variant could
> work correctly with any alignment, but the new variant will fail if
> the pointer is somehow not naturally aligned. So I think the patch is
> very dangerous and every affected line of code needs to be carefully
> reviewed.
>
> --
> Best regards,
> Siarhei Siamashka

Good call!

I wonder if these misalignments are bugs in their own right? Maybe we
should just use __builtin_bswap32 instead.

I think Keith's suggestion to make to use an inline function sounds
good. I suppose I'll tack on a patch before this series that makes the
lswap[ls] macros inline functions.

Thanks!
Matt