[PATCH] D85691: lld: link libatomic if needed for Timer

[Aaron Puchert via Phabricator via llvm-commits]

aaronpuchert added a comment.

First of all thanks for doing this, I was observing the same issue on `arvm6l` and `armv7l`.

In D85691#2208323 <https://reviews.llvm.org/D85691#2208323>, @jfb wrote:

> You're probably better off putting the `std::chrono::nanoseconds::rep` behind a lock if it's not always lock free on all platforms.

I think the vast majority of LLVM users are on platforms with 64-bit atomics, so they'll benefit from using atomics here, if only slightly. And on the remaining platforms `libatomic` will probably just do the same locking that you'd do manually. It would be bad if we had some non-trivial lock-free data structure, but here I don't think there is any benefit in locking manually.

> Some platforms just don't have `libatomic`, so this patch might cause other issues.

What this patch **doesn't do** is introducing the fallback to libatomic, that is already there, for example in clangd <https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clangd/support/CMakeLists.txt#L14-L16> or as mentioned in lldb. It just extends the fallback to another subproject.

@cuviper is right here that there is a `FATAL_ERROR` if a platform has neither native support for atomics nor `libatomic`. (See below.)

In D85691#2208375 <https://reviews.llvm.org/D85691#2208375>, @jfb wrote:

> macOS doesn't have libatomic by default 🙂

I guess macOS doesn't support any platform without native 64-bit atomics? (At least not anymore.)

Or maybe it links the required libraries automatically, or generates some code directly.

In D85691#2208445 <https://reviews.llvm.org/D85691#2208445>, @joerg wrote:

> I think for the use case of Timer, just splitting it into second and subsecond with explicit overflow handling is perfectly reasonable and does not affect the correctness of the normal use as the aggregates are only accessed on completion?

You're probably right that this should be possible (would be nice to have a combination of `__builtin_add_overflow` and `__atomic_fetch_add` for this), but maybe it's not worth the effort.

In D85691#2212007 <https://reviews.llvm.org/D85691#2212007>, @cuviper wrote:

> I also see a positive flag `HAVE_CXX_ATOMICS64_WITH_LIB`, but it's not used in as many places, and now I don't feel confident that this is propagated the way I expected.

The `WITH_LIB` variant is only set if `WITHOUT_LIB` is false:

  if(NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB)
    # ...
    if(HAVE_CXX_LIBATOMICS64)
      # ...
      if (NOT HAVE_CXX_ATOMICS64_WITH_LIB)
        message(FATAL_ERROR "Host compiler must support 64-bit std::atomic!")
      endif()
    else()
      message(FATAL_ERROR "Host compiler appears to require libatomic for 64-bit operations, but cannot find it.")
    endif()
  endif()

But I think you can use both, because variables that are not set are implicitly false. (Though it's a bit nasty to rely on that in my opinion.)

In D85691#2212015 <https://reviews.llvm.org/D85691#2212015>, @cuviper wrote:

> Oh, when LLDB uses this flag, they have `LLDBStandalone.cmake` with `include(CheckAtomic)`.

And you were observing this in an `lld` standalone build? Because otherwise we have `llvm/CMakeLists.txt` -> `llvm/cmake/config-ix.cmake` -> `llvm/cmake/modules/CheckAtomic.cmake`. So yeah, you should probably make sure this is included somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85691