Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hash.verification test failure on ppc64/s390x #72

Closed
QuLogic opened this issue Mar 20, 2018 · 5 comments
Closed

hash.verification test failure on ppc64/s390x #72

QuLogic opened this issue Mar 20, 2018 · 5 comments
Labels

Comments

@QuLogic
Copy link
Contributor

QuLogic commented Mar 20, 2018

Firstly, I had to modify the CMakeLists.txt to remove the -march=native before the C++14 check which would fail on ppc64(le)/s390x.

Secondly, I get this test error in hash.verification on ppc64 and s390x. I'm guessing this is because of some little-endian assumption somewhere:

/builddir/build/BUILD/xtl-0.4.4/test/test_xhash.cpp:157: Failure
      Expected: actual
      Which is: 2413447565
To be equal to: res
      Which is: 520960004
@SylvainCorlay
Copy link
Member

Thanks for the report. Indeed there seems to be an endianess issue.

@QuLogic
Copy link
Contributor Author

QuLogic commented Aug 8, 2018

Could perhaps some of these warnings be relevant (from x86_64, though)?

In file included from /builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:15:
/builddir/build/BUILD/xtl-0.4.13/include/xtl/xhash.hpp: In function 'std::size_t xtl::detail::murmur_hash(const void*, std::size_t, std::size_t) [with long unsigned int N = 4; std::size_t = long unsigned int]':
/builddir/build/BUILD/xtl-0.4.13/include/xtl/xhash.hpp:68:33: warning: conversion to 'std::size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
                 hash ^= data[2] << 16;
                         ~~~~~~~~^~~~~
/builddir/build/BUILD/xtl-0.4.13/include/xtl/xhash.hpp:70:33: warning: conversion to 'std::size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
                 hash ^= data[1] << 8;
                         ~~~~~~~~^~~~
/builddir/build/BUILD/xtl-0.4.13/include/xtl/xhash.hpp: In function 'std::size_t xtl::detail::murmur_hash(const void*, std::size_t, std::size_t) [with long unsigned int N = 8; std::size_t = long unsigned int]':
/builddir/build/BUILD/xtl-0.4.13/include/xtl/xhash.hpp:105:48: warning: unsigned conversion from 'int' to 'std::size_t' {aka 'long unsigned int'} changes value from '-8' to '18446744073709551608' [-Wsign-conversion]
             const char* end = data + (length & ~0x7);
                                                ^~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp: In function 'void xtl::rand_p(void*, int)':
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:60:34: warning: conversion to 'uint32_t' {aka 'unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
             blocks[0] = std::rand();
                         ~~~~~~~~~^~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp: In function 'void xtl::flipbit(void*, int, uint32_t)':
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:75:24: warning: conversion to 'int' from 'uint32_t' {aka 'unsigned int'} may change the sign of the result [-Wsign-conversion]
         int byte = bit >> 3;
                    ~~~~^~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:79:21: warning: conversion from 'int' to 'uint8_t' {aka 'unsigned char'} may change value [-Wconversion]
             b[byte] ^= (1 << bit);
             ~~~~~~~~^~~~~~~~~~~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp: In instantiation of 'uint32_t xtl::verification_test(F, int) [with F = long unsigned int (*)(const void*, long unsigned int, long unsigned int); uint32_t = unsigned int]':
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:155:77:   required from here
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:29:42: warning: conversion to 'size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
         std::memset(hashes, 0, hashbytes * 256);
                                ~~~~~~~~~~^~~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:30:29: warning: conversion to 'size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
         std::memset(res, 0, hashbytes);
                             ^~~~~~~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:37:56: warning: conversion to 'std::size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
             *reinterpret_cast<std::size_t*>(hashes + i * hashbytes) = f(key,i,256-i);
                                                      ~~^~~~~~~~~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:41:66: warning: conversion to 'long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
         *reinterpret_cast<std::size_t*>(res) = f(hashes,hashbytes*256,0);
                                                         ~~~~~~~~~^~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:45:80: warning: conversion to 'uint32_t' {aka 'unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
         uint32_t verification = (res[0] << 0) | (res[1] << 8) | (res[2] << 16) | (res[3] << 24);
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp: In instantiation of 'bool xtl::sanity_test(F, int) [with F = long unsigned int (*)(const void*, long unsigned int, long unsigned int)]':
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:162:9:   required from here
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:112:43: warning: conversion to 'size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
                     std::memcpy(key2,key1,len);
                                           ^~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:114:69: warning: conversion to 'long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
                     *reinterpret_cast<std::size_t*>(hash1) = f(key1,len,0);
                                                                     ^~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:119:42: warning: conversion to 'uint32_t' {aka 'unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
                         flipbit(key2,len,bit);
                                          ^~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:120:73: warning: conversion to 'long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
                         *reinterpret_cast<std::size_t*>(hash2) = f(key2,len,0);
                                                                         ^~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:122:52: warning: conversion to 'size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
                         if(std::memcmp(hash1,hash2,hashbytes) == 0)
                                                    ^~~~~~~~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:127:42: warning: conversion to 'uint32_t' {aka 'unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
                         flipbit(key2,len,bit);
                                          ^~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:128:73: warning: conversion to 'long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
                         *reinterpret_cast<std::size_t*>(hash2) = f(key2,len,0);
                                                                         ^~~
/builddir/build/BUILD/xtl-0.4.13/test/test_xhash.cpp:130:52: warning: conversion to 'size_t' {aka 'long unsigned int'} from 'int' may change the sign of the result [-Wsign-conversion]
                         if(std::memcmp(hash1,hash2,hashbytes) != 0)
                                                    ^~~~~~~~~

@QuLogic
Copy link
Contributor Author

QuLogic commented Aug 8, 2018

Actually, seems to be a limitation of MurmurHash2.

@QuLogic
Copy link
Contributor Author

QuLogic commented Aug 8, 2018

It may be possible to use the MurmurHash3 implementation by Shane Day that's used in the R digest package, or maybe xxHash, which both claim to be more portable.

@SylvainCorlay
Copy link
Member

@QuLogic thanks. Indeed, MurmurHash3 gives the same results regardless of the architecture.

However, for our use of murmurhash2, we don't really need it to give the same results on all machines, and modifying the test to check for the expected result on big endian architecture should be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants