[libcxx-commits] [PATCH] D114109: [libc++] Enable <atomic> when threads are disabled

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: libc++.
ldionne added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: libcxx/include/__config:1196
-     !defined(_LIBCPP_HAS_EXTERNAL_ATOMIC_IMP)) \
-     || defined(_LIBCPP_HAS_NO_THREADS)
 #  define _LIBCPP_HAS_NO_ATOMIC_HEADER
----------------
nilayvaish wrote:
> Do we know why this constraint on having atomics only when threads are enabled was put in place?
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.


================
Comment at: libcxx/include/__thread/poll_with_backoff.h:39-44
+      if (__f())
+        return true; // _Fn completion means success
+      if (__count < __libcpp_polling_count) {
+        __count += 1;
+        continue;
+      }
----------------
nilayvaish wrote:
> 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.
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.


================
Comment at: libcxx/include/__thread/poll_with_backoff.h:59
+// are no other threads to compete with.
+struct __spinning_backoff_policy {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
----------------
Mordante wrote:
> Should this be marked with `_LIBCPP_TYPE_VIS`?
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.


================
Comment at: libcxx/include/atomic:1524
+    using _Policy = __libcpp_timed_backoff_policy;
+#endif
+    return __libcpp_thread_poll_with_backoff(__test_fn, _Policy());
----------------
Mordante wrote:
> 
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.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:674
 lit_markup = {
-  "atomic": ["UNSUPPORTED: libcpp-has-no-threads"],
   "barrier": ["UNSUPPORTED: libcpp-has-no-threads"],
----------------
curdeius wrote:
> I think you need to do the same modification in other generate_* scripts. E.g. https://github.com/llvm/llvm-project/blob/main/libcxx/utils/generate_header_inclusion_tests.py#L93, https://github.com/llvm/llvm-project/blob/main/libcxx/utils/generate_header_tests.py#L24.
You're right, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114109