search

valkey-io / valkey

A flexible distributed key-value datastore that is optimized for caching and other realtime workloads.
https://valkey.io
Other
17.06k stars 636 forks source link

[BUG] 32-bit arch may need linking to libatomic, otherwise build fails #434

Open barracuda156 opened 6 months ago

barracuda156 commented 6 months ago

Describe the bug

Nothing checks for support of 8-byte atomics in hardware. So linking to libatomic is not done where required.

To reproduce

Build on ppc or arm.

Expected behavior

-latomic should be passed when needed.

Additional information

/opt/local/var/macports/build/_opt_PPCSnowLeopardPorts_databases_valkey/valkey/work/compwrap/cc/opt/local/bin/gcc-mp-13 -Wl,-headerpad_max_install_names -L/opt/local/lib -lMacportsLegacySupport -arch ppc -O3 -flto -g -ggdb -L/usr/local/opt/openssl/lib -o valkey-benchmark ae.o anet.o valkey-benchmark.o adlist.o dict.o zmalloc.o redisassert.o release.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o strl.o ../deps/hiredis/libhiredis.a ../deps/hdr_histogram/libhdrhistogram.a ../deps/jemalloc/lib/libjemalloc.a -lm -ldl ../deps/hiredis/libhiredis_ssl.a -L/opt/local/libexec/openssl3/lib -lssl -L/opt/local/libexec/openssl3/lib -lcrypto 
ld: warning: directory '/usr/local/opt/openssl/lib' following -L not found
Undefined symbols:
  "___atomic_fetch_add_8", referenced from:
      _hdr_record_values_atomic in libhdrhistogram.a(hdr_histogram.o)
      _hdr_record_values_atomic in libhdrhistogram.a(hdr_histogram.o)
  "___atomic_load_8", referenced from:
      _hdr_record_values_atomic in libhdrhistogram.a(hdr_histogram.o)
      _hdr_record_values_atomic in libhdrhistogram.a(hdr_histogram.o)
  "___atomic_compare_exchange_8", referenced from:
      _hdr_record_values_atomic in libhdrhistogram.a(hdr_histogram.o)
      _hdr_record_values_atomic in libhdrhistogram.a(hdr_histogram.o)
ld: symbol(s) not found
collect2: error: ld returned 1 exit status
madolson commented 4 months ago

@PingXie Can we close this, it seems like Neal fixed this?

barracuda156 commented 4 months ago

@PingXie If you mean this https://github.com/valkey-io/valkey/commit/71dd85dc5a89a4e07d837efa14937990b01e317f then no, not yet. uname -m returns Power Macintosh on Darwin, not ppc. The latter is a build arch, not machine type.

By the way, it should not be needed when building for ppc64 (any OS), but that patch seems to apply to either case on Linux.

If we do not mind redundant linking on ppc64, then uname -p returns powerpc (or after all Power*Macintosh can be checked for with uname -m).

PingXie commented 4 months ago

I see. I can't test the fix. @barracuda156 can you please help review #663?