FTBFS on 32-bit architectures

[lamby]

See, for example:

https://buildd.debian.org/status/fetch.php?pkg=redisearch&arch=i386&ver=0.90.0~alpha1-1&stamp=1509653366&raw=0

cc -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wall -Wno-unused-function -Wno-unused-variable -Wno-unused-result -fPIC -D_GNU_SOURCE -std=gnu99 -I"/<<PKGBUILDDIR>>/src" -DREDIS_MODULE_TARGET -DREDISMODULE_EXPERIMENTAL_API -Wl,-z,relro -Wl,-z,now -g -ggdb -O3  -Wdate-time -D_FORTIFY_SOURCE=2  -c /<<PKGBUILDDIR>>/src/buffer.c -o /<<PKGBUILDDIR>>/src/buffer.o -MMD -MF /<<PKGBUILDDIR>>/src/buffer.d
cc -g -O2 -fdebug-prefix-map=/<<PKGBUILDDIR>>=. -fstack-protector-strong -Wformat -Werror=format-security -Wall -Wno-unused-function -Wno-unused-variable -Wno-unused-result -fPIC -D_GNU_SOURCE -std=gnu99 -I"/<<PKGBUILDDIR>>/src" -DREDIS_MODULE_TARGET -DREDISMODULE_EXPERIMENTAL_API -Wl,-z,relro -Wl,-z,now -g -ggdb -O3  -Wdate-time -D_FORTIFY_SOURCE=2  -c /<<PKGBUILDDIR>>/src/tokenize.c -o /<<PKGBUILDDIR>>/src/tokenize.o -MMD -MF /<<PKGBUILDDIR>>/src/tokenize.d
In file included from /<<PKGBUILDDIR>>/src/inverted_index.h:4:0,
                 from /<<PKGBUILDDIR>>/src/inverted_index.c:2:
/<<PKGBUILDDIR>>/src/redisearch.h:12:9: error: unknown type name '__uint128_t'
 typedef __uint128_t t_fieldMask;
         ^~~~~~~~~~~

[lamby]

This is being tracked within @Debian as https://bugs.debian.org/880801.

[mnunberg]

Maybe it's time to implement the same using uint64_t

[dvirsky]

I'll limit to 64 fields in 32 bit architectures. I've never considered that people might want to use it with 32 bit archs, but it's a small change, so no problem.

@lamby BTW you know that redisearch will not work in ARM architectures at all? (I haven't tried but it shouldn't - a lot of the encoding uses unaligned access).

[dvirsky]

@lamby should be okay now in master

[dvirsky]

@lamby should be okay now in master

[dvirsky]

There seems to be some problem with github comments. In case this time it succeeds - master works in 32 bit mode, and I've altered the tests a bit to pass in that mode as well (they didn't, prior to this change)

[lamby]

@dvirsky Really? 32-bit archs are stil used quite extensively, especially on small VMs where not having 64-bit pointers/cache is kinda useful...

Re. ARM, that's fine (but somewhat regrettable). As long as it fails to build, rather than it builds fine and then fails in production. ;-)

[dvirsky]

Fixed in master, and tested on a 32 bit compilation target, but not a 32 bit machine. I should drop a line in the docs about fields limit in 32 bit archs.

…

On Sun, Nov 5, 2017 at 10:32 AM Chris Lamb ***@***.***> wrote: @dvirsky <https://github.com/dvirsky> Really? 32-bit archs are stil used quite extensively, especially on small VMs where not having 64-bit pointers/cache is kinda useful... Re. ARM, that's fine (but somewhat regrettable). As long as it fails to build, rather than it builds fine and then fails in production. ;-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#208 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAetuv-lh5d0vItY5n0gI6lk2CIuOoHQks5szXKwgaJpZM4QSJ65> .

[dvirsky]

I'm actually not sure it will fail automatically, we might want to add this check and fail completely. I have a brand new Raspberry Pi sitting here, I might need to fire it up soon :)

Anyway, I'll submit a patch to the 64 bit thing soon, I don't have a 32 bit machine here, I'll let you know and you can run the tests on it.

[dvirsky]

Oh, comments are back. for a while I couldn't comment.
I've modified and tested the code (had to modify a few tests. they didn't pass even before the change) an it works now.

[mnunberg]

I mean, we can always make it work on ARM if we really wanted to, just a bunch of ifdefs and maybe an arch-dependent memcpy. Would be slower on ARM, of course.

@dvirsky what about the RDB format between 128b and 64b fields -- would it change? do we want it to be compatible?

[dvirsky]

@mnunberg making stuff ARM compatible would be much harder, especially in the trie. It took Salvatore a lot of time to get his trie impl to work well in arm and it made it much less readable.

Re RDB format - as long as you have under 64 fields everything should work as expected. Over 64 fields - and you will get bits erased if you switch a 64 bit rdb to a 32 bit one. We might want to document it and make sure we're telling the truth, but I would not bother beyond that. Maybe fail the whole RDB load in such a case?