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

glaubitz · 2022-12-09T11:57:46Z

Signed-off-by: John Paul Adrian Glaubitz glaubitz@physik.fu-berlin.de

…api-src#980) Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

pavelkumbrasev · 2023-01-09T13:53:35Z

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

glaubitz · 2023-01-09T13:56:30Z

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 · 2023-01-09T14:02:24Z

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 · 2023-01-09T15:23:07Z

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 · 2023-01-09T15:40:35Z

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 · 2023-01-09T15:44:37Z

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 · 2023-01-09T15:53:56Z

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 · 2023-01-10T09:25:23Z

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 · 2023-01-10T10:00:15Z

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 · 2023-01-10T11:37:27Z

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 · 2023-01-10T16:00:06Z

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: MariaDB/server@7de110a

and: MariaDB/server@f502ccb

barracuda156 · 2023-02-08T11:16:23Z

It would be nice to have this merged.

glaubitz · 2023-05-31T09:24:24Z

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 · 2023-05-31T09:52:50Z

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?

isaevil · 2023-06-01T08:30:18Z

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 <atomic>
+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 <atomic>

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 <atomic>

int main() {

std::atomic<uint64_t> w;

return ++w;

}

" TBB_BUILDS_WITHOUT_LIBATOMIC)

Can you check this code sample again?

glaubitz · 2023-06-01T08:46:44Z

@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 <atomic> +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 <atomic> -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 <atomic> + 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.

Summary: RISC-V is a growing platform, and Folly and commonly used dependency. We want to make sure that any projects that relies on Folly can be ported to RISC-V. I'm verifying that everything builds and run correctly by cross-compiling it locally and passing the test suite using QEMU. Pull Request resolved: #2104 Reviewed By: yfeldblum Differential Revision: D51691588 Pulled By: Orvid fbshipit-source-id: 9c4ae6718ffbf6b55432bf6e1ffba06311c7d345

glaubitz mentioned this pull request Dec 9, 2022

Build fails on 32-bit PowerPC due to missing linking against libatomic #980

Open

glaubitz force-pushed the libatomic-fix branch from c544a08 to faea2c5 Compare December 9, 2022 12:22

glaubitz changed the title ~~Add cmake check for libatomic requirement when building with gcc~~ Add cmake check for libatomic requirement when building with gcc (#980) Dec 9, 2022

This was referenced Dec 11, 2022

onetbb: add ppc32 support, fix build for Darwin PPC (updated & rebased) #840

Open

Multiple testsuite failures on Linux ia64 #984

Open

barracuda156 mentioned this pull request Jan 3, 2023

Fixes for older macOS and atomic builtins detection facebook/folly#1907

Open

Add cmake check for libatomic requirement when building with gcc (one…

c828ae4

…api-src#980) Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>

glaubitz force-pushed the libatomic-fix branch from faea2c5 to c828ae4 Compare January 9, 2023 12:02

barracuda156 mentioned this pull request Jan 16, 2023

Support for 32-bit platforms: 94% of tests pass; need to fix serial.scan kokkos/kokkos#5769

Closed

isaevil reviewed Jun 1, 2023

View reviewed changes

barracuda156 mentioned this pull request Jan 12, 2024

[Bug]: Build fails with -Werror on 32-bit platform facebookincubator/dispenso#25

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

glaubitz commented Dec 9, 2022 •

edited

pavelkumbrasev commented Jan 9, 2023

glaubitz commented Jan 9, 2023

pavelkumbrasev commented Jan 9, 2023

glaubitz commented Jan 9, 2023

glaubitz commented Jan 9, 2023

pavelkumbrasev commented Jan 9, 2023 •

edited

glaubitz commented Jan 9, 2023

pavelkumbrasev commented Jan 10, 2023

glaubitz commented Jan 10, 2023

pavelkumbrasev commented Jan 10, 2023

glaubitz commented Jan 10, 2023

barracuda156 commented Feb 8, 2023

glaubitz commented May 31, 2023 •

edited

pavelkumbrasev commented May 31, 2023 •

edited

isaevil Jun 1, 2023

glaubitz commented Jun 1, 2023 via email

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

Are you sure you want to change the base?

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

Conversation

glaubitz commented Dec 9, 2022 • edited

pavelkumbrasev commented Jan 9, 2023

glaubitz commented Jan 9, 2023

pavelkumbrasev commented Jan 9, 2023

glaubitz commented Jan 9, 2023

glaubitz commented Jan 9, 2023

pavelkumbrasev commented Jan 9, 2023 • edited

glaubitz commented Jan 9, 2023

pavelkumbrasev commented Jan 10, 2023

glaubitz commented Jan 10, 2023

pavelkumbrasev commented Jan 10, 2023

glaubitz commented Jan 10, 2023

barracuda156 commented Feb 8, 2023

glaubitz commented May 31, 2023 • edited

pavelkumbrasev commented May 31, 2023 • edited

isaevil Jun 1, 2023

Choose a reason for hiding this comment

glaubitz commented Jun 1, 2023 via email

glaubitz commented Dec 9, 2022 •

edited

pavelkumbrasev commented Jan 9, 2023 •

edited

glaubitz commented May 31, 2023 •

edited

pavelkumbrasev commented May 31, 2023 •

edited