Fix Build on illumos / Solaris

[SIN3R6Y]

On illumos and Solaris, endian.h does not define _BIG_ENDIAN, but rather only defines __BIG_ENDIAN. Also, endian.h is located in /usr/include/ast/endian.h compared to /usr/include/endian.h as expected by the generic unix macro.

Sorry for the previous bad pull request, I applied an improper patch. This request contains the proper changes.

[svaarala]

Hi guys, I've added svaarala/duktape#453 to ensure this fix (or its equivalent) makes it into the next Duktape release. The generation of duk_config.h will be much more modular in the next release, so adding platforms should become much easier.

[judofyr]

v1.3.0.4 includes this fix and has been tagged and released.

[SIN3R6Y]

@svaarala yes i would like to see that. Not that duk_config.h is bad, but there are areas that are hard to read / disorganized. Once you figure out what's going on it's easy though.

[svaarala]

Hmm, duk_config.h tries to accept both _BIG_ENDIAN and __BIG_ENDIAN, for example: https://github.com/svaarala/duktape-releases/blob/v1.3.0/src/duk_config.h#L1331-L1332

Could you paste the error what happens when you don't define _BIG_ENDIAN but still use the ast/endian.h header?

[SIN3R6Y]

This results in an (_BYTE_ORDER == _BIG_ENDIAN) operator '==' has no right operand error regarding line 1357

#elif defined(__BYTE_ORDER) && defined(__BIG_ENDIAN) && (__BYTE_ORDER == __BIG_ENDIAN) || \
      defined(_BYTE_ORDER) && defined(_BIG_ENDIAN) && (_BYTE_ORDER == _BIG_ENDIAN) || \
      defined(__BIG_ENDIAN__)

The first comparison, __BYTE_ORDER == __BIG_ENDIAN, is true, but the second "==" comparison is still evaluated even though the if statement is already true. So when it gets to the next comparison the statement fails, because _BIG_ENDIAN is not defined.

So the only way this statement works is if both __BIG_ENDIAN and _BIG_ENDIAN are defined, otherwise one of the == comparisons will fail.

The clean way of fixing this, in my opinion, would be to figure out whether __BYTE_ORDER, _BYTE_ORDER, __BIG_ENDIAN, and _BIG_ENDIAN are defined first. Then based on those results make "==" comparisons. I.E. if only one set is defined, then only test that set, if both are defined then test both.

If that sounds right to you, i can make another patch implementing that logic.

[svaarala]

Ok, now I know what's going on :)

That undefined value thing is only an issue on some older compilers AFAIK; others seem to handle it using short circuiting so that they don't barf like that. For example, it's not an issue on GCC.

This is actually one cleanup item for duk_config.h, i.e. ensure all comparisons are inside an #ifdef wrapper. So this should get fixed properly when that's done.

[SIN3R6Y]

@svaarala FWIW, i am using GCC 4.8.5, which isn't that old.

[svaarala]

That's interesting. I was thinking maybe the order of comparison matters, but it seems not. Using gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04):

Test 1, compiles and works correctly:

#include <stdio.h>

#define FOO 123
#undef BAR

int main(int argc, char *argv[]) {
#if defined(FOO) && defined(BAR) && (FOO == BAR)
    printf("FOO == BAR\n");
#else
    printf("FOO != BAR\n");
#endif
    return 0;
}

Test 2, with order of comparison changed, still compiles and works correctly:

#include <stdio.h>

#define FOO 123
#undef BAR

int main(int argc, char *argv[]) {
#if defined(FOO) && defined(BAR) && (BAR == FOO)
    printf("FOO == BAR\n");
#else
    printf("FOO != BAR\n");
#endif
    return 0;
}

I wonder what happens when you compile these?

[SIN3R6Y]

@svaarala Both of those tests succeed for me as well.

root@build:~/test# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/local/libexec/gcc/sparc-sun-solaris2.11/4.8.5/lto-wrapper
Target: sparc-sun-solaris2.11
Configured with: ../gcc-4.8.5/configure --disable-bootstrap --enable-languages=c,c++,fortran,go,objc,obj-c++ --with-as=/usr/bin/as --with-ld=/usr/bin/ld --without-gnu-as --without-gnu-ld
Thread model: posix
gcc version 4.8.5 (GCC)

So i did a little more investigating, and it turns out that _BIG_ENDIAN is defined, but it's defined with no value.

So i did this....

#include <stdio.h>

#define FOO 123
#define BAR

