On Thu, Apr 28, 2011 at 2:33 AM, Matt Turner <matts...@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 _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel