[libc++] Refactoring __sync_* builtins; NFC

[weimingz]

Summary

Wrap sync_* builtins with libcpp_ functions to facility future customizations as atomic operations are unavailable on some targets.

[joerg]

Lock-free atomic operations are also signal safe. Your code is not. While I don't know whether all this functions are not required to be signal safe, the general assertion is certainly questionable.

[weimingz]

In D34918#797661, @joerg wrote:

Lock-free atomic operations are also signal safe. Your code is not. While I don't know whether all this functions are not required to be signal safe, the general assertion is certainly questionable.

No, they are no signal safe. Per [1], "call to any library function, except the following signal-safe functions (note, in particular, dynamic allocation is not signal-safe):".

Locale.cpp should be fine with the change.
{set,get}_new_handler, {set,get}_unexpected_handler are not in the list.
I'm not very sure about "__libcpp_refstring". Seems it is only used by stdexcept. Although throw expression is explicitly stated as not signal safe, the ref counting of stdexcept might be. We can leave it unchanged.

[1] http://en.cppreference.com/w/cpp/utility/program/signal

[EricWF]

On architectures without atomic instructions, the atomic built-ins cannot be lowered. If _LIBCPP_HAS_NO_THREADS is enabled, we should just use regular code.

Does "cannot be lowered" mean using them causes a compile error? If so I'm curious as to why you're the first one to run into this issue. If no compile error is caused then could you re-explain the rational for this change.

In D34918#798422, @weimingz wrote:

In D34918#797661, @joerg wrote:

Lock-free atomic operations are also signal safe. Your code is not. While I don't know whether all this functions are not required to be signal safe, the general assertion is certainly questionable.

No, they are no signal safe. Per [1], "call to any library function, except the following signal-safe functions (note, in particular, dynamic allocation is not signal-safe):".

Locale.cpp should be fine with the change.
{set,get}_new_handler, {set,get}_unexpected_handler are not in the list.
I'm not very sure about "__libcpp_refstring". Seems it is only used by stdexcept. Although throw expression is explicitly stated as not signal safe, the ref counting of stdexcept might be. We can leave it unchanged.

It would be really nice if we didn't cause the user unnecessary pain. Just because these functions aren't *required* to be signal safe doesn't necessarily mean they shouldn't be.

