eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.57k stars 373 forks source link

Is `libatomic` still required to build Iceoryx? #2295

Closed t0ny-peng closed 1 month ago

t0ny-peng commented 1 month ago

Brief feature description

It seems libatomic is required for iceoryx_hoof if built for LINUX. I'm curious is this still required? I've been testing with GCC8 on x86_64 and the compiler generates the correct atomic instructions in the output binary.

https://github.com/eclipse-iceoryx/iceoryx/blob/661eee549fe597efab6636ebcc644cea8777ea24/iceoryx_hoofs/CMakeLists.txt#L207

Detailed information

In the change log, it's stated that libatomic is added for RHEL. Is that still the case now? Thanks!

  • Add libatomic to dependency list in iceoryx_utils for RHEL

The reason I'm asking this is because libatomic isn't a standard package, e.g., some Clang toolchains doesn't provide this and only GCC ones do.

Thanks!

elBoberido commented 1 month ago

@t0ny-peng there is a good chance it isn't required anymore. We had to use this for GCC 5.4 with it's poor C++11 support. But since we do not support GCC 5.4 anymore there is a good chance that all our targets would build without the flag.

You could try to remove that flag and create a PR to check if everything builds. If that's the case, then there is no reason to not remove it.

t0ny-peng commented 1 month ago

Thanks @elBoberido. Unfortunately as I tested with aarch64 Clang18 and GCC11, both compilers seem to generate assembly code to call, e.g., __aarch64_swp4_acq_rel and other libatomic library symbols, unless -mno-outline-atomics flag is set in the compiler command. So removing libatomic will break many builds.

   44db0:       ff 25 8a 2e 00 00       jmp    *0x2e8a(%rip)        # 47c40 <__atomic_is_lock_free@Base>

https://godbolt.org/z/nfs8jETK7

However I haven't found a way to disable generating __atomic_is_lock_free on Clang12 x86_64 platform. Any thought?

elBoberido commented 1 month ago

Okay, then we need to keep libatomic. I assumed that the functionality meanwhile moved into libstdc++ but it seems to not be the case. May I ask which toolchain does not provide libatomic?

t0ny-peng commented 1 month ago

We are using an internal toolchain that unfortunately does not provide libatomic.a/libatomic.so.

@elBoberido Since this is the only place where is_lock_free is used, can we move such check to the test so that the libiceoryx_hoofs.so itself doesn't contain undefined symbol __atomic_is_lock_free if compiled with Clang?

image

And, what's the purpose of forcing m_head here to be lock free? On an imaginary platform where std::atomic<{struct-of-8-bytes}> isn't lock free, do we expect Iceoryx to still work with less performance, or do we simply don't expect it to work at all?

elfenpiff commented 1 month ago

@t0ny-peng

And, what's the purpose of forcing m_head here to be lock free? On an imaginary platform where std::atomic<{struct-of-8-bytes}> isn't lock free, do we expect Iceoryx to still work with less performance, or do we simply don't expect it to work at all?

Iceoryx will still compile and run, but the problem is the shared memory. A non-lock-free atomic in shared memory will use a process local mutex to ensure its thread safety. Assume you have two different processes working on the same atomic; they both use their own individual process-local mutex in an inter-process context, making the atomic non-thread-safe.

This causes data races and undefined behavior, resulting in erratic bugs everywhere.

elBoberido commented 1 month ago

@t0ny-peng unfortunately it turned out that users do not always compile or run the tests on their hardware and this is a critical requirements since data races will be introduced if it is used although the check fails. We will have to add even more of these checks.

A workaround for you would be to just create a dummy libatomic with a dummy __atomic_is_lock_free function.

Another workaround would be to abstract this code away into the platform layer and create a custom platform for your target. This will be more work than the dummy libatomic.

The last resort would be to carry a small patch for your target which removes this check.

t0ny-peng commented 1 month ago

Thanks @elBoberido and @elfenpiff.

Since our project targets specific platform where we know a priori that atomic ops are always lock free, the easiest solution for us is to remove that check.

Please consider this issue resolved. Thank you!