[clang-tools-extra] fix the check for if '-latomic' is necessary

[gokturk]

Summary

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

[efriedma]

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.

[compnerd]

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.

[gokturk]

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 <atomic>
#include <cstdint>
std::atomic<uint64_t> 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:

00000000000006bc <main>:
 6bc:	fe010113          	addi	sp,sp,-32
 6c0:	00113c23          	sd	ra,24(sp)
 6c4:	00813823          	sd	s0,16(sp)
 6c8:	02010413          	addi	s0,sp,32
 6cc:	fe042023          	sw	zero,-32(s0)
 6d0:	fe042703          	lw	a4,-32(s0)
 6d4:	000107b7          	lui	a5,0x10
 6d8:	fff78593          	addi	a1,a5,-1 # ffff <__global_pointer$+0xd7ff>
 6dc:	00070513          	mv	a0,a4
 6e0:	040000ef          	jal	ra,720 <_ZStanSt12memory_orderSt23__memory_order_modifier>
 6e4:	00050793          	mv	a5,a0
 6e8:	fef42223          	sw	a5,-28(s0)
 6ec:	00002797          	auipc	a5,0x2
 6f0:	97478793          	addi	a5,a5,-1676 # 2060 <x>
 6f4:	0ff0000f          	fence
 6f8:	0007b783          	ld	a5,0(a5)
 6fc:	0ff0000f          	fence
 700:	00000013          	nop
 704:	fef43423          	sd	a5,-24(s0)
 708:	00000793          	li	a5,0
 70c:	00078513          	mv	a0,a5
 710:	01813083          	ld	ra,24(sp)
 714:	01013403          	ld	s0,16(sp)
 718:	02010113          	addi	sp,sp,32
 71c:	00008067          	ret

0000000000000720 <_ZStanSt12memory_orderSt23__memory_order_modifier>:
 720:	fe010113          	addi	sp,sp,-32
 724:	00813c23          	sd	s0,24(sp)
 728:	02010413          	addi	s0,sp,32
 72c:	00050793          	mv	a5,a0
 730:	00058713          	mv	a4,a1
 734:	fef42623          	sw	a5,-20(s0)
 738:	00070793          	mv	a5,a4
 73c:	fef42423          	sw	a5,-24(s0)
 740:	fec42703          	lw	a4,-20(s0)
 744:	fe842783          	lw	a5,-24(s0)
 748:	00f777b3          	and	a5,a4,a5
 74c:	0007879b          	sext.w	a5,a5
 750:	0007879b          	sext.w	a5,a5
 754:	00078513          	mv	a0,a5
 758:	01813403          	ld	s0,24(sp)
 75c:	02010113          	addi	sp,sp,32
 760:	00008067          	ret

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 <cstdint>
 std::atomic<uint64_t> 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():

li	a1,0
auipc	a0,0x2
addi	a0,a0,-1648 # 2060 <x>
jal	ra,774 <_ZNSt13__atomic_baseImEppEi>

where _ZNSt13__atomic_baseImEppEi is:

0000000000000774 <_ZNSt13__atomic_baseImEppEi>:
 774:	fc010113          	addi	sp,sp,-64
 778:	02813c23          	sd	s0,56(sp)
 77c:	04010413          	addi	s0,sp,64
 780:	fca43423          	sd	a0,-56(s0)
 784:	00058793          	mv	a5,a1
 788:	fcf42223          	sw	a5,-60(s0)
 78c:	fc843783          	ld	a5,-56(s0)
 790:	fef43023          	sd	a5,-32(s0)
 794:	00100793          	li	a5,1
 798:	fef43423          	sd	a5,-24(s0)
 79c:	00500793          	li	a5,5
 7a0:	fcf42e23          	sw	a5,-36(s0)
 7a4:	fe043783          	ld	a5,-32(s0)
 7a8:	fe843703          	ld	a4,-24(s0)
 7ac:	0f50000f          	fence	iorw,ow
 7b0:	04e7b6af          	amoadd.d.aq	a3,a4,(a5)
 7b4:	00000013          	nop
 7b8:	00068793          	mv	a5,a3
 7bc:	00078513          	mv	a0,a5
 7c0:	03813403          	ld	s0,56(sp)
 7c4:	04010113          	addi	sp,sp,64
 7c8:	00008067          	ret

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 atomic memory operation for a double 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 @@
 #include <atomic>
 #include <cstdint>
-std::atomic<uint64_t> x (0);
+std::atomic<uint8_t> x (0);
 int main() {
   x++;
-  uint64_t i = x.load(std::memory_order_relaxed);
+  uint8_t i = x.load(std::memory_order_relaxed);
   return 0;
 }

and try to compile with g++ -o atomic64 -march=rv64imafd atomic64.cpp, it fails with the following:

/usr/lib/gcc/riscv64-unknown-linux-gnu/9.2.0/../../../../riscv64-unknown-linux-gnu/bin/ld: /tmp/ccNet28n.o: in function `.L0 ':
atomic64.cpp:(.text._ZNSt13__atomic_baseIhEppEi[_ZNSt13__atomic_baseIhEppEi]+0x4c): undefined reference to `__atomic_fetch_add_1'
collect2: error: ld returned 1 exit status

The reason why it failed is because there are no RISC-V assembly instructions that operate on 8-bit data. So the compiler is forced to generate a call to libatomic.

Appending -latomic fixes the problem. A look at the assembly output roughly shows:

-	0f50000f          	fence	iorw,ow
-	04e7b6af          	amoadd.d.aq	a3,a4,(a5)
+	00068613          	mv	a2,a3
+	00070593          	mv	a1,a4
+	00078513          	mv	a0,a5
+	e0dff0ef          	jal	ra,670 <__atomic_fetch_add_1@plt>

which clearly identifies the call to libatomic.

So how is this relevant to clang-tools-extra? ClangdLSPServer defines std::atomic<bool> Replied = {false};. As a result, Replied.exchange(true) will produce a call to __atomic_exchange_1. The build will fail because check_working_cxx_atomics64 makes it seem as though passing -latomic is not needed where in reality it is.

I hope this clears things up a bit. I'm open to any suggestions on improving the commit message.

[gokturk]

ping

[luismarques]

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.

clang-tools-extra/clangd/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   )
 
 if(CLANG_BUILT_STANDALONE)
   # needed to get HAVE_CXX_ATOMICS64_WITHOUT_LIB defined
   include(CheckAtomic)
 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()
 
 add_clang_library(clangDaemon
   AST.cpp
   Cancellation.cpp
   ClangdLSPServer.cpp
   ClangdServer.cpp