dovecot - Mar 2018 - 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

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