int main(int argc, char *argv[]) {
#if defined(FOO) && defined(BAR) && (FOO == BAR)
    printf("FOO == BAR\n");
#else
    printf("FOO != BAR\n");
#endif
    return 0;
}

and i got this back...

root@build:~/test# gcc test1.c
test1.c: In function 'main':
test1.c:7:48: error: operator '==' has no right operand
 #if defined(FOO) && defined(BAR) && (FOO == BAR)
                                                ^

EDIT: _BIG_ENDIAN is not defined in endian.h, so i am not sure where it's being defined with no value. I also checked endian.h's includes.

[svaarala]

Ok, thanks. Now we at least know what the actual preprocessor trigger is :)

[SIN3R6Y]

@svaarala What about pulling this information from GCC?

root@build:~/test# gcc -dM -E - <<<'' | grep ENDIAN
#define __ORDER_LITTLE_ENDIAN__ 1234
#define __FLOAT_WORD_ORDER__ __ORDER_BIG_ENDIAN__
#define __ORDER_PDP_ENDIAN__ 3412
#define __ORDER_BIG_ENDIAN__ 4321
#define __BYTE_ORDER__ __ORDER_BIG_ENDIAN__

GCC should always know what the byte order is, but maybe other compilers don't provide this information by default?

This could also be useful for anyone cross compiling for architectures with different endians.

EDIT: Note, im using SPARC here. This is the result from an x86 box.

root@x86build:~# gcc -dM -E - <<<'' | grep ENDIAN
#define __ORDER_LITTLE_ENDIAN__ 1234
#define __FLOAT_WORD_ORDER__ __ORDER_LITTLE_ENDIAN__
#define __ORDER_PDP_ENDIAN__ 3412
#define __ORDER_BIG_ENDIAN__ 4321
#define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__

Duktape probably has more uses than Rails, but in this case i can't see anyone not using GCC. illumos is GCC only, and Oracle recommends using GCC for any software not built specifically for Sun Studio (Oracle's compiler). I imagine other Unix vendors have the same view, as most Linux software falls flat on their face without GCC or Clang, and sometimes even with Clang.

[svaarala]

I can add these to the byte order detection code.

[svaarala]

Oh, actually those are already used (later in duk_config.h):

/* GCC and Clang provide endianness defines as built-in predefines, with
 * leading and trailing double underscores (e.g. __BYTE_ORDER__).  See
 * output of "make gccpredefs" and "make clangpredefs".  Clang doesn't
 * seem to provide __FLOAT_WORD_ORDER__.
 * http://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
 */
#if !defined(DUK_F_BYTEORDER) && defined(__BYTE_ORDER__)
#if defined(__ORDER_LITTLE_ENDIAN__) && (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
/* Integer little endian */
#if defined(__FLOAT_WORD_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && \
    (__FLOAT_WORD_ORDER__ == __ORDER_LITTLE_ENDIAN__)
#define DUK_F_BYTEORDER 1
#elif defined(__FLOAT_WORD_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && \
      (__FLOAT_WORD_ORDER__ == __ORDER_BIG_ENDIAN__)
#define DUK_F_BYTEORDER 2
#elif !defined(__FLOAT_WORD_ORDER__)
/* Float word order not known, assume not a hybrid. */
#define DUK_F_BYTEORDER 1
[...]

Maybe they could be checked first though. I guess it doesn't really matter if the preprocessor equality comparisons are #ifdef guarded.

[SIN3R6Y]

@svaarala Maybe it would make more sense to have it check for GCC / Clang first, then if that fails check endian.h? As most of us use GCC / Clang that should solve any issues with this on any os.

[svaarala]

I agree, although assuming that the #include is in place and the preprocessor comparisons are #ifdef guarded, that shouldn't make a difference. But I'll change it to work like that in any case.

Edit: it would also be possible to move the endianness include into the byte order detection part. But since you can't "trial include" a header, the platform detection ladder would need to be replicated there to be sure which header is safe to include. This wouldn't be very nice, so that's why I was assuming above that the endianness header is in place already.

[svaarala]

Come to think of it, the #ifdef guards don't solve the problem. Even with the following there will be a comparison error because a comparison argument is empty:

#define FOO 123
#define BAR

#if defined(FOO) && defined(BAR)
#if (FOO == BAR)
/* ... */
#endif
#endif

So I'll change the gcc/clang check to be first; the less safe check is then avoided because the whole block will be skipped. With the more modular duk_config.h the gcc/clang byte order check can be in the compiler specific snippet, and this generic fallback can be avoided entirely.