[PATCH] D69869: [clang-tools-extra] fix the check for if '-latomic' is necessary
gokturk created this revision. gokturk added reviewers: ilya-biryukov, nridge, kadircet, beanz, compnerd. Herald added subscribers: cfe-commits, usaxena95, s.egerton, lenary, PkmX, jfb, arphaman, jkorous, simoncook, mgorny. Herald added a project: clang. The CheckAtomic module performs two tests to determine if passing '-latomic' to the linker is required: one for 64-bit atomics, and another for non-64-bit atomics. clangd only uses the result from HAVE_CXX_ATOMICS64_WITHOUT_LIB. This is incomplete because there are uses of non-64-bit atomics in the code, such as the ReplyOnce::Replied of type std::atomic defined in clangd/ClangdLSPServer.cpp. It is possible to have a requirement for an explicit '-latomic' for non-64-bit atomics and not have for atomics64. One example is the RISC-V rv64 (64-bit) architecture with the 'A' (atomic) extension, where the host compiler gcc will convert any 64-bit atomic operation to their hardware assembly equivalents, giving the impression that '-latomic' is not required, while failing on 8-bit atomic operations as there is no hardware support for that and linking against libatomic is necessary. As a matter of fact, clang-tools-extra (commit 5c40544fa40bfb85ec888b6a03421b3905e4a4e7) fails to compile with: /usr/lib/gcc/riscv64-unknown-linux-gnu/9.1.0/../../../../riscv64-unknown-linux-gnu/bin/ld: tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o: in function `.L0 ' : ClangdLSPServer.cpp:(.text._ZN5clang6clangd15ClangdLSPServer14MessageHandler9ReplyOnceclEN4llvm8ExpectedINS4_4json5ValueEEE[_ZN5clang6clangd15ClangdLSPServer14MessageHandler9ReplyOnceclEN4llvm8ExpectedINS4_4json5ValueEEE]+0x2a): undefined reference to `__atomic_exchange_1' collect2: error: ld returned 1 exit status Fix by also checking for the result of HAVE_CXX_ATOMICS_WITHOUT_LIB. See also: https://reviews.llvm.org/D68964 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D69869 Files: clang-tools-extra/clangd/CMakeLists.txt Index: clang-tools-extra/clangd/CMakeLists.txt === --- clang-tools-extra/clangd/CMakeLists.txt +++ clang-tools-extra/clangd/CMakeLists.txt @@ -30,7 +30,7 @@ endif() set(CLANGD_ATOMIC_LIB "") -if(NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB) +if(NOT HAVE_CXX_ATOMICS_WITHOUT_LIB OR NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB) list(APPEND CLANGD_ATOMIC_LIB "atomic") endif() Index: clang-tools-extra/clangd/CMakeLists.txt === --- clang-tools-extra/clangd/CMakeLists.txt +++ clang-tools-extra/clangd/CMakeLists.txt @@ -30,7 +30,7 @@ endif() set(CLANGD_ATOMIC_LIB "") -if(NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB) +if(NOT HAVE_CXX_ATOMICS_WITHOUT_LIB OR NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB) list(APPEND CLANGD_ATOMIC_LIB "atomic") endif() ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69869: [clang-tools-extra] fix the check for if '-latomic' is necessary
gokturk added a comment.
Let me try my best to explain what's happening. The goal of this check is to
determine whether passing `-latomic` is required.
Let's start with the code used by `check_working_cxx_atomics64`
(https://github.com/llvm/llvm-project/blob/master/llvm/cmake/modules/CheckAtomic.cmake)
#include
#include
std::atomic x (0);
int main() {
uint64_t i = x.load(std::memory_order_relaxed);
return 0;
}
If we compile this with `g++ -o atomic64 -march=rv64imafd atomic64.cpp`, it
produces the following assembly output:
06bc :
6bc: fe010113addisp,sp,-32
6c0: 00113c23sd ra,24(sp)
6c4: 00813823sd s0,16(sp)
6c8: 02010413addis0,sp,32
6cc: fe042023sw zero,-32(s0)
6d0: fe042703lw a4,-32(s0)
6d4: 000107b7lui a5,0x10
6d8: fff78593addia1,a5,-1 #
<__global_pointer$+0xd7ff>
6dc: 00070513mv a0,a4
6e0: 04efjal ra,720
<_ZStanSt12memory_orderSt23__memory_order_modifier>
6e4: 00050793mv a5,a0
6e8: fef42223sw a5,-28(s0)
6ec: 2797auipc a5,0x2
6f0: 97478793addia5,a5,-1676 # 2060
6f4: 0ffffence
6f8: 0007b783ld a5,0(a5)
6fc: 0ffffence
700: 0013nop
704: fef43423sd a5,-24(s0)
708: 0793li a5,0
70c: 00078513mv a0,a5
710: 01813083ld ra,24(sp)
714: 01013403ld s0,16(sp)
718: 02010113addisp,sp,32
71c: 8067ret
0720 <_ZStanSt12memory_orderSt23__memory_order_modifier>:
720: fe010113addisp,sp,-32
724: 00813c23sd s0,24(sp)
728: 02010413addis0,sp,32
72c: 00050793mv a5,a0
730: 00058713mv a4,a1
734: fef42623sw a5,-20(s0)
738: 00070793mv a5,a4
73c: fef42423sw a5,-24(s0)
740: fec42703lw a4,-20(s0)
744: fe842783lw a5,-24(s0)
748: 00f777b3and a5,a4,a5
74c: 0007879bsext.w a5,a5
750: 0007879bsext.w a5,a5
754: 00078513mv a0,a5
758: 01813403ld s0,24(sp)
75c: 02010113addisp,sp,32
760: 8067ret
Ignoring the fine details of the RISC-V assembly, if we just focus on `jal`
instructions that are used for function calls, we see that no actual calls to
libatomic are being made. This may be a separate bug of a false-positive
`check_working_cxx_atomics64` test that is not addressed by this patch.
If we tweak the example a little bit to introduce a post-increment for `x`:
@@ -2,6 +2,7 @@
#include
std::atomic x (0);
int main() {
+ x++;
uint64_t i = x.load(std::memory_order_relaxed);
return 0;
}
the post-increment generates these extra instructions in `main()`:
lia1,0
auipc a0,0x2
addi a0,a0,-1648 # 2060
jal ra,774 <_ZNSt13__atomic_baseImEppEi>
where `_ZNSt13__atomic_baseImEppEi` is:
0774 <_ZNSt13__atomic_baseImEppEi>:
774: fc010113addisp,sp,-64
778: 02813c23sd s0,56(sp)
77c: 04010413addis0,sp,64
780: fca43423sd a0,-56(s0)
784: 00058793mv a5,a1
788: fcf42223sw a5,-60(s0)
78c: fc843783ld a5,-56(s0)
790: fef43023sd a5,-32(s0)
794: 00100793li a5,1
798: fef43423sd a5,-24(s0)
79c: 00500793li a5,5
7a0: fcf42e23sw a5,-36(s0)
7a4: fe043783ld a5,-32(s0)
7a8: fe843703ld a4,-24(s0)
7ac: 0f5ffence iorw,ow
7b0: 04e7b6afamoadd.d.aq a3,a4,(a5)
7b4: 0013nop
7b8: 00068793mv a5,a3
7bc: 00078513mv a0,a5
7c0: 03813403ld s0,56(sp)
7c4: 04010113addisp,sp,64
7c8: 8067ret
Again ignoring the fine details of RISC-V assembly, we see that the
post-increment is provided primarily by the instruction at address `0x7b0`,
which is `amoadd.d` and stands for **`a`**tomic **`m`**emory **`o`**peration
for a **`d`**ouble word. As a result, no calls to libatomic are generated.
If we further modify our example to use `uint8_t` instead of `uint64_t`:
@@ -1,8 +1,8 @@
#incl
[PATCH] D69869: [clang-tools-extra] fix the check for if '-latomic' is necessary
gokturk added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69869/new/ https://reviews.llvm.org/D69869 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
