Page MenuHomePhabricator

[libc++] Refactoring __sync_* builtins; NFC
ClosedPublic

Authored by weimingz on Jun 30 2017, 4:36 PM.

Diff Detail

Event Timeline

weimingz created this revision.Jun 30 2017, 4:36 PM
joerg added a subscriber: joerg.Jul 1 2017, 3:37 AM

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.

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 edited edge metadata.Jul 4 2017, 2:54 PM

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.

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 or the 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.

EricWF requested changes to this revision.Jul 4 2017, 2:54 PM
This revision now requires changes to proceed.Jul 4 2017, 2:54 PM

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.

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 or the 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)

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.

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

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 updated this revision to Diff 105565.Jul 6 2017, 5:29 PM
weimingz edited edge metadata.

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

weimingz updated this revision to Diff 105566.Jul 6 2017, 5:31 PM
jroelofs edited edge metadata.Jul 7 2017, 3:36 PM

@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 retitled this revision from [libc++] Avoid atomic built-ins for NO_THREADS build to [libc++] Refactoring __sync_* builtins; NFC.Jul 10 2017, 1:41 PM
weimingz edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
weimingz updated this revision to Diff 105924.Jul 10 2017, 2:31 PM

missed to upload the change to include __atomic_support header file

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

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?

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

EricWF reopened this revision.Jul 11 2017, 6:15 PM

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.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 7 2019, 5:49 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 5:49 AM