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

[Siarhei Siamashka]

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