Page MenuHomePhabricator

lld: link libatomic if needed for Timer
ClosedPublic

Authored by cuviper on Aug 10 2020, 3:10 PM.

Details

Summary

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

Diff Detail

Event Timeline

cuviper created this revision.Aug 10 2020, 3:10 PM
Herald added a project: Restricted Project. ยท View Herald Transcript
Herald added subscribers: llvm-commits, jfb, mgorny. ยท View Herald Transcript
cuviper requested review of this revision.Aug 10 2020, 3:10 PM
MaskRay added inline comments.Aug 10 2020, 3:22 PM
lld/Common/CMakeLists.txt
5

2-space indentation

cuviper updated this revision to Diff 284516.Aug 10 2020, 3:25 PM

Fixed indentation

cuviper marked an inline comment as done.Aug 10 2020, 3:25 PM
MaskRay accepted this revision.Aug 10 2020, 3:30 PM

Thanks!

lld/Common/CMakeLists.txt
4

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

This revision is now accepted and ready to land.Aug 10 2020, 3:30 PM
jfb added a comment.Aug 10 2020, 3:31 PM

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.

MaskRay resigned from this revision.Aug 10 2020, 3:37 PM
This revision now requires review to proceed.Aug 10 2020, 3:37 PM
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
4

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

cuviper updated this revision to Diff 284519.Aug 10 2020, 3:43 PM

More whitespace cleanup

cuviper marked an inline comment as done.Aug 10 2020, 3:44 PM
Harbormaster completed remote builds in B67801: Diff 284516.
jfb added a comment.Aug 10 2020, 3:59 PM
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.

@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 added a subscriber: joerg.Aug 10 2020, 4:40 PM

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?

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.

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

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.

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.

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

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 updated this revision to Diff 286158.Aug 17 2020, 3:33 PM

include(CheckAtomic) for standalone builds

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 accepted this revision.Aug 18 2020, 8:58 AM

Thanks, this looks good to me.

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

This revision is now accepted and ready to land.Aug 18 2020, 8:58 AM
This revision was automatically updated to reflect the committed changes.

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

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

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.