Apollon Oikonomopoulos
2018-Mar-27 10:15 UTC
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...