If this patch is indeed needed I would like to see it done similar to how [__libcpp_refcount_foo](https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L3370) or the [src/include/atomic_support.h](https://github.com/llvm-mirror/libcxx/blob/master/src/include/atomic_support.h) logic is implemented, where instead of having #ifdef branches at each call site the logic has been abstracted away into a function.

That being said I would like to better understand the rational for this patch before proceeding.

[weimingz]

In D34918#799141, @EricWF wrote:

On architectures without atomic instructions, the atomic built-ins cannot be lowered. If _LIBCPP_HAS_NO_THREADS is enabled, we should just use regular code.

Does "cannot be lowered" mean using them causes a compile error? If so I'm curious as to why you're the first one to run into this issue. If no compile error is caused then could you re-explain the rational for this change.

In D34918#798422, @weimingz wrote:

In D34918#797661, @joerg wrote:

Lock-free atomic operations are also signal safe. Your code is not. While I don't know whether all this functions are not required to be signal safe, the general assertion is certainly questionable.

No, they are no signal safe. Per [1], "call to any library function, except the following signal-safe functions (note, in particular, dynamic allocation is not signal-safe):".

Locale.cpp should be fine with the change.
{set,get}_new_handler, {set,get}_unexpected_handler are not in the list.
I'm not very sure about "__libcpp_refstring". Seems it is only used by stdexcept. Although throw expression is explicitly stated as not signal safe, the ref counting of stdexcept might be. We can leave it unchanged.

It would be really nice if we didn't cause the user unnecessary pain. Just because these functions aren't *required* to be signal safe doesn't necessarily mean they shouldn't be.

If this patch is indeed needed I would like to see it done similar to how [__libcpp_refcount_foo](https://github.com/llvm-mirror/libcxx/blob/master/include/memory#L3370) or the [src/include/atomic_support.h](https://github.com/llvm-mirror/libcxx/blob/master/src/include/atomic_support.h) logic is implemented, where instead of having #ifdef branches at each call site the logic has been abstracted away into a function.

That being said I would like to better understand the rational for this patch before proceeding.

When the builtin cannot be lowered, compiler just emits function call, which eventually becomes a linker error as no libs implement those functions (libgcc. libclang-rt, libc)

[weimingz]

If the architecture doesn't support atomics (like cortex-m0), seems there is no software solution to make them signal-safe. Mutex/lock won't work.

[weimingz]

How about we use "#ifdef GCC_ATOMIC_POINTER_LOCK_FREE < 2 && LIBCPP_HAS_NO_THREAD" ?
This can minimize the impact.

[EricWF]

Why don't you start by adding __libcpp_sync_foo functions that we can use, then we can decide on what macros to use to guard their definition.

I'm not exactly sure where the __libcpp_sync_foo functions should live since they have to be used in __refstring, but I would just put wherever works at first.

[weimingz]

@EricWF , do you mean factoring out the __sync_* functions like this?

[jroelofs]

In D34918#801611, @weimingz wrote:

@EricWF , do you mean factoring out the __sync_* functions like this?

@weimingz this refactoring LGTM. we can work out what the second set of definitions in __atomic_support looks like in another patch.

[weimingz]

missed to upload the change to include __atomic_support header file

[weimingz]

I should use existing include/atomic_support.h. Will upload the patch shortly.

[EricWF]

I'm not OK with adding a new header here. The problem is that __refstring requires using the new <__atomic_support> but <__refstring> shouldn't be a header anyway.

Could you please revert this till we figure out a better place for the builtins to live?

[EricWF]

The return types for the new __libcpp_sync builtins are incorrect as well.

[EricWF]

And they lack reserved names. Sorry for the repeated comment spam.

I'm going to go ahead and revert this myself. Please let me sign off next time.

libcxx/include/__atomic_support

+// -*- C++ -*-
+//===------------------------ __atomic_support -----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef _LIBCPP___ATOMIC_SUPPORT
+#define _LIBCPP___ATOMIC_SUPPORT
+
+#include <__config>
+
+template<typename T>
+T __libcpp_sync_add_and_fetch(T *ptr, T value) {
+  return __sync_add_and_fetch(ptr, value);
+}
+
+template<typename T>
+T __libcpp_sync_fetch_and_add(T *ptr, T value) {
+  return __sync_fetch_and_add(ptr, value);
+}
+
+template<typename T>
+T __libcpp_sync_lock_test_and_set(T *ptr, T value) {
+  return __sync_lock_test_and_set(ptr, value);
+}
+
+#endif //_LIBCPP___ATOMIC_SUPPORT

libcxx/include/__refstring

 //===------------------------ __refstring ---------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___REFSTRING
 #define _LIBCPP___REFSTRING
 
 #include <__config>
 #include <stdexcept>
 #include <cstddef>
 #include <cstring>
+#include <__atomic_support>
 #ifdef __APPLE__
 #include <dlfcn.h>
 #include <mach-o/dyld.h>
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 namespace __refstring_imp { namespace {
__libcpp_refstring::__libcpp_refstring(const char* msg) {
     __imp_ = data;
 }
 
 inline
 __libcpp_refstring::__libcpp_refstring(const __libcpp_refstring &s) _NOEXCEPT
     : __imp_(s.__imp_)
 {
     if (__uses_refcount())
-        __sync_add_and_fetch(&rep_from_data(__imp_)->count, 1);
+        __libcpp_sync_add_and_fetch(&rep_from_data(__imp_)->count, 1);
 }
 
 inline
 __libcpp_refstring& __libcpp_refstring::operator=(__libcpp_refstring const& s) _NOEXCEPT {
     bool adjust_old_count = __uses_refcount();
     struct _Rep_base *old_rep = rep_from_data(__imp_);
     __imp_ = s.__imp_;
     if (__uses_refcount())
-        __sync_add_and_fetch(&rep_from_data(__imp_)->count, 1);
+        __libcpp_sync_add_and_fetch(&rep_from_data(__imp_)->count, 1);
     if (adjust_old_count)
     {
-        if (__sync_add_and_fetch(&old_rep->count, count_t(-1)) < 0)
+        if (__libcpp_sync_add_and_fetch(&old_rep->count, count_t(-1)) < 0)
         {
             ::operator delete(old_rep);
         }
     }
     return *this;
 }
 
 inline
 __libcpp_refstring::~__libcpp_refstring() {
     if (__uses_refcount()) {
         _Rep_base* rep = rep_from_data(__imp_);
-        if (__sync_add_and_fetch(&rep->count, count_t(-1)) < 0) {
+        if (__libcpp_sync_add_and_fetch(&rep->count, count_t(-1)) < 0) {
             ::operator delete(rep);
         }
     }
 }
 
 inline
 bool __libcpp_refstring::__uses_refcount() const {
 #ifdef __APPLE__

libcxx/src/locale.cpp

 #  include "type_traits"
 #endif
 #include "clocale"
 #include "cstring"
 #if defined(_LIBCPP_MSVCRT)
 #define _CTYPE_DISABLE_MACROS
 #endif
 #include "cwctype"
+#include "__atomic_support"
 #include "__sso_allocator"
 #if defined(_LIBCPP_MSVCRT) || defined(__MINGW32__)
 #include "support/win32/locale_win32.h"
 #elif !defined(__BIONIC__)
 #include <langinfo.h>
 #endif
 #include <stdlib.h>
 #include <stdio.h>

 {
     call_once(__flag_, __fake_bind(&locale::id::__init, this));
     return __id_ - 1;
 }
 
 void
 locale::id::__init()
 {
-    __id_ = __sync_add_and_fetch(&__next_id, 1);
+    __id_ = __libcpp_sync_add_and_fetch(&__next_id, 1);
 }
 
 // template <> class collate_byname<char>
 
 collate_byname<char>::collate_byname(const char* n, size_t refs)
     : collate<char>(refs),
       __l(newlocale(LC_ALL_MASK, n, 0))
 {

libcxx/src/support/runtime/exception_fallback.ipp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include <cstdio>
+#include <__atomic_support>
 
 namespace std {
 
 _LIBCPP_SAFE_STATIC static std::terminate_handler  __terminate_handler;
 _LIBCPP_SAFE_STATIC static std::unexpected_handler __unexpected_handler;
 
 
 // libcxxrt provides implementations of these functions itself.
 unexpected_handler
 set_unexpected(unexpected_handler func) _NOEXCEPT
 {
-  return __sync_lock_test_and_set(&__unexpected_handler, func);
+  return __libcpp_sync_lock_test_and_set(&__unexpected_handler, func);
 }
 
 unexpected_handler
 get_unexpected() _NOEXCEPT
 {
-  return __sync_fetch_and_add(&__unexpected_handler, (unexpected_handler)0);
+  return __libcpp_sync_fetch_and_add(&__unexpected_handler, (unexpected_handler)0);
 
 }
 
 _LIBCPP_NORETURN
 void unexpected()
 {
     (*get_unexpected())();
     // unexpected handler should not return
     terminate();
 }
 
 terminate_handler
 set_terminate(terminate_handler func) _NOEXCEPT
 {
-  return __sync_lock_test_and_set(&__terminate_handler, func);
+  return __libcpp_sync_lock_test_and_set(&__terminate_handler, func);
 }
 
 terminate_handler
 get_terminate() _NOEXCEPT
 {
-  return __sync_fetch_and_add(&__terminate_handler, (terminate_handler)0);
-
+  return __libcpp_sync_fetch_and_add(&__terminate_handler, (terminate_handler)0);
 }
 
 #ifndef __EMSCRIPTEN__ // We provide this in JS
 _LIBCPP_NORETURN
 void
 terminate() _NOEXCEPT
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS

libcxx/src/support/runtime/new_handler_fallback.ipp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is dual licensed under the MIT and the University of Illinois Open
 // Source Licenses. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include <__atomic_support>
+
 namespace std {
 
 _LIBCPP_SAFE_STATIC static std::new_handler __new_handler;
 
 new_handler
 set_new_handler(new_handler handler) _NOEXCEPT
 {
-    return __sync_lock_test_and_set(&__new_handler, handler);
+    return __libcpp_sync_lock_test_and_set(&__new_handler, handler);
 }
 
 new_handler
 get_new_handler() _NOEXCEPT
 {
-    return __sync_fetch_and_add(&__new_handler, nullptr);
+    return __libcpp_sync_fetch_and_add<new_handler>(&__new_handler, nullptr);
 }
 
 } // namespace std