Re: [PATCH] Ensure __BYTE_ORDER is always set

From: Jeff King <peff@peff.net>
Date: Jan 30, 2014, 20:45 UTC
Message-ID: <20140130204538.GA1130@sigill.intra.peff.net>
In-Reply-To: <1391111741-28994-1-git-send-email-brian@gernhardtsoftware.com>

On Thu, Jan 30, 2014 at 02:55:41PM -0500, Brian Gernhardt wrote:

Show 13 quoted lines

> a201c20 (ewah: support platforms that require aligned reads) added a
> reliance on the existence of __BYTE_ORDER and __BIG_ENDIAN.  However,
> these macros are spelled without the leading __ on some platforms (OS
> X at least).  In this case, the endian-swapping code was added even
> when unnecessary, which caused assertion failures in
> t5310-pack-bitmaps.sh as the code that used the bitmap would read past
> the end.
> 
> We already had code to handle this case in compat/bswap.h, but it was
> only used if we couldn't already find a reasonable version of bswap64.
> Move the macro-defining and checking code out of a conditional so that
> either __BYTE_ORDER is defined or we get a compilation error instead
> of a runtime error in the bitmap code.

Thanks, this makes sense, and matches the assumption that a201c20 made.

I do find the failure mode interesting. The endian-swapping code kicked in when it did not, meaning your are on a big-endian system. Is this on an ancient PPC Mac? Or is the problem that the code did not kick in when it should?

Either way, we should perhaps be more careful in the bitmap code, too, that the values we get are sensible. It's better to die("your bitmap is broken") than to read off the end of the array. I can't seem to trigger the same failure mode, though. On my x86 system, turning off the endian-swap (i.e., the opposite of what should happen) makes t5310 fail, but it is because we end up trying to set the bit very far into a dynamic bitfield, and die allocating memory.

-Peff