Re: murmurhash3 test failures on big-endian systems
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
Re: murmurhash3 test failures on big-endian systems
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...
Re: murmurhash3 test failures on big-endian systems
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*
Re: murmurhash3 test failures on big-endian systems
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
Re: murmurhash3 test failures on big-endian systems
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
Re: murmurhash3 test failures on big-endian systems
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
Re: murmurhash3 test failures on big-endian systems
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
diff --git a/src/lib/murmurhash3.c b/src/lib/murmurhash3.c
index 45dcc22..d0336a1 100644
--- a/src/lib/murmurhash3.c
+++ b/src/lib/murmurhash3.c
@@ -94,6 +94,8 @@ void murmurhash3_32 (const void *key, size_t len, uint32_t
seed,
h1 = fmix32(h1);
+ h1 = cpu32_to_be(h1);
+
memcpy(out, &h1, sizeof(h1));
}
@@ -206,6 +208,9 @@ void murmurhash3_128(const void *key, size_t len, uint32_t
seed,
h1 += h2;
h2 += h1;
+ h1 = cpu64_to_be(h1);
+ h2 = cpu64_to_be(h2);
+
memcpy(out, &h1, sizeof(h1));
memcpy(out+sizeof(h1), &h2, sizeof(h2));
}
@@ -323,6 +328,11 @@ void murmurhash3_128(const void *key, size_t len, uint32_t
seed,
h1 += h2; h1 += h3; h1 += h4;
h2 += h1; h3 += h1; h4 += h1;
+ h1 = cpu32_to_be(h1);
+ h2 = cpu32_to_be(h2);
+ h3 = cpu32_to_be(h3);
+ h4 = cpu32_to_be(h4);
+
memcpy(out, &h1, sizeof(h1));
memcpy(out+sizeof(h1), &h2, sizeof(h2));
memcpy(out+sizeof(h1)*2, &h3, sizeof(h3));
diff --git a/src/lib/test-murmurhash3.c b/src/lib/test-murmurhash3.c
index 9da3d28..4848fbd 100644
--- a/src/lib/test-murmurhash3.c
+++ b/src/lib/test-murmurhash3.c
@@ -7,7 +7,19 @@ struct murmur3_test_vectors {
const char *input;
size_t len;
uint32_t seed;
- uint32_t result[4]; /* fits all results */
+
+ /* murmurhash3_128() produces a different output on ILP32 and LP64
+ systems (by design). Therefore, we must use different expected
+ results based on what system we're on. We define both all the
+ time, but use the below pre-processor magic to select which
+ version we'll use. */
+ uint8_t result_ilp32[MURMURHASH3_128_RESULTBYTES]; /* fits all results
*/
+ uint8_t result_lp64[MURMURHASH3_128_RESULTBYTES]; /* fits all results */
+#ifdef _LP64
+#define result result_lp64
+#else
+#define result result_ilp32
+#endif
};
static void test_murmurhash3_algorithm(const char *name,
@@ -29,24 +41,49 @@ static void test_murmurhash3_algorithm(const char *name,
static void test_murmurhash3_32(void)
{
+ /* murmurhash3_32() produces the same output on both ILP32 and LP64
+ systems, so use the same expected outputs for both */
struct murmur3_test_vectors vectors[] = {
- { "", 0, 0, { 0, 0, 0, 0}},
- { "", 0, 0x1, { 0x514E28B7, 0, 0, 0 }},
- { "", 0, 0x, { 0x81F16F39, 0, 0, 0 }},
- { "\0\0\0\0", 4, 0, { 0x2362F9DE, 0, 0, 0 }},
- { "", 4, 0x9747b28c, { 0x5A97808A, 0, 0, 0 }},
- { "aaa", 3, 0x9747b28c, { 0x283E0130, 0, 0, 0 }},
- { "aa", 2, 0x9747b28c, { 0x5D211726, 0, 0, 0 }},
- { "a", 1, 0x9747b28c, { 0x7FA09EA6, 0, 0, 0 }},
- { "abcd", 4, 0x9747b28c, { 0xF0478627, 0, 0, 0 }},
- { "abc", 3, 0x9747b28c, { 0xC84A62DD, 0, 0, 0 }},
- { "ab", 2, 0x9747b28c, { 0x74875592, 0, 0, 0 }},
- { "Hello, world!", 13, 0x9747b28c, { 0x24884CBA, 0, 0, 0 }},
+ { "", 0, 0, { 0, }, { 0, } },
+ { "", 0, 0x1,
+ { 0x51, 0x4E, 0x28, 0xB7, },
+ { 0x51, 0x4E, 0x28, 0xB7, } },
+ { "", 0, 0x,
+ { 0x81, 0xF1, 0x6F, 0x39, },
+ { 0x81, 0xF1, 0x6F, 0x39, } },
+ { "\0\0\0\0", 4, 0,
+ { 0x23, 0x62, 0xF9, 0xDE, },
+ { 0x23, 0x62, 0xF9, 0xDE, } },
+ { "", 4, 0x9747b28c,
+ { 0x5A, 0x97, 0x80, 0x8A, },
+ { 0x5A, 0x97, 0x80, 0x8A, } },
+ { "aaa", 3, 0x9747b28c,
+ { 0x28, 0x3E, 0x01, 0x30, },
+ { 0x28, 0x3E, 0x01, 0x30, } },
+ { "aa", 2, 0x9747b28c,
+ { 0x5D, 0x21, 0x17, 0x26, },
+ { 0x5D, 0x21, 0x17, 0x26, } },
+ { "a", 1, 0x9747b28c,
+ { 0x7F, 0xA0, 0x9E, 0xA6, },
+ { 0x7F, 0xA0, 0x9E, 0xA6, } },
+ { "abcd", 4, 0x9747b28c,
+ { 0xF0, 0x47, 0x86, 0x27, },
+ { 0xF0, 0x47, 0x86, 0x27, } },
+ { "abc", 3, 0x9747b28c,
+ { 0xC8, 0x4A, 0x62, 0xDD, },
+ { 0xC8, 0x4A, 0x62, 0xDD, } },
+ { "ab", 2, 0x9747b28c,
+ { 0x74, 0x87, 0x55, 0x92, },
+ { 0x74, 0x87, 0x55, 0x92, } },
+ { "Hello, world!", 13, 0x9747b28c,
+ { 0x
Re: murmurhash3 test failures on big-endian systems
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
Re: murmurhash3 test failures on big-endian systems
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
