libc++/win: Make once_flag have the same size as a pointer

[thakis]

Summary

unsigned long is 32-bit on 32-bit systems and 64-bit on 64-bit systems
on LP64 systems -- which most Unix systems are, but Windows isn't.
Windows is LLP64, which means unsigned long is 32-bit even on 64-bit
systems.

pplwin.h contains

static_assert(alignof(void *) == alignof(::std::once_flag), ...)

which fails due to this problem.

Instead of unsigned long, use uintptr_t, which consistently is 32-bit
on 32-bit systems and 64-bit on 64-bit systems.

Only do this when targeting the Windows ABI or when ABI
stability isn't required, since the type of once_flag::state_ gets
mangled into the exported symbol call_once.

No functional change except on 64-bit Windows.

[EricWF]

Can you add a description of why this is a problem on Windows?

libcxx/include/mutex

590

Because this change is ABI breaking and can't be done unconditionally, I would create a typedef _StateType that's uintptr_t on Windows and unsigned long everywhere else.

660–661

This change is ABI breaking because uintptr_t isn't always unsigned long. On 32 bit platforms it may be unsigned int, which mangles differently.

[thakis]

update

[thakis]

Done, thanks for catching that. As discussed on chat, the patch description already had a description on why this is a problem :)

[EricWF]

libcxx/include/mutex

579

I don't see value changing this in the unstable ABI.

[EricWF]

Otherwise, after addressing inline comments, this LGTM.

[thakis]

Thanks, landing with UNSTABLE part removed.

libcxx/include/mutex

579

I figured then it'll use the same type everywhere eventually, but removed ABI_UNSTABLE again.

[ruiu]

libcxx/trunk/include/mutex

579–583 ↗

(On Diff #191597)

Did you have to use if defined? Looks like you can always use uintptr_t.

[EricWF]

libcxx/trunk/include/mutex

579–583 ↗

(On Diff #191597)

Yes. Because although uintptr_t may always have the same size and alignment as unsigned long on non-windows platforms, it may not produce the same mangling. And we have symbols in the dylib that depend on the mangling.

[BillyONeal]

We (MSVC++) could just fix pplwin.h to not have this assumption, since clang doesn't need the workaround that forces us to reinterperet_cast void*, but that fix probably wouldn't make it shipped until VS 2019 Update 1 at the earliest (I'd have to go through ship room) and Update 2 more likely.

Comments in the other reivew about "it's odd that you're trying to use libc++ but linking with msvc++'s STL DLL, msvcp140.dll", still apply here :)

[thakis]

In D59607#1438932, @BillyONeal wrote:

We (MSVC++) could just fix pplwin.h to not have this assumption, since clang doesn't need the workaround that forces us to reinterperet_cast void*, but that fix probably wouldn't make it shipped until VS 2019 Update 1 at the earliest (I'd have to go through ship room) and Update 2 more likely.

Comments in the other reivew about "it's odd that you're trying to use libc++ but linking with msvc++'s STL DLL, msvcp140.dll", still apply here :)

I think this is a good change regardless: it didn't look intentional that once_flag::__state_ was 64-bit on 64-bit systems everywhere except on Windows. It seems someone just wasn't thinking of LLP64 / LP64.

[BillyONeal]

I think this is a good change regardless:

But it looks like it's used as a single bit here, so saving 4 bytes is a win, right?

libcxx/include/mutex

template<class Callable, class ...Args>
   void call_once(once_flag& flag, Callable&& func, Args&&... args);
 
 }  // std
 
 */
 
 #include <__config>
 #include <__mutex_base>
+#include <cstdint>
 #include <functional>
 #include <memory>
 #ifndef _LIBCPP_CXX03_LANG
 #include <tuple>
 #endif
 #include <version>
 #include <__threading_support>
 

 #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) || defined(_LIBCPP_ABI_UNSTABLE)
+   typedef uintptr_t _State_type;
+#else
+   typedef unsigned long _State_type;
+#endif
+
+
 private:
     once_flag(const once_flag&); // = delete;
     once_flag& operator=(const once_flag&); // = delete;
 
-    unsigned long __state_;
+    _State_type __state_;
 
 #ifndef _LIBCPP_CXX03_LANG
     template<class _Callable, class... _Args>
     friend
     void call_once(once_flag&, _Callable&&, _Args&&...);
 #else  // _LIBCPP_CXX03_LANG
     template<class _Callable>
     friend

 template <class _Fp>
 void
 __call_once_proxy(void* __vp)
 {
     __call_once_param<_Fp>* __p = static_cast<__call_once_param<_Fp>*>(__vp);
     (*__p)();
 }
 
-_LIBCPP_FUNC_VIS void __call_once(volatile unsigned long&, void*, void(*)(void*));
+_LIBCPP_FUNC_VIS void __call_once(volatile once_flag::_State_type&, void*,
+                                  void (*)(void*));
 
 #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_) != ~0ul)
+    if (__libcpp_acquire_load(&__flag.__state_) != ~once_flag::_State_type(0))
     {
         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_) != ~0ul)
+    if (__libcpp_acquire_load(&__flag.__state_) != ~once_flag::_State_type(0))
     {
         __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_) != ~0ul)
+    if (__libcpp_acquire_load(&__flag.__state_) != ~once_flag::_State_type(0))
     {
         __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 unsigned long& flag, void* arg, void(*func)(void*))
+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 = ~0ul;
+            flag = ~once_flag::_State_type(0);
 #ifndef _LIBCPP_NO_EXCEPTIONS
         }
         catch (...)
         {
-            flag = 0ul;
+            flag = 0;
             throw;
         }
 #endif  // _LIBCPP_NO_EXCEPTIONS
     }
 #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, 1ul);
+            __libcpp_relaxed_store(&flag, once_flag::_State_type(1));
             __libcpp_mutex_unlock(&mut);
             func(arg);
             __libcpp_mutex_lock(&mut);
-            __libcpp_atomic_store(&flag, ~0ul, _AO_Release);
+            __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, 0ul);
+            __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
-
 }
 
 _LIBCPP_END_NAMESPACE_STD