[Clang] Wrong return type of atomic_is_lock_free

[kamleshbhalui]

Summary

Fixing the return type of atomic_is_lock_free as per
https://en.cppreference.com/w/c/atomic/atomic_is_lock_free

[jfb]

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.

[rsmith]

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]

In D79504#2023274, @rsmith wrote:

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.

[kamleshbhalui]

Addressed @rsmith concerns.

[ldionne]

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

[kamleshbhalui]

I do not have commit access.

[ldionne]

What name and email do you want this committed with?

[kamleshbhalui]

In D79504#2029267, @ldionne wrote:

What name and email do you want this committed with?

Kamlesh Kumar
[e-mail]

[ldionne]

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

clang/include/clang/Basic/Builtins.def

 ATOMIC_BUILTIN(__c11_atomic_fetch_sub, "v.", "t")
 ATOMIC_BUILTIN(__c11_atomic_fetch_and, "v.", "t")
 ATOMIC_BUILTIN(__c11_atomic_fetch_or, "v.", "t")
 ATOMIC_BUILTIN(__c11_atomic_fetch_xor, "v.", "t")
 ATOMIC_BUILTIN(__c11_atomic_fetch_max, "v.", "t")
 ATOMIC_BUILTIN(__c11_atomic_fetch_min, "v.", "t")
 BUILTIN(__c11_atomic_thread_fence, "vi", "n")
 BUILTIN(__c11_atomic_signal_fence, "vi", "n")
-BUILTIN(__c11_atomic_is_lock_free, "iz", "n")
+BUILTIN(__c11_atomic_is_lock_free, "bz", "n")
 
 // GNU atomic builtins.
 ATOMIC_BUILTIN(__atomic_load, "v.", "t")
 ATOMIC_BUILTIN(__atomic_load_n, "v.", "t")
 ATOMIC_BUILTIN(__atomic_store, "v.", "t")
 ATOMIC_BUILTIN(__atomic_store_n, "v.", "t")
 ATOMIC_BUILTIN(__atomic_exchange, "v.", "t")
 ATOMIC_BUILTIN(__atomic_exchange_n, "v.", "t")

 ATOMIC_BUILTIN(__atomic_or_fetch, "v.", "t")
 ATOMIC_BUILTIN(__atomic_xor_fetch, "v.", "t")
 ATOMIC_BUILTIN(__atomic_max_fetch, "v.", "t")
 ATOMIC_BUILTIN(__atomic_min_fetch, "v.", "t")
 ATOMIC_BUILTIN(__atomic_nand_fetch, "v.", "t")
 BUILTIN(__atomic_test_and_set, "bvD*i", "n")
 BUILTIN(__atomic_clear, "vvD*i", "n")
 BUILTIN(__atomic_thread_fence, "vi", "n")
 BUILTIN(__atomic_signal_fence, "vi", "n")
-BUILTIN(__atomic_always_lock_free, "izvCD*", "n")
-BUILTIN(__atomic_is_lock_free, "izvCD*", "n")
+BUILTIN(__atomic_always_lock_free, "bzvCD*", "n")
+BUILTIN(__atomic_is_lock_free, "bzvCD*", "n")
 
 // OpenCL 2.0 atomic builtins.
 ATOMIC_BUILTIN(__opencl_atomic_init, "v.", "t")
 ATOMIC_BUILTIN(__opencl_atomic_load, "v.", "t")
 ATOMIC_BUILTIN(__opencl_atomic_store, "v.", "t")
 ATOMIC_BUILTIN(__opencl_atomic_exchange, "v.", "t")
 ATOMIC_BUILTIN(__opencl_atomic_compare_exchange_strong, "v.", "t")
 ATOMIC_BUILTIN(__opencl_atomic_compare_exchange_weak, "v.", "t")

clang/test/CodeGen/atomic-ops.c

struct Seventeen {
   char c[17];
 } seventeen;
 
 struct Incomplete;
 
 int lock_free(struct Incomplete *incomplete) {
   // CHECK-LABEL: @lock_free
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 3, i8* null)
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 3, i8* null)
   __c11_atomic_is_lock_free(3);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 16, i8* {{.*}}@sixteen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 16, i8* {{.*}}@sixteen{{.*}})
   __atomic_is_lock_free(16, &sixteen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 17, i8* {{.*}}@seventeen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 17, i8* {{.*}}@seventeen{{.*}})
   __atomic_is_lock_free(17, &seventeen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i32 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 4, {{.*}})
   __atomic_is_lock_free(4, incomplete);
 
   char cs[20];
-  // CHECK: call i32 @__atomic_is_lock_free(i32 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i32 4, {{.*}})
   __atomic_is_lock_free(4, cs+1);
 
   // CHECK-NOT: call
   __atomic_always_lock_free(3, 0);
   __atomic_always_lock_free(16, 0);
   __atomic_always_lock_free(17, 0);
   __atomic_always_lock_free(16, &sixteen);
   __atomic_always_lock_free(17, &seventeen);

clang/test/CodeGen/big-atomic-ops.c

 } sixteen;
 struct Seventeen {
   char c[17];
 } seventeen;
 
 int lock_free(struct Incomplete *incomplete) {
   // CHECK: @lock_free
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 3, i8* null)
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 3, i8* null)
   __c11_atomic_is_lock_free(3);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 16, i8* {{.*}}@sixteen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 16, i8* {{.*}}@sixteen{{.*}})
   __atomic_is_lock_free(16, &sixteen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 17, i8* {{.*}}@seventeen{{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 17, i8* {{.*}}@seventeen{{.*}})
   __atomic_is_lock_free(17, &seventeen);
 
-  // CHECK: call i32 @__atomic_is_lock_free(i64 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 4, {{.*}})
   __atomic_is_lock_free(4, incomplete);
 
   char cs[20];
-  // CHECK: call i32 @__atomic_is_lock_free(i64 4, {{.*}})
+  // CHECK: call zeroext i1 @__atomic_is_lock_free(i64 4, {{.*}})
   __atomic_is_lock_free(4, cs+1);
 
   // CHECK-NOT: call
   __atomic_always_lock_free(3, 0);
   __atomic_always_lock_free(16, 0);
   __atomic_always_lock_free(17, 0);
   __atomic_always_lock_free(16, &sixteen);
   __atomic_always_lock_free(17, &seventeen);