Build failed in Jenkins: cyrus-imapd-master #717

[Greg Banks]

On Wed, Jul 18, 2012, at 11:42 PM, Bron Gondwana wrote:
> On Wed, Jul 18, 2012, at 05:03 PM, Jenkins wrote:
> > Running unit tests
> > ./cunit-to-junit.pl: ran 278 tests, 2 failed
> > make[3]: *** [check-local] Error 1
> > make[3]: Leaving directory `<http://ci.cyrusimap.org/job/cyrus-imapd-master/ws/imapd'>
> > make[2]: *** [check-am] Error 2
> > make[2]: Leaving directory `<http://ci.cyrusimap.org/job/cyrus-imapd-master/ws/imapd'>
> > make[1]: *** [check-recursive] Error 1
> > make[1]: Leaving directory `<http://ci.cyrusimap.org/job/cyrus-imapd-master/ws/imapd'>
> > make: *** [check] Error 2
> > + fatal 'Can'\''t make check'
> > + echo 'imapd/tools/jenkins-build.sh: Can'\''t make check'
> > imapd/tools/jenkins-build.sh: Can't make check
> 
> Of course it bloody runs clean on my machine.  Short of logging in and...
> 
> ./cunit/byteorder64.testc:12: CU_ASSERT_EQUAL(ntohll(*((uint64_t
> *)(b64)))=72057594037927936,1=1)
> 
> Oh shit.
> 
> So what's different from my machine?  The architecture certainly isn't. 
> This means that cmu/master is super-dangerous right now for anyone not
> building on the same platform as me.
> 
> Greg - any ideas??  It should be using be64toh from endian.h on Linux
> according to the code.

Hmm.

static void test_byteorder(void)
{
    const char b64[8] = { 0, 0, 0, 0, 0, 0, 0, 1 };
    const char b32[4] = { 0, 0, 0, 1 };

    char i64[8];
    char i32[4];

Here's your first problem, the alignment of all these variables is
uncontrolled.  The code is only working by accident because the C
runtime environment gives you an aligned stack, and also because x86
doesn't care so much about alignment.

    /* test 64 bit */
    CU_ASSERT_EQUAL(ntohll(*((uint64_t *)(b64))), 1);
    *((uint64_t *)i64) = htonll(1);
    CU_ASSERT_EQUAL(memcmp((char *)i64, b64, 8), 0);

    /* test 32 bit */
    CU_ASSERT_EQUAL(ntohl(*((uint32_t *)(b32))), 1);
    *((uint32_t *)i32) = htonl(1);
    CU_ASSERT_EQUAL(memcmp((char *)i32, b32, 4), 0);
}

In the header file lib/byteorder64.h

/* http://stackoverflow.com/a/4410728/94253 */

#if defined(__linux__)
#  include <endian.h>
#elif defined(__FreeBSD__) || defined(__NetBSD__)
#  include <sys/endian.h>
#elif defined(__OpenBSD__)
#  include <sys/types.h>
#  define be16toh(x) betoh16(x)
#  define be32toh(x) betoh32(x)
#  define be64toh(x) betoh64(x)
#elif defined(WORDS_BIGENDIAN)
#define CYRUS_BYTESWAP
#endif

That could do with some improvement.

Solaris also has an optimised ntohll() in libc which uses the 64b bswap
instruction, although it's not inlined.

http://cvs.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/lib/libc/amd64/gen/byteorder.s

and gcc has a builtin which uses the instruction on platforms which have
it

http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-g_t_005f_005fbuiltin_005fbswap64-3333

which presumably will work on some platforms where Linux's <endian.h>
(which re-implements the same thing differently) is not available.

Disassembling cunit/byteorder64.o, I don't see any bswap instructions in
there, but nm shows undefined symbols for libc's htonl and ntohl. 
Writing a function that calls ntohll() and nothing else, it seems that
ntohll() expands to a no-op.  Running gcc -E confirms this.

Aha..

#ifdef be64toh
#define htonll(x) htobe64(x)
#define ntohll(x) be64toh(x)
#elif defined (CYRUS_BYTESWAP)
/* little-endian 64-bit host/network byte-order swap functions */
extern unsigned long long _htonll(unsigned long long);
extern unsigned long long _ntohll(unsigned long long);
#define htonll(x) _htonll(x)
#define ntohll(x) _ntohll(x)
#else
#define htonll(x) (x)
#define ntohll(x) (x)
#endif

The <endian.h> on my desktop defines a be64toh().  The same header on
ci.cyrusimap.org doesn't.

-- 
Greg.