[libc++][NFC] Clean up std::__call_once

[ldionne]

Summary

__call_once is large and cluttered with #ifdef preprocessor guards. This
cleans it up a bit by using an exception guard instead of try-catch.

[• Quuxplusone]

IMHO the "before" picture is pretty gross, but the "after" picture isn't very much improved. (Of course the diff makes the change look scarier than it probably really is, but still...)
Perhaps consider naming the helpers something like call_once_update_from_zero_to_one(flag), call_once_store_zero(flag), call_once_store_minusone(flag), and maybe adding some comments about the flow: or maybe those comments are already present in <mutex>. The flow here, AIUI, is:

• flag starts as 0
• While any one thread is climbing the glass hill, flag is 1
• When that thread fails, it resets flag back to 0
• Once any thread has reached the top of the hill, flag becomes -1 and stays that way forever

Interestingly,

std::once_flag f;
int main() {
    std::call_once(f, [&]() { std::call_once(f, []() { puts("hello"); }); });
}

is a deadlock when !defined(_LIBCPP_HAS_NO_THREADS) but finishes with no output when defined(_LIBCPP_HAS_NO_THREADS). (This is probably intentional and a good idea, to keep the no-threads code even simpler than it would be by default, in a case that is presumably undefined behavior according to the Standard. OTOH, libstdc++ somehow manages in this case to throw system_error instead of deadlocking.)

Anyway, FWIW, I see no particular problem with this PR.

[DanielMcIntosh-IBM]

Change _State_type to an enum. This hopefully indirectly addresses @Quuxplusone's comment

[• Quuxplusone]

libcxx/include/mutex

581–583

This approach looks reasonable to me. However,

