Endianness check doesn't work on s390

[GoogleCodeExporter]

WebP uses this check to detect system endianness:

  #if !defined(__BIG_ENDIAN__)

However, that macro is not defined on some big endian systems, like s390.

According to 
<https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html#Common-Predefi
ned-Macros>, to detect endianness, you should use this check instead:

  #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__

I am attaching a patch from Fedora that changes the check to be correct.

See for details: https://bugzilla.redhat.com/show_bug.cgi?id=962091#c33

Original issue reported on code.google.com by Mitya57 on 5 Jun 2014 at 8:56

Attachments:

• libwebp-0.4.0-endian-check.patch

[GoogleCodeExporter]

thanks for the patch.
There was already some extra-code in bit_reader_inl.h to try and define 
__BIG_ENDIAN__ when missing, but
i guess explicitly going for the  "__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__" 
test is better.
And all the endian-related stuff should go in its own endian.h header, btw. We 
have too much of these
scattered all over the place.

I wonder, though, how much are this test and #define's related to gnu/gcc 
exclusively?
Is it the recommended (POSIX?) way of testing endianness?

Original comment by pascal.m...@gmail.com on 5 Jun 2014 at 3:09

• Changed state: Accepted

[GoogleCodeExporter]

I don't think that macro is documented in any standard.

If you want to support compilers that don't define __BYTE_ORDER__, it is always 
possible to manually check byte order (using int* -> char* conversion).

Original comment by Mitya57 on 5 Jun 2014 at 3:17

[GoogleCodeExporter]

As a configure check that would cover everything except a cross-compile. We 
have an is_big_endian() function in lossless, but within the bit reader better 
to avoid a runtime test.

Original comment by jz...@google.com on 6 Jun 2014 at 3:23

[GoogleCodeExporter]

Then you can use __BYTE_ORDER__ and fall back to the current logic if it is not 
defined.

Original comment by Mitya57 on 6 Jun 2014 at 4:35

[GoogleCodeExporter]

Yes, AC_C_BIGENDIAN would help here. While we're autoconf'ing a test for 
endian.h could be added too.

Original comment by jz...@google.com on 6 Jun 2014 at 7:34

[GoogleCodeExporter]

The combination of some configure changes and the addition of endian_inl.h 
should have addressed this. Works with mips/ppc big-endian builds, I don't have 
a s390 setup to test that configuration.

6781423 configure: check for __builtin_bswapXX()
e458bad endian_inl.h: implement htoleXX with BSwapXX
f2664d1 endian_inl.h: add BSwap16
380cca4 configure.ac: add AC_C_BIGENDIAN
ee70a90 endian_inl.h: add BSwap64
47779d4 endian_inl.h: add BSwap32
d5104b1 utils: add endian_inl.h

Original comment by jz...@google.com on 23 Jul 2014 at 1:00

• Changed state: Started

[GoogleCodeExporter]

Thanks. I either don't have access to s390, but the current code in 
endian_inl.h looks fine to me.

Original comment by Mitya57 on 25 Jul 2014 at 5:56

[GoogleCodeExporter]

Thanks for the update, I'll close this for now.

Original comment by jz...@google.com on 26 Jul 2014 at 12:22

• Changed state: Fixed