[PATCH] D130569: [Driver] Use libatomic for 32-bit SPARC atomics support on Linux

Thu Jul 28 00:54:38 PDT 2022

ro marked 2 inline comments as done.
ro added inline comments.

================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:634

+      // LLVM support for atomics on 32-bit SPARC V8+ is incomplete, so
+      // forcibly link with libatomic as a workaround.
----------------
MaskRay wrote:
> `// TODO ...` and attach a bug link
It's split between the original bug report and a patch review: I only noticed later (when `compiler-rt` started to make use of 64-bit atomics) that those aren't implemented by `clang -m32 -mcpu=v9`.

================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:637
+      if (getToolChain().getTriple().getArch() == llvm::Triple::sparc) {
+        CmdArgs.push_back("--as-needed");
+        CmdArgs.push_back("-latomic");
----------------
MaskRay wrote:
> Such --as-needed usage is a bit fragile if clang driver in the future passes `--as-needed`  in the beginning. Better to use --push-state if you have a not-too-old ld.
Good point: I initially fell into that trap in D130571 when I added `--as-needed -latomic --no-as-needed` to `SANITIZER_COMMON_LINK_FLAGS`: it got added early to the link line, before the objects using atomics, and had no effect.  Only then did Iearn that `target_link_libraries` accepts not only library names, but also linker flags.

As for `--push-state`, it was introduced in GNU `ld` 2.25 back in 2014, to it's save to assume it's present.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130569/new/

https://reviews.llvm.org/D130569