• Looks like Clang does not support enum E : U { as an extension to C++03, which explains the failing C++03 tests now. Maybe you can keep __state_ as an integer type and just use a C++03 enum to name these values? Or just use static const _State_type static data members.
• These identifiers will need to be _Uglified because they're in a public header. Moving them into mutex.cpp would be one option; or you could make them _Unset, _Pending, _Complete. (Compare to the private enum types in <filesystem>.)
• And you'll still have to wait for @ldionne to look at this PR in general.

[DanielMcIntosh-IBM]

Address @Quuxplusone's comment

[DanielMcIntosh-IBM]

libcxx/include/mutex

581–583

I would have put them inside mutex.cpp, except then they couldn't be used here on line 587, 673, 689, and 701

[DanielMcIntosh-IBM]

Change indentation to match the rest of the file

[DanielMcIntosh-IBM]

Ping

[• Quuxplusone]

Still looks like "big ugly diff but probably harmless" to me. Needs more attention from not-me.
Also, your three new static const should probably be static _LIBCPP_CONSTEXPR const just for consistency with everywhere else we do that.

[ldionne]

I really like the introduction of named values like once_flag::_Unset. IMO, we should introduce them in a separate patch which would be trivial to approve.

Then, I would suggest we use a scope guard variable to refactor the messy _LIBCPP_NO_EXCEPTIONS. For example:

void __call_once(volatile once_flag::_State_type& flag, void* arg, void (*func)(void*))
{
#if defined(_LIBCPP_HAS_NO_THREADS)
    if (flag == once_flag::_Unset)
    {
        __scope_guard guard = [&flag] { flag = once_flag::_Unset; };
        flag = once_flag::_Pending;
        func(arg);
        flag = once_flag::_Complete;
    }

#else // !_LIBCPP_HAS_NO_THREADS

    __libcpp_mutex_lock(&mut);
    while (flag == once_flag::_Pending)
        __libcpp_condvar_wait(&cv, &mut);

    if (flag == once_flag::_Unset)
    {
        __scope_guard guard = [&mut, &flag, &cv] {
            __libcpp_mutex_lock(&mut);
            __libcpp_relaxed_store(&flag, once_flag::_Unset);
            __libcpp_mutex_unlock(&mut);
            __libcpp_condvar_broadcast(&cv);
        };

        __libcpp_relaxed_store(&flag, once_flag::_Pending);
        __libcpp_mutex_unlock(&mut);
        func(arg);
        __libcpp_mutex_lock(&mut);
        __libcpp_atomic_store(&flag, once_flag::_Complete, _AO_Release);
        __libcpp_mutex_unlock(&mut);
        __libcpp_condvar_broadcast(&cv);
    }
    else
    {
        __libcpp_mutex_unlock(&mut);
    }
#endif // !_LIBCPP_HAS_NO_THREADS
}

This is much cleaner IMO since we don't have any #ifdefs related to exceptions anymore, and we don't need to change the structure of the code, which obscures what's happening. Also, we've been meaning to introduce such a scope guard with @var-const to refactor some algorithms that look messy right now, so this would kill two birds with one stone. LMK what you think about this suggestion and I can open a patch for __scope_guard if you agree.

[• Quuxplusone]

FWIW, when I see the name __scope_guard I assume it'll work like a finally clause, running its code unconditionally on scope exit. (Like https://quuxplusone.github.io/blog/2018/08/11/the-auto-macro/ . :))
But what we want here is a catch clause, running its code only if an exception actually is thrown. No matter what you name the scope-guard, it's going to be a speed bump for the reader, whereas try { func(arg); } catch(...) { some code; throw; } can be understood instantly.
Perhaps Louis's idea, but with the scope-guard class type named struct __catch_and_rethrow_logic? It needs something to catch the reader's attention that it's specifically the catch path, not a general-purpose scope guard.

[ldionne]

In D112319#3190723, @Quuxplusone wrote:

FWIW, when I see the name __scope_guard I assume it'll work like a finally clause, running its code unconditionally on scope exit. (Like https://quuxplusone.github.io/blog/2018/08/11/the-auto-macro/ . :))
But what we want here is a catch clause, running its code only if an exception actually is thrown. No matter what you name the scope-guard, it's going to be a speed bump for the reader, whereas try { func(arg); } catch(...) { some code; throw; } can be understood instantly.
Perhaps Louis's idea, but with the scope-guard class type named struct __catch_and_rethrow_logic? It needs something to catch the reader's attention that it's specifically the catch path, not a general-purpose scope guard.

Fair point. Let's bikeshed about the name and the specific design of that type in D115730.

With that review, we'd be able to write:

void __call_once(volatile once_flag::_State_type& flag, void* arg, void (*func)(void*))
{
#if defined(_LIBCPP_HAS_NO_THREADS)
    if (flag == once_flag::_Unset)
    {
        __transaction transaction([&flag] { flag = once_flag::_Unset; });
        flag = once_flag::_Pending;
        func(arg);
        flag = once_flag::_Complete;
        transaction.__complete();
    }

#else // !_LIBCPP_HAS_NO_THREADS

    __libcpp_mutex_lock(&mut);
    while (flag == once_flag::_Pending)
        __libcpp_condvar_wait(&cv, &mut);

    if (flag == once_flag::_Unset)
    {
        __transaction transaction([&mut, &flag, &cv] {
            __libcpp_mutex_lock(&mut);
            __libcpp_relaxed_store(&flag, once_flag::_Unset);
            __libcpp_mutex_unlock(&mut);
            __libcpp_condvar_broadcast(&cv);
        });

        __libcpp_relaxed_store(&flag, once_flag::_Pending);
        __libcpp_mutex_unlock(&mut);
        func(arg);
        __libcpp_mutex_lock(&mut);
        __libcpp_atomic_store(&flag, once_flag::_Complete, _AO_Release);
        __libcpp_mutex_unlock(&mut);
        __libcpp_condvar_broadcast(&cv);
        transaction.__complete();
    }
    else
    {
        __libcpp_mutex_unlock(&mut);
    }
#endif // !_LIBCPP_HAS_NO_THREADS
}

[ldionne]

I am doing the pre-requisite refactoring here: https://github.com/llvm/llvm-project/pull/66289

[ldionne]

[Github PR transition cleanup]

Commandeering to finish.

[ldionne]

Rebase on top of https://github.com/llvm/llvm-project/pull/66289 and update with the approach suggested in https://reviews.llvm.org/D112319#3190157.

[ldionne]

Rebase onto main.

libcxx/include/mutex

 template<class _Callable>
 _LIBCPP_INLINE_VISIBILITY
 void call_once(once_flag&, const _Callable&);
 
 #endif // _LIBCPP_CXX03_LANG
 
 struct _LIBCPP_TEMPLATE_VIS once_flag
 {
-    _LIBCPP_INLINE_VISIBILITY
-    _LIBCPP_CONSTEXPR
-        once_flag() _NOEXCEPT : __state_(0) {}
-
 #if defined(_LIBCPP_ABI_MICROSOFT)
-   typedef uintptr_t _State_type;
+    enum _State_type : uintptr_t {
 #else
-   typedef unsigned long _State_type;
+    enum _State_type : unsigned long {
 #endif
+        UNSET,
+        PENDING,
+        COMPLETE = ~_State_type(0)
+    };
 
+    _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_CONSTEXPR
+        once_flag() _NOEXCEPT : __state_(UNSET) {}
 
 private:
     once_flag(const once_flag&); // = delete;
     once_flag& operator=(const once_flag&); // = delete;
 
     _State_type __state_;
 
 #ifndef _LIBCPP_CXX03_LANG

 
 #ifndef _LIBCPP_CXX03_LANG
 
 template<class _Callable, class... _Args>
 inline _LIBCPP_INLINE_VISIBILITY
 void
 call_once(once_flag& __flag, _Callable&& __func, _Args&&... __args)
 {
-    if (__libcpp_acquire_load(&__flag.__state_) != ~once_flag::_State_type(0))
+    if (__libcpp_acquire_load(&__flag.__state_) != once_flag::COMPLETE)
     {
         typedef tuple<_Callable&&, _Args&&...> _Gp;
         _Gp __f(_VSTD::forward<_Callable>(__func), _VSTD::forward<_Args>(__args)...);
         __call_once_param<_Gp> __p(__f);
         __call_once(__flag.__state_, &__p, &__call_once_proxy<_Gp>);
     }
 }
 
 #else  // _LIBCPP_CXX03_LANG
 
 template<class _Callable>
 inline _LIBCPP_INLINE_VISIBILITY
 void
 call_once(once_flag& __flag, _Callable& __func)
 {
-    if (__libcpp_acquire_load(&__flag.__state_) != ~once_flag::_State_type(0))
+    if (__libcpp_acquire_load(&__flag.__state_) != once_flag::COMPLETE)
     {
         __call_once_param<_Callable> __p(__func);
         __call_once(__flag.__state_, &__p, &__call_once_proxy<_Callable>);
     }
 }
 
 template<class _Callable>
 inline _LIBCPP_INLINE_VISIBILITY
 void
 call_once(once_flag& __flag, const _Callable& __func)
 {
-    if (__libcpp_acquire_load(&__flag.__state_) != ~once_flag::_State_type(0))
+    if (__libcpp_acquire_load(&__flag.__state_) != once_flag::COMPLETE)
     {
         __call_once_param<const _Callable> __p(__func);
         __call_once(__flag.__state_, &__p, &__call_once_proxy<const _Callable>);
     }
 }
 
 #endif // _LIBCPP_CXX03_LANG
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP_MUTEX

libcxx/src/mutex.cpp

 // call into dispatch_once_f instead of here. Relevant radar this code needs to
 // keep in sync with:  7741191.
 
 #ifndef _LIBCPP_HAS_NO_THREADS
 _LIBCPP_SAFE_STATIC static __libcpp_mutex_t mut = _LIBCPP_MUTEX_INITIALIZER;
 _LIBCPP_SAFE_STATIC static __libcpp_condvar_t cv = _LIBCPP_CONDVAR_INITIALIZER;
 #endif
 
-void __call_once(volatile once_flag::_State_type& flag, void* arg,
-                 void (*func)(void*))
-{
-#if defined(_LIBCPP_HAS_NO_THREADS)
-    if (flag == 0)
-    {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        try
-        {
-#endif // _LIBCPP_NO_EXCEPTIONS
-            flag = 1;
-            func(arg);
-            flag = ~once_flag::_State_type(0);
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        }
-        catch (...)
-        {
-            flag = 0;
-            throw;
-        }
-#endif // _LIBCPP_NO_EXCEPTIONS
-    }
+/// Returns true if it's already been called, and false otherwise
+static inline bool call_once_prep(volatile once_flag::_State_type& flag) {
+#ifdef _LIBCPP_HAS_NO_THREADS
+  if (flag != once_flag::UNSET)
+    return true;
+  flag = once_flag::PENDING;
+  return false;
 #else // !_LIBCPP_HAS_NO_THREADS
-    __libcpp_mutex_lock(&mut);
-    while (flag == 1)
-        __libcpp_condvar_wait(&cv, &mut);
-    if (flag == 0)
-    {
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        try
-        {
-#endif // _LIBCPP_NO_EXCEPTIONS
-            __libcpp_relaxed_store(&flag, once_flag::_State_type(1));
-            __libcpp_mutex_unlock(&mut);
-            func(arg);
-            __libcpp_mutex_lock(&mut);
-            __libcpp_atomic_store(&flag, ~once_flag::_State_type(0),
-                                  _AO_Release);
-            __libcpp_mutex_unlock(&mut);
-            __libcpp_condvar_broadcast(&cv);
-#ifndef _LIBCPP_NO_EXCEPTIONS
-        }
-        catch (...)
-        {
-            __libcpp_mutex_lock(&mut);
-            __libcpp_relaxed_store(&flag, once_flag::_State_type(0));
-            __libcpp_mutex_unlock(&mut);
-            __libcpp_condvar_broadcast(&cv);
-            throw;
-        }
-#endif // _LIBCPP_NO_EXCEPTIONS
-    }
-    else
-        __libcpp_mutex_unlock(&mut);
-#endif // !_LIBCPP_HAS_NO_THREADS
+  while (flag == once_flag::PENDING)
+
+  if (flag != once_flag::UNSET) {
+    return true;
+  }
+
+  __libcpp_relaxed_store(&flag, once_flag::PENDING);
+  __libcpp_mutex_unlock(&mut);
+  return false;
+#endif // !_LIBCPP_HAS_NO_THREADS
+}
+
+static inline void call_once_finish(volatile once_flag::_State_type& flag) {
+#ifdef _LIBCPP_HAS_NO_THREADS
+  flag = once_flag::COMPLETE;
+#else // !_LIBCPP_HAS_NO_THREADS
+  __libcpp_atomic_store(&flag, once_flag::COMPLETE, _AO_Release);
+#endif // !_LIBCPP_HAS_NO_THREADS
+
+static inline void call_once_abort(volatile once_flag::_State_type& flag) {
+#ifdef _LIBCPP_HAS_NO_THREADS
+  flag = once_flag::UNSET;
+#else // !_LIBCPP_HAS_NO_THREADS
+  __libcpp_relaxed_store(&flag, once_flag::UNSET);
+#endif // !_LIBCPP_HAS_NO_THREADS
+
+void __call_once(volatile once_flag::_State_type& flag, void* arg, void (*func)(void*)) {
+  if (call_once_prep(flag))
+    return;
+#ifdef _LIBCPP_NO_EXCEPTIONS
+  func(arg);
+#else // !_LIBCPP_NO_EXCEPTIONS
+  try {
+    func(arg);
+  } catch (...) {
+    call_once_abort(flag);
+    throw;
+#endif // !_LIBCPP_NO_EXCEPTIONS
+  call_once_finish(flag);
 }
 
 _LIBCPP_END_NAMESPACE_STD