lld: link libatomic if needed for Timer

[cuviper]

Summary

D80298 made Timer::total atomic, but this requires linking libatomic
on some targets.

[MaskRay]

lld/Common/CMakeLists.txt

5

2-space indentation

[cuviper]

Fixed indentation

[MaskRay]

Thanks!

lld/Common/CMakeLists.txt

3

I don't whether having a space after if is prevailing or not, but the space before ) is uncommon and should be removed.

[jfb]

You're probably better off putting the std::chrono::nanoseconds::rep behind a lock if it's not always lock free on all platforms. Some platforms just don't have libatomic, so this patch might cause other issues.

[cuviper]

In 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. Some platforms just don't have libatomic, so this patch might cause other issues.

Wouldn't such a platform get a fatal error in llvm/cmake/modules/CheckAtomic.cmake?

lld/Common/CMakeLists.txt

3

That was copy-pasted from lldb, but I'll remove these spaces for consistency with the other if in this file.

[cuviper]

More whitespace cleanup

[jfb]

In D85691#2208348, @cuviper wrote:

In 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. Some platforms just don't have libatomic, so this patch might cause other issues.

Wouldn't such a platform get a fatal error in llvm/cmake/modules/CheckAtomic.cmake?

Not sure without digging through cmake-isms... but macOS doesn't have libatomic by default 🙂

More importantly, it seems like std::chrono::nanoseconds::rep might be the wrong type to make atomic, or if it is then it should be behind a lock. Using locks implicitly through libatomic jsut hides the lock in the runtime. Or maybe atomics and locks are the wrong solution for D80298, and it should accumulate timers some other way 🤷‍♂️

In any case, I don't think using non-lock-free atomics is what you want here. Linking libatomic seems to be patching the wrong issue.

[cuviper]

@jfb You did see that this is only for NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB, right? So I'm only adding that link (and hidden locking) for targets that can't do 64-bit atomics natively, otherwise you get link errors like this Rust CI failure on arm. This kind of conditional atomic link exists in several places in the monorepo already.

It makes sense to me for Timer to use a 64-bit atomic, whether native or emulated, but I'll defer to @rnk as the author on that.

[joerg]

This issue is on my long term TODO list. If there are no 64bit atomics, it is a really stupid idea to use them for timekeeping. 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?

[cuviper]

Well, I now have evidence that this may indeed try to link a nonexistent library -- a new Rust CI run failed on MSVC:

LINK : fatal error LNK1181: cannot open input file 'atomic.lib'

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.

[cuviper]

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

[aaronpuchert]

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

In 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 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, @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, @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, @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, @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.

[cuviper]

include(CheckAtomic) for standalone builds

[cuviper]

In 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.

Yeah, that was a standalone build. I did confirm the fix when I included that in Rust CI, but forgot to push it here, sorry.

[aaronpuchert]

Thanks, this looks good to me.

Maybe give the other commenters a few days to intervene before you commit this.

[aaronpuchert]

@hans, could we bring this to the release-11.x branch?

[cuviper]

FWIW, I also tagged bug 47123 for release-11.0.0.

[aaronpuchert]

In D85691#2232079, @cuviper wrote:

FWIW, I also tagged bug 47123 for release-11.0.0.

Good to know, I wasn't aware of the bug report. Next time if there is a bug, just add something like "Fixes PR47123." to the commit message.

[hans]

Cherry-picked to 11.x as 1708358fbbf58d4a870be015ef21eeea9adae87b. Thanks!

lld/Common/CMakeLists.txt

+set(LLD_SYSTEM_LIBS ${LLVM_PTHREAD_LIB})
+
+if (NOT HAVE_CXX_ATOMICS64_WITHOUT_LIB )
+  list(APPEND LLD_SYSTEM_LIBS atomic)
+endif()
+
 find_first_existing_vc_file("${LLVM_MAIN_SRC_DIR}" llvm_vc)
 find_first_existing_vc_file("${LLD_SOURCE_DIR}" lld_vc)
 
 set(version_inc "${CMAKE_CURRENT_BINARY_DIR}/VCSVersion.inc")
 set(generate_vcs_version_script "${LLVM_CMAKE_PATH}/GenerateVersionFromVCS.cmake")
 
 if(lld_vc AND LLVM_APPEND_VC_REV)
   set(lld_source_dir ${LLD_SOURCE_DIR})
add_lld_library(lldCommon
   DebugInfoDWARF
   Demangle
   MC
   Option
   Support
   Target
 
   LINK_LIBS
-  ${LLVM_PTHREAD_LIB}
+  ${LLD_SYSTEM_LIBS}
 
   DEPENDS
   intrinsics_gen
   )