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

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...