[PATCH] D69869: [clang-tools-extra] fix the check for if '-latomic' is necessary
This revision was automatically updated to reflect the committed changes. Closed by commit rG1d40c4150630: [clang-tools-extra] fix the check for if -latomic is necessary (authored by gokturk, committed by luismarques). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69869/new/ 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
luismarques accepted this revision. luismarques added a comment. This revision is now accepted and ready to land. Whether or not GCC behaves the way it should behave regarding atomics, this seems like a sensible patch to make things work given the current situation. LGTM. 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
[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
[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 @@
[PATCH] D69869: [clang-tools-extra] fix the check for if '-latomic' is necessary
compnerd added a comment. I agree with @efriedma that it sounds odd, could you explain that please? It took about 3 reads before I saw the OUT in the WITHOUT, and then realized that this was just: `NOT (HAVE_CXX_ATOMICS_WITHOUT_LIB AND HAVE_CXX_ATOMICS64_WITHOUT_LIB)`. I don't know if there is a better way to write this though. Given the similar change that was made elsewhere in the build, I think that the change from the build perspective makes sense and is correct. Perhaps a little note for readers would be helpful to indicate that this is the negation of the pair so that its easier to spot the fact that the variable themselves are negated. 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
[PATCH] D69869: [clang-tools-extra] fix the check for if '-latomic' is necessary
efriedma added a comment. > while failing on 8-bit atomic operations as there is no hardware support That's weird; it should be possible to emulate 8-bit atomic operations on top of 64-bit cmpxchg. 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
[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
