murmurhash3 test failures on big-endian systems
Hi,
The dovecot 2.3.0.1 Debian package currently fails to build on all big-endian architectures[1], due to murmurhash3 tests failing. The relevant output from e.g. s390x is:
test-murmurhash3.c:22: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_32) ......................................... : FAILED test-murmurhash3.c:22: Assert(#1) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#2) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#3) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#4) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#5) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#6) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#7) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#9) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#10) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#13) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_128) ........................................ : FAILED
Looks like the murmurhash3 implementation in Dovecot is currently broken on big-endian systems.
Regards, Apollon
[1] https://buildd.debian.org/status/package.php?p=dovecot&suite=experimental
On 26.03.2018 15:49, Apollon Oikonomopoulos wrote:
Hi,
The dovecot 2.3.0.1 Debian package currently fails to build on all big-endian architectures[1], due to murmurhash3 tests failing. The relevant output from e.g. s390x is:
test-murmurhash3.c:22: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_32) ......................................... : FAILED test-murmurhash3.c:22: Assert(#1) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#2) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#3) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#4) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#5) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#6) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#7) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#9) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#10) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#13) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_128) ........................................ : FAILED
Looks like the murmurhash3 implementation in Dovecot is currently broken on big-endian systems.
Regards, Apollon
[1] https://buildd.debian.org/status/package.php?p=dovecot&suite=experimental Hi!
Thanks for reporting this, we'll look at it. It's not going to get fixed on 2.3.1 though, but we can provide a patch for that.
Aki
Hi Aki,
On 15:55 Mon 26 Mar , Aki Tuomi wrote:
On 26.03.2018 15:49, Apollon Oikonomopoulos wrote:
Hi,
The dovecot 2.3.0.1 Debian package currently fails to build on all big-endian architectures[1], due to murmurhash3 tests failing. The relevant output from e.g. s390x is:
test-murmurhash3.c:22: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_32) ......................................... : FAILED test-murmurhash3.c:22: Assert(#1) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#2) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#3) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#4) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#5) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#6) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#7) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#9) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#10) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:22: Assert(#13) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_128) ........................................ : FAILED
Looks like the murmurhash3 implementation in Dovecot is currently broken on big-endian systems.
Regards, Apollon
[1] https://buildd.debian.org/status/package.php?p=dovecot&suite=experimental Hi!
Thanks for reporting this, we'll look at it. It's not going to get fixed on 2.3.1 though, but we can provide a patch for that.
I'd be happy to test the patch, thanks!
Apollon
On Mon, Mar 26, 2018 at 15:57:01 +0300, Apollon Oikonomopoulos wrote: ...
I'd be happy to test the patch, thanks!
Ok, try the attached patch. (It is a first pass at the issue, so it may not be the final diff that'll end up getting committed. It'd be good to know if it actually fixes the issue for you - sadly, I don't have a big endian system to play with.)
Thanks,
Jeff.
-- The obvious mathematical breakthrough would be development of an easy way to factor large prime numbers. - Bill Gates, The Road Ahead, pg. 265
Hi,
On 12:55 Mon 26 Mar , Josef 'Jeff' Sipek wrote:
On Mon, Mar 26, 2018 at 15:57:01 +0300, Apollon Oikonomopoulos wrote: ...
I'd be happy to test the patch, thanks!
Ok, try the attached patch. (It is a first pass at the issue, so it may not be the final diff that'll end up getting committed. It'd be good to know if it actually fixes the issue for you - sadly, I don't have a big endian system to play with.)
Thanks for the quick response!
Unfortunately still fails, although with fewer assertion errors than before:
test-murmurhash3.c:34: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:34: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:34: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_32) ......................................... : FAILED test-murmurhash3.c:34: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_128) ........................................ : FAILED
Regards, Apollon
On 11:31 Tue 27 Mar , Apollon Oikonomopoulos wrote:
Hi,
On 12:55 Mon 26 Mar , Josef 'Jeff' Sipek wrote:
On Mon, Mar 26, 2018 at 15:57:01 +0300, Apollon Oikonomopoulos wrote: ...
I'd be happy to test the patch, thanks!
Ok, try the attached patch. (It is a first pass at the issue, so it may not be the final diff that'll end up getting committed. It'd be good to know if it actually fixes the issue for you - sadly, I don't have a big endian system to play with.)
Thanks for the quick response!
Unfortunately still fails, although with fewer assertion errors than before:
test-murmurhash3.c:34: Assert(#8) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:34: Assert(#11) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 test-murmurhash3.c:34: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_32) ......................................... : FAILED test-murmurhash3.c:34: Assert(#12) failed: memcmp(result, vectors[i].result, sizeof(result)) == 0 murmurhash3 (murmurhash3_128) ........................................ : FAILED
It turns out there's a missing byte-inversion when loading the blocks which should be addressed in getblock{32,64}. Murmurhash treats each block as an integer expecting little-endian storage. Applying this additional change fixes the build on s390x (and does not break it on x864_64): --- b/src/lib/murmurhash3.c +++ b/src/lib/murmurhash3.c @@ -23,7 +23,7 @@ static inline uint32_t getblock32(const uint32_t *p, int i) { - return p[i]; + return cpu32_to_le(p[i]); } //----------------------------------------------------------------------------- @@ -105,7 +105,7 @@ static inline uint64_t getblock64(const uint64_t *p, int i) { - return p[i]; + return cpu64_to_le(p[i]); } Regards, Apollon
On 13:05 Tue 27 Mar , Apollon Oikonomopoulos wrote:
On 11:31 Tue 27 Mar , Apollon Oikonomopoulos wrote: It turns out there's a missing byte-inversion when loading the blocks which should be addressed in getblock{32,64}. Murmurhash treats each block as an integer expecting little-endian storage. Applying this additional change fixes the build on s390x (and does not break it on x864_64):
--- b/src/lib/murmurhash3.c +++ b/src/lib/murmurhash3.c @@ -23,7 +23,7 @@
static inline uint32_t getblock32(const uint32_t *p, int i) { - return p[i]; + return cpu32_to_le(p[i]);
… or perhaps le32_to_cpu, although it should be the same in the end.
}
//----------------------------------------------------------------------------- @@ -105,7 +105,7 @@
static inline uint64_t getblock64(const uint64_t *p, int i) { - return p[i]; + return cpu64_to_le(p[i]); }
Regards, Apollon
On Tue, Mar 27, 2018 at 13:15:31 +0300, Apollon Oikonomopoulos wrote:
On 13:05 Tue 27 Mar , Apollon Oikonomopoulos wrote:
On 11:31 Tue 27 Mar , Apollon Oikonomopoulos wrote: It turns out there's a missing byte-inversion when loading the blocks which should be addressed in getblock{32,64}. Murmurhash treats each block as an integer expecting little-endian storage. Applying this additional change fixes the build on s390x (and does not break it on x864_64):
--- b/src/lib/murmurhash3.c +++ b/src/lib/murmurhash3.c @@ -23,7 +23,7 @@
static inline uint32_t getblock32(const uint32_t *p, int i) { - return p[i]; + return cpu32_to_le(p[i]);
… or perhaps le32_to_cpu, although it should be the same in the end.
Right. I'm going get the changes reviewed & committed. I'll ping you when there is a commit with the "official" fix. Thanks, Jeff.
}
//----------------------------------------------------------------------------- @@ -105,7 +105,7 @@
static inline uint64_t getblock64(const uint64_t *p, int i) { - return p[i]; + return cpu64_to_le(p[i]); }
Regards, Apollon
-- *NOTE: This message is ROT-13 encrypted twice for extra protection*
On Tue, Mar 27, 2018 at 08:46:20 -0400, Josef 'Jeff' Sipek wrote:
On Tue, Mar 27, 2018 at 13:15:31 +0300, Apollon Oikonomopoulos wrote:
On 13:05 Tue 27 Mar , Apollon Oikonomopoulos wrote:
On 11:31 Tue 27 Mar , Apollon Oikonomopoulos wrote: It turns out there's a missing byte-inversion when loading the blocks which should be addressed in getblock{32,64}. Murmurhash treats each block as an integer expecting little-endian storage. Applying this additional change fixes the build on s390x (and does not break it on x864_64):
--- b/src/lib/murmurhash3.c +++ b/src/lib/murmurhash3.c @@ -23,7 +23,7 @@
static inline uint32_t getblock32(const uint32_t *p, int i) { - return p[i]; + return cpu32_to_le(p[i]);
… or perhaps le32_to_cpu, although it should be the same in the end.
Right.
I'm going get the changes reviewed & committed. I'll ping you when there is a commit with the "official" fix.
Ok, there are two commits: 35497604d80090a02619024aeec069b32568e4b4 and 5522b8b3d3ed1a99c3b63bb120216af0bd427403 Together, they should be identical to the patch I sent the other day plus your fixup. Let me know if you have any problems. Jeff. -- A CRAY is the only computer that runs an endless loop in just 4 hours...
On 15:55 Wed 28 Mar , Josef 'Jeff' Sipek wrote:
Ok, there are two commits:
35497604d80090a02619024aeec069b32568e4b4 and 5522b8b3d3ed1a99c3b63bb120216af0bd427403
Together, they should be identical to the patch I sent the other day plus your fixup. Let me know if you have any problems.
Indeed, there are no differences, but I updated the patch in the package to include the commit IDs nevertheless.
Also, dovecot now seems to build fine on all big-endian architectures[1].
Thanks again, Apollon
[1] https://buildd.debian.org/status/package.php?p=dovecot&suite=experimental
participants (3)
-
Aki Tuomi
-
Apollon Oikonomopoulos
-
Josef 'Jeff' Sipek