[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.

include/__refstring

__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())
+#ifdef _LIBCPP_HAS_NO_THREADS
+        ++rep_from_data(__imp_)->count;
+#else
         __sync_add_and_fetch(&rep_from_data(__imp_)->count, 1);
+#endif
 }
 
 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);
     if (adjust_old_count)
     {
+#ifdef _LIBCPP_HAS_NO_THREADS
+        if ((old_rep->count -=  count_t(-1)) < 0)
+#else
         if (__sync_add_and_fetch(&old_rep->count, count_t(-1)) < 0)
+#endif
         {
             ::operator delete(old_rep);
         }
     }
     return *this;
 }
 
 inline
 __libcpp_refstring::~__libcpp_refstring() {
     if (__uses_refcount()) {
         _Rep_base* rep = rep_from_data(__imp_);
+#ifdef _LIBCPP_HAS_NO_THREADS
+        if ((rep->count -= count_t(-1)) < 0) {
+#else
         if (__sync_add_and_fetch(&rep->count, count_t(-1)) < 0) {
+#endif
             ::operator delete(rep);
         }
     }
 }
 
 inline
 bool __libcpp_refstring::__uses_refcount() const {
 #ifdef __APPLE__

src/locale.cpp

 {
     call_once(__flag_, __fake_bind(&locale::id::__init, this));
     return __id_ - 1;
 }
 
 void
 locale::id::__init()
 {
+#ifdef _LIBCPP_HAS_NO_THREADS
+    __id_ = ++__next_id;
+#else
     __id_ = __sync_add_and_fetch(&__next_id, 1);
+#endif
 }
 
 // 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))
 {

src/support/runtime/exception_fallback.ipp

 _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
 {
+#ifdef _LIBCPP_HAS_NO_THREADS
+  unexpected_handler old = __unexpected_handler;
+  __unexpected_handler = func;
+  return old;
+#else
   return __sync_lock_test_and_set(&__unexpected_handler, func);
+#endif
 }
 
 unexpected_handler
 get_unexpected() _NOEXCEPT
 {
+#ifdef _LIBCPP_HAS_NO_THREADS
+  return __unexpected_handler;
+#else
   return __sync_fetch_and_add(&__unexpected_handler, (unexpected_handler)0);
+#endif
 
 }
 
 _LIBCPP_NORETURN
 void unexpected()
 {
     (*get_unexpected())();
     // unexpected handler should not return
     terminate();
 }
 
 terminate_handler
 set_terminate(terminate_handler func) _NOEXCEPT
 {
+#ifdef _LIBCPP_HAS_NO_THREADS
+  terminate_handler old = __terminate_handler;
+  __terminate_handler = func;
+  return old;
+#else
   return __sync_lock_test_and_set(&__terminate_handler, func);
+#endif
 }
 
 terminate_handler
 get_terminate() _NOEXCEPT
 {
+#ifdef _LIBCPP_HAS_NO_THREADS
+  return __terminate_handler;
+#else
   return __sync_fetch_and_add(&__terminate_handler, (terminate_handler)0);
-
+#endif
 }
 
 #ifndef __EMSCRIPTEN__ // We provide this in JS
 _LIBCPP_NORETURN
 void
 terminate() _NOEXCEPT
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS

src/support/runtime/new_handler_fallback.ipp

 
 namespace std {
 
 _LIBCPP_SAFE_STATIC static std::new_handler __new_handler;
 
 new_handler
 set_new_handler(new_handler handler) _NOEXCEPT
 {
+#ifdef _LIBCPP_HAS_NO_THREADS
+    auto old = __new_handler;
+    __new_handler = handler;
+    return old;
+#else
     return __sync_lock_test_and_set(&__new_handler, handler);
+#endif
 }
 
 new_handler
 get_new_handler() _NOEXCEPT
 {
+#ifdef _LIBCPP_HAS_NO_THREADS
+    return __new_handler;
+#else
     return __sync_fetch_and_add(&__new_handler, nullptr);
+#endif
 }
 
 } // namespace std