rapidfuzz / RapidFuzz

Rapid fuzzy string matching in Python using various string metrics
https://rapidfuzz.github.io/RapidFuzz/
MIT License
2.61k stars 116 forks source link

ppc(32): `process_cpp_impl.cpython-311-powerpc-linux-gnu.so: undefined symbol: __atomic_compare_exchange_8` #354

Closed mgorny closed 9 months ago

mgorny commented 9 months ago

When RapidFuzz is compiled on ppc32, it fails to load because of missing symbols, e.g.:

E   ImportError: /var/tmp/portage/dev-python/rapidfuzz-3.5.2/work/rapidfuzz-3.5.2-python3_11/install/usr/lib/python3.11/site-packages/r
apidfuzz/process_cpp_impl.cpython-311-powerpc-linux-gnu.so: undefined symbol: __atomic_compare_exchange_8

Apparently the current CMake checks to detect the necessity of -latomic are insufficient since they all pass:

//Test HAVE_CXX_ATOMICS_INT_WITHOUT_LIB
HAVE_CXX_ATOMICS_INT_WITHOUT_LIB:INTERNAL=1
//Test HAVE_CXX_ATOMICS_SIZE_T_WITHOUT_LIB
HAVE_CXX_ATOMICS_SIZE_T_WITHOUT_LIB:INTERNAL=1
//Test HAVE_CXX_ATOMICS_UNSIGNED_WITHOUT_LIB
HAVE_CXX_ATOMICS_UNSIGNED_WITHOUT_LIB:INTERNAL=1
//Test HAVE_CXX_ATOMICS_VOID_PTR_WITHOUT_LIB
HAVE_CXX_ATOMICS_VOID_PTR_WITHOUT_LIB:INTERNAL=1

I guess yet another type needs to be tested. I'm afraid I don't really know how to figure out where the atomic calls are coming from, so I don't know which type.

maxbachmann commented 9 months ago

Hm my problem is that I do not really have a system to test these issues. I would probably test:

1) does it work when manually adding -latomic 2) The only other atomic type I could find in taskflow is std::atomic<bool>, so maybe adding a check for this helps 3) if that doesn't help maybe the detection macro itself doesn't work properly and has to test doing some things with the atomics?

maxbachmann commented 9 months ago

Ah and I do not really remember, but this did work in the past right? If it did, when did it break?

Given the fact that it does compile + link, but fails at importing I am not even sure whether I can detect this using a pure compile check :thinking: I mean presumably that check would succeed similar to the way the compilation as a whole did succeed. E.g. here: https://github.com/grpc/grpc/blob/2d3c7afe253982e49735097a86d083bfd220d00a/tools/distrib/python/grpcio_tools/setup.py#L86 they appear to test compile and then run the test program. No clue how that works when cross compiling though ...

maxbachmann commented 9 months ago

One option might be to invert the logic: 1) try to build with -latomic worst case it's unused (possible use as-needed to get rid of it again if unused) 2) try to build without if not available

Not sure whether that approach has it's own problems.

mgorny commented 9 months ago

It's too late for me today but if I don't forget, I'll try to dig some more tomorrow. That said, I don't think bool would work. It's clearly trying to use 64-bit atomics, and all of the types being tested right now are 32-bit. I suspect it's coming from somewhere deeper but I'll have to dig.

maxbachmann commented 9 months ago

This was my initial understanding with the _u8 version as well, but those are at least not explicitly used. I did check for int64_t in the past, but this caused issues in the x86 builds for netbsd: https://github.com/maxbachmann/RapidFuzz/issues/342

mgorny commented 9 months ago

Ok, so I've played a bit with -Wl,-z,defs and the problem clearly comes from taskflow. I see some instances of std::atomic<uint64_t> there, so I guess that's the problem.

Unless I'm mistaken, from the linked thread I gather that the -latomic problem was an issue on NetBSD end.

maxbachmann commented 9 months ago

Hm you are right I completely missed the std::atomic<uint64_t> in taskflow. And yes reading through the NetBsd issue again I think I misunderstood this back then and they just forgot to add the possibility to add -latomic.

So this sounds like I should just check atomic<uint64_t> support as well.

maxbachmann commented 9 months ago

https://github.com/maxbachmann/RapidFuzz/commit/3e8e563de2c54d2ecc71fb02ea3a3e1a2b8d4dc0 tests atomic again. Are you able to test if this fixes your issue?

mgorny commented 9 months ago

Yes, I can confirm that the module loads now. Thanks!