[PATCH] D34918: [libc++] Avoid atomic built-ins for NO_THREADS build

[Eric Fiselier via Phabricator via llvm-commits]

EricWF added a comment.

> 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 https://reviews.llvm.org/D34918#798422, @weimingz wrote:

> In https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D34918