Add cmake check for libatomic requirement when building with gcc (#980)

[glaubitz]

Signed-off-by: John Paul Adrian Glaubitz [email protected]

[pavelkumbrasev]

Hi @glaubitz why such code sample is chosen? (For build check)

[glaubitz]

Hi @glaubitz why such code sample is chosen? (For build check)

Because it's a very simple code that allows us to check whether the target platform requires libatomic for atomic operations.

If you have a better idea, I am open to suggestions, of course.

[pavelkumbrasev]

To be honest, it looks like a attempt to guess missing functionality :)

int main() {
  std::atomic<int> test_val{};
  return ++test_val;
}

Would be easier to read. Is there guarantee that compiler will not optimize this code and throw atomics away?

[glaubitz]

To be honest, it looks like a attempt to guess missing functionality :)

int main() {
  std::atomic<int> test_val{};
  return ++test_val;
}

Would be easier to read. Is there guarantee that compiler will not optimize this code and throw atomics away?

I will test your code and update my PR if it works with yours.

[glaubitz]

OK, so your suggested variant actually doesn't work:

FAILED: gnu_12.2_cxx11_32_relwithdebinfo/test_tick_count
: && /usr/bin/c++ -O2 -g -DNDEBUG -rdynamic test/CMakeFiles/test_tick_count.dir/tbb/test_tick_count.cpp.o -o gnu_12.2_cxx11_32_relwithdebinfo/test_tick_count  -Wl,-rpath,/home/glaubitz/oneTBB/build/gnu_12.2_cxx11_32_relwithdebinfo  gnu_12.2_cxx11_32_relwithdebinfo/libtbb.so.12.9  -ldl && :
/usr/bin/ld: gnu_12.2_cxx11_32_relwithdebinfo/libtbb.so.12.9: undefined reference to `__atomic_fetch_sub_8'
/usr/bin/ld: gnu_12.2_cxx11_32_relwithdebinfo/libtbb.so.12.9: undefined reference to `__atomic_load_8'
/usr/bin/ld: gnu_12.2_cxx11_32_relwithdebinfo/libtbb.so.12.9: undefined reference to `__atomic_fetch_add_8'
collect2: error: ld returned 1 exit status

The reason is that your code does not perform any tests with 64-bit atomics which is why the test succeeds even without -latomic.

[pavelkumbrasev]

So, the original approach seems to be a kinda guessing because this features might be implemented and we need different operation for example on 128 atomics or some RMW operation. And still the question is: Is there guarantee that compiler will not optimize this code and throw atomics away? - because it will always compile and link

[glaubitz]

So, the original approach seems to be a kinda guessing because this features might be implemented and we need different operation for example on 128 atomics or some RMW operation.

So, you think this should also include a test for 128-bit atomics? Are these supported natively on most 64-bit architectures?

And still the question is: Is there guarantee that compiler will not optimize this code and throw atomics away? - because it will always compile

Doesn't seem to be the case, see: https://godbolt.org/z/7cTcxsW84

[pavelkumbrasev]

So, you think this should also include a test for 128-bit atomics? Are these supported natively on most 64-bit architectures?

By that I mean we don't have specification that states which operations would be inside libatomic. We are playing in guessing game and this is not a best approach.

Doesn't seem to be the case, see: https://godbolt.org/z/7cTcxsW84

There is no call in assembly how it would lead to the link error?

[glaubitz]

So, you think this should also include a test for 128-bit atomics? Are these supported natively on most 64-bit architectures?

By that I mean we don't have specification that states which operations would be inside libatomic. We are playing in guessing game and this is not a best approach.

Well, if you have a 32-bit processor, you usually need libatomic for 64-bit atomics.

Normally, gcc would link against libatomic where needed but it doesn't because of this bug:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81358

Doesn't seem to be the case, see: https://godbolt.org/z/7cTcxsW84

There is no call in assembly how it would lead to the link error?

Well, you simply try to compile the sample code without -latomic. If it fails, you know that you need to link against libatomic.

It's a simple test that is commonly used by many open source projects in their build scripts such as mariadb.

[pavelkumbrasev]

Well, if you have a 32-bit processor, you usually need libatomic for 64-bit atomics.

Normally, gcc would link against libatomic where needed but it doesn't because of this bug:

But which operation and types we should test? What will be minimal test? For example proposed by me test doesn't work as expected.

Well, you simply try to compile the sample code without -latomic. If it fails, you know that you need to link against libatomic.

Compiler might optimized this test and build it successfully and later failed on library build because of different operation usage.

It's a simple test that is commonly used by many open source projects in their build scripts such as mariadb.

Could you please provide a link on such solutions?

[glaubitz]

Well, if you have a 32-bit processor, you usually need libatomic for 64-bit atomics. Normally, gcc would link against libatomic where needed but it doesn't because of this bug:

But which operation and types we should test? What will be minimal test? For example proposed by me test doesn't work as expected.

Well, the tests I currently suggested work for the current code. Whether it's 100% future-proof, I can't say.

Well, you simply try to compile the sample code without -latomic. If it fails, you know that you need to link against libatomic.

Compiler might optimized this test and build it successfully and later failed on library build because of different operation usage.

I don't think it would. The code cannot be really optimized as the variables w1 are non-deterministic.

It's a simple test that is commonly used by many open source projects in their build scripts such as mariadb.

Could you please provide a link on such solutions?

See: https://github.com/MariaDB/server/pull/979/commits/7de110af8c25697855ac4925b2c91f757b356655

and: https://github.com/MariaDB/server/commit/f502ccbcb5dfce29067434885a23db8d1bd5f134

[barracuda156]

It would be nice to have this merged.

[glaubitz]

It would be nice to have this merged.

Yes, that would be nice. But I have given up hope. I don't think this will ever get merged. ¯\(ツ)/¯

[pavelkumbrasev]

It would be nice to have this merged.

Yes, that would be nice. But I have given up hope. I don't think this will ever get merged. ¯_(ツ)_/¯

Let's try to move with this one :)

Please add comment that will state that this is a workaround because as you stated before GCC normally automatically links against libatomic on 32 bit. @isaevil Do you see any problems with such approach?

[glaubitz]

@isaevil commented on this pull request.

In cmake/compilers/GNU.cmake:

+# Check whether code with full atomics can be built without libatomic +# see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81358 +include(CheckCXXSourceCompiles) +check_cxx_source_compiles("#include +int main() {

• std::atomic<uint8_t> w1;
• std::atomic<uint16_t> w2;
• std::atomic<uint32_t> w4;
• std::atomic<uint64_t> w8;
• return ++w1 + ++w2 + ++w4 + ++w8; +}" TBB_BUILDS_WITHOUT_LIBATOMIC)

⬇️ Suggested change -# Check whether code with full atomics can be built without libatomic -# see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81358 -include(CheckCXXSourceCompiles) -check_cxx_source_compiles("#include -int main() {

• std::atomic<uint8_t> w1;
• std::atomic<uint16_t> w2;
• std::atomic<uint32_t> w4;
• std::atomic<uint64_t> w8;
• return ++w1 + ++w2 + ++w4 + ++w8; -}" TBB_BUILDS_WITHOUT_LIBATOMIC) +# Check whether code with full atomics can be built without libatomic +# Workaround for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81358 +include(CheckCXXSourceCompiles) +check_cxx_source_compiles("

• #include
• int main() {

•    std::atomic<uint64_t> w;
  

•    return ++w;
  

• } +" TBB_BUILDS_WITHOUT_LIBATOMIC)

Can you check this code sample again?Message ID: @.***> Just a second. I’m not at the computer yet.

[chewi]

Gentoo is now applying this patch in its package.