Support risc-v architecture
#598
Conversation
|
It would be better to check if libatomic is necessary before linking against it. The |
|
I just noticed that |
|
Updated. Link |
| # This isn't necessary on MSVC, so avoid command-line switch annoyance | ||
| # by only running on GCC-like hosts. | ||
| if (LLVM_COMPILER_IS_GCC_COMPATIBLE) | ||
| # First check if atomics work without the library. | ||
| check_working_cxx_atomics(HAVE_CXX_ATOMICS_WITHOUT_LIB) | ||
| # If not, check if the library exists, and atomics work with it. | ||
| if(NOT HAVE_CXX_ATOMICS_WITHOUT_LIB) | ||
| check_library_exists(atomic __atomic_fetch_add_4 "" HAVE_LIBATOMIC) |
There was a problem hiding this comment.
I'd like to make some further explanation here.
CHECK_LIBRARY_EXISTS won't work if the function to look for has a built-in version. The test program will cause a signature conflict thus fail to compile, leading to a false negative. This line and related branches are removed to avoid that.
|
I reverted the changes in CMakeLists because adding the library is supposed to be automatic through CMAKE_REQUIRED_LIBRARIES if I understand correctly. |
|
@aberaud In fact, in my previous tests, Could we get some error messages on macOS? I didn't find it from the CI. This change is not expected to affect macOS, maybe there are some bugs and I will try to fix it :) |
A |
|
Looking at various examples as well as the original CheckAtomic from llvm, it seems like using CMAKE_REQUIRED_LIBRARIES is how people do it: The error on macOS: cmake: build: |
|
@aberaud I did some investigation and I now understand how everything is going on.
I wrote a minimal example for testing and the result is that Quoted from the documentation of
The code in the StackOverflow question you mentioned also implies my point. Quoted from the StackOverflow question code: # Before setting CMAKE_REQUIRED_FLAGS, we preserve the current state
cmake_push_check_state()
set(CMAKE_REQUIRED_LIBRARIES "-L${atomic_lib_dir} -latomic")
check_cxx_source_compiles("${ATOMIC32_TEST_CODE}" atomic32_test_with_atomic_linking)
check_cxx_source_compiles("${ATOMIC64_TEST_CODE}" atomic64_test_with_atomic_linking)
cmake_pop_check_state()I treat the Quoted from the documentation of
This is because the Let's take a look at the code snippet: # This isn't necessary on MSVC, so avoid command-line switch annoyance
# by only running on GCC-like hosts.
if (LLVM_COMPILER_IS_GCC_COMPATIBLE)
# First check if atomics work without the library.
check_working_cxx_atomics(HAVE_CXX_ATOMICS_WITHOUT_LIB)
# If not, check if the library exists, and atomics work with it.
if(NOT HAVE_CXX_ATOMICS_WITHOUT_LIB)
list(APPEND CMAKE_REQUIRED_LIBRARIES "atomic")
check_working_cxx_atomics(HAVE_CXX_ATOMICS_WITH_LIB)
if (NOT HAVE_CXX_ATOMICS_WITH_LIB)
message(FATAL_ERROR "Host compiler must support std::atomic!")
endif()
endif()
endif()The When if (NOT HAVE_CXX_ATOMICS_WITHOUT_LIB)
target_link_libraries(opendht PUBLIC atomic)
endif ()always links to Here's an same bug report from another project: CheckAtomic doesn't set LLVM_COMPILER_IS_GCC_COMPATIBLE and fails check on Apple Clang I would wait for the macOS CI of this repo to work properly and then open a new PR. If you have any questions let me know :) |
|
Thanks for the detailed analysis and explanations !
|
Of course, it should work.
Nice! I will open a PR later. |
Currently this project cannot be compiled for
risc-varchitecture,gcccompiler raises an error:This is because the
risc-varchitecture does not support 1-byte and 2-byte lock-free atomic operations.By passing the
-latomicoption,gccwill statically link thelibatomiclibrary. Inside that library, thestd::atomic<bool>used in the project will fallback to the mutex implementation, which is allowed by C++ standard.Ref: riscv-collab/riscv-gnu-toolchain#183 (comment)