Page MenuHomePhabricator

[Clang] Wrong return type of atomic_is_lock_free
ClosedPublic

Authored by kamleshbhalui on May 6 2020, 10:17 AM.

Diff Detail

Event Timeline

kamleshbhalui created this revision.May 6 2020, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2020, 10:17 AM
jfb added a comment.May 6 2020, 10:57 AM

Just a guess, but maybe the return type was intended to match what a C implementation does with atomic_is_lock_free as a macro: https://en.cppreference.com/w/c/atomic/atomic_is_lock_free
If so, maybe the expectation is that a macro could define atomic_is_lock_free using __atomic_is_lock_free, and expect something compatible with _Bool?
In C++ we use __atomic_is_lock_free in inlined functions, so int to bool is fine.

In other words, the builtin can return int just fine, but we should consider how we expect library implementations to use them before changing the return type.

Clang's <stdatomic.h> uses the builtin as follows:

#define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj)))

... which, combined with the builtin returning int, results in a call having the wrong type. So there's definitely a bug *somewhere*.

I think what happened here is: the return type of the GCC __atomic_is_lock_free / __atomic_always_lock_free builtins was int in GCC 4.6 and earlier, and changed to _Bool in GCC 4.7 and later. Clang's implementation followed GCC's behavior at the time, so returned int, and no-one noticed that GCC changed its behavior in version 4.7. So we still provide the old GCC behavior.

Then, when __c11_atomic_is_lock_free was added, it got an int return type as (I think) a copy-paste error. The __c11 builtins are intended to exactly match the semantics of the C11 atomic functions, so this was always wrong.

I think we should make this change, to all three affected builtin functions.

clang/include/clang/Basic/Builtins.def
755–756

This one is also wrong, and should return *bool*.

jfb added a comment.May 6 2020, 11:56 AM

Clang's <stdatomic.h> uses the builtin as follows:

#define atomic_is_lock_free(obj) __c11_atomic_is_lock_free(sizeof(*(obj)))

... which, combined with the builtin returning int, results in a call having the wrong type. So there's definitely a bug *somewhere*.

I think what happened here is: the return type of the GCC __atomic_is_lock_free / __atomic_always_lock_free builtins was int in GCC 4.6 and earlier, and changed to _Bool in GCC 4.7 and later. Clang's implementation followed GCC's behavior at the time, so returned int, and no-one noticed that GCC changed its behavior in version 4.7. So we still provide the old GCC behavior.

Then, when __c11_atomic_is_lock_free was added, it got an int return type as (I think) a copy-paste error. The __c11 builtins are intended to exactly match the semantics of the C11 atomic functions, so this was always wrong.

I think we should make this change, to all three affected builtin functions.

This sounds sensible to me.

Addressed @rsmith concerns.

ldionne accepted this revision.May 7 2020, 8:50 AM

This change LGTM, libc++ will keep working as-is since we assume the builtins return bool (I checked).

This revision is now accepted and ready to land.May 7 2020, 8:50 AM

I do not have commit access.

What name and email do you want this committed with?

What name and email do you want this committed with?

Kamlesh Kumar
kamleshbhalui@gmail.com

I'll commit this since I think everyone was fine with it.

This revision was automatically updated to reflect the committed changes.