This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Enable <atomic> when threads are disabled
ClosedPublic

Authored by ldionne on Nov 17 2021, 10:37 AM.

Details

Summary

std::atomic is, for the most part, just a thin veneer on top of compiler
builtins. Hence, it should be available even when threads are not available
on the system, and in fact there has been requests for such support.

This patch:

  • Moves __libcpp_thread_poll_with_backoff to its own header so it can be used in <atomic> when threads are disabled.
  • Adds a dummy backoff policy for atomic polling that doesn't know about threads.
  • Adjusts the <atomic> feature-test macros so they are provided even when threads are disabled.
  • Runs the <atomic> tests when threads are disabled.

rdar://77873569

Diff Detail

Event Timeline

ldionne created this revision.Nov 17 2021, 10:37 AM
ldionne requested review of this revision.Nov 17 2021, 10:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 10:37 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision as: Mordante.Nov 17 2021, 11:30 AM
Mordante added a subscriber: Mordante.

Nice change! LGTM modulo some nits. (Obviously assuming the CI is happy.)

libcxx/include/__thread/poll_with_backoff.h
60

Should this be marked with _LIBCPP_TYPE_VIS?

libcxx/include/atomic
1524
libcxx/test/std/atomics/types.pass.cpp
21
nilayvaish accepted this revision.Nov 17 2021, 12:24 PM
nilayvaish added a subscriber: nilayvaish.
nilayvaish added inline comments.
libcxx/include/__config
1196

Do we know why this constraint on having atomics only when threads are enabled was put in place?

libcxx/include/__thread/poll_with_backoff.h
40–45

I am feeling slightly strange about this code. Do you think following would be better?

while (true) {

while (count < polling_count) {
// check __f
 count++;
}
// check elapsed time
// check backoff policy.

}

Or the following two separate loop version:

while (count < polling_count) {

// check __f
count++;

}
while (!__f()) {

// check elapsed time
// check backoff policy.

}

This is just for discussion since the code here has been moved from another file.

curdeius added inline comments.
libcxx/utils/generate_feature_test_macro_components.py
674
ldionne updated this revision to Diff 388019.Nov 17 2021, 1:13 PM
ldionne marked 4 inline comments as done.

Address review comments

ldionne accepted this revision as: Restricted Project.Nov 17 2021, 8:01 PM

It seems I forgot to submit my reply to review comments. Doing this now. I'm also going to ship this since I addressed everything so far and CI is green, however please feel free to comment if you have further observations/requests and I will honor them post-commit.

libcxx/include/__config
1196

I suspect there's a couple of reasons:

  1. One could argue that without threads, there is no need for atomic operations since there's no concurrent accesses. In practice, however, it can still be useful if there are threads but just not ones with an API that can be used to make libc++'s <thread> work.
  2. Some large atomics require taking locks, which are most likely unavailable when threads are not available. I suspect a system that doesn't provide threads wouldn't provide something akin to libatomic, and so non-lockfree atomics wouldn't work.
  3. We were including <__threading_support>, which requires threading to be available. Perhaps it was just out of "laziness" that it was disabled.

This is all speculation, I don't know for sure.

libcxx/include/__thread/poll_with_backoff.h
40–45

Oh my, actually the current implementation looks like a bug to me. I don't think the continue should be there at all, since it means we're never going to back off until we've polled __libcpp_polling_count times back-to-back (or __f() has been satisfied). Are we on the same page? If so, I'll create a patch once this is landed. *Great* catch.

60

It doesn't matter because the type doesn't have a vtable, and hopefully nobody is ever going to use its RTTI. And its only member function is marked as having hidden visibility. I would rather not use the attribute since it adds an unnecessary annotation.

libcxx/include/atomic
1524

Actually I will unindent the whole thing since IMO it increases readability most. While it's technically nested inside another #if block, it's so far away that I think it's OK to *not* indent the inner one at all.

libcxx/utils/generate_feature_test_macro_components.py
674

You're right, thanks!

This revision is now accepted and ready to land.Nov 17 2021, 8:01 PM
This revision was automatically updated to reflect the committed changes.

Questions about platforms w/limited 'atomic' support.

libcxx/include/atomic
2697

After this commit landed downstream we encountered a failure building for an arm baremetal target that defines the __CLANG_ATOMIC_*_LOCK_FREE vals to '1' ("sometimes lock free").

This results in the atomic_{,un}signed_lock_free typedefs here referencing an undefined _libcpp_{,un}signed_lock_free.

I suspect that before this commit _LIBCPP_HAS_NO_ATOMIC_HEADER was false.

I think maybe we could consider an #error: in the else case above that might make it clearer?

I could use some advice as to how to handle this -- does it make sense to have the atomic header in the case where these types are only designated 'sometimes lock free'? If not I think the fix would be to change the conditional for _LIBCPP_HAS_NO_ATOMIC_HEADER to consider these? Otherwise, if it does make sense to have an atomic header, can I just suppress these typedefs? That does seem to address the build failure so far.

efriedma added inline comments.
libcxx/include/atomic
2697

The standard suggests we should just suppress the typedefs, not the entire header.

From the current C++ standard, [compliance]p3:

The supplied version of the header <atomic> shall meet the same requirements as for a hosted implementation except that support for always lock-free integral atomic types ([atomics.lockfree]) is implementation-defined, and whether or not the type aliases atomic_­signed_­lock_­free and atomic_­unsigned_­lock_­free are defined ([atomics.alias]) is implementation-defined. The other headers listed in this table shall meet the same requirements as for a hosted implementation.

androm3da added inline comments.Jan 26 2022, 4:27 PM
libcxx/include/atomic
2697

The standard suggests we should just suppress the typedefs, not the entire header.

From the current C++ standard, [compliance]p3:

The supplied version of the header <atomic> shall meet the same requirements as for a hosted implementation except that support for always lock-free integral atomic types ([atomics.lockfree]) is implementation-defined, and whether or not the type aliases atomic_­signed_­lock_­free and atomic_­unsigned_­lock_­free are defined ([atomics.alias]) is implementation-defined. The other headers listed in this table shall meet the same requirements as for a hosted implementation.

Ok, thanks, that clears it up.

@efriedma @ldionne should I post a change like this?

--- a/libcxx/include/atomic
+++ b/libcxx/include/atomic
@@ -2691,10 +2691,13 @@ typedef conditional<_LIBCPP_CONTENTION_LOCK_FREE, __cxx_contention_t, char>::typ
 typedef conditional<_LIBCPP_CONTENTION_LOCK_FREE, __cxx_contention_t, unsigned char>::type      __libcpp_unsigned_lock_free;
 #else
     // No signed/unsigned lock-free types
+#define _LIBCPP_NO_LOCK_FREE_TYPES
 #endif
 
+#if !defined(_LIBCPP_NO_LOCK_FREE_TYPES)
 typedef atomic<__libcpp_signed_lock_free> atomic_signed_lock_free;
 typedef atomic<__libcpp_unsigned_lock_free> atomic_unsigned_lock_free;
+#endif
 
 #define ATOMIC_FLAG_INIT {false}
 #define ATOMIC_VAR_INIT(__v) {__v}
efriedma added inline comments.Jan 26 2022, 5:25 PM
libcxx/include/atomic
2697

Looks reasonable to me.

ldionne added inline comments.Jan 31 2022, 7:21 AM
libcxx/include/atomic
2697

Sorry for missing this thread, but I think I saw this change go by and approved it, so I guess there's no additional action needed here. Please LMK if you're still looking for an answer to this.

libcxx/test/libcxx/atomics/ext-int.verify.cpp