Disable use of _ExtInt with '__atomic' builtins

[jtmott-intel]

Summary

We're (temporarily) disabling ExtInt for the '__atomic' builtins so we can better design their behavior later. The idea is until we do an audit/design for the way atomic builtins are supposed to work with _ExtInt, we should leave them restricted so they don't limit our future options, such as by binding us to a sub-optimal implementation via ABI.

Example after this change:

$ cat test.c

    void f(_ExtInt(64) *ptr) {
      __atomic_fetch_add(ptr, 1, 0);
    }

$ clang -c test.c

    test.c:2:22: error: argument to atomic builtin of type '_ExtInt' is not supported
      __atomic_fetch_add(ptr, 1, 0);
                         ^
    1 error generated.

[jtmott-intel]

This change is intended to be merged into the release/11 branch.

[erichkeane]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7970

I don't think this error message is proper. The 'please file a bug to request support' isn't something that we do.

Also, the odd 'ticks' are not something we use either. Hopefully someone can suggest better, but I'd say:

"argument to atomic builtin of type '_ExtInt' is not supported"

[jtmott-intel]

Updated diagnostic message. Unless someone suggests something different, I used Erich's message.

[jtmott-intel]

Hi @jfb . This commit it meant to address the concerns raised in https://reviews.llvm.org/D83340 . Thanks!

[jfb]

I think you also want tests with the following:

#include <atomic>

_Atomic _ExtInt(8) ae0;
_Atomic(_ExtInt(8)) ae1;
std::atomic<_ExtInt(8)> ae3;

[jfb]

Might be worth thinking about what volatile _ExtInt(1) ought to do as well :)

[jtmott-intel]

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

[jfb]

In D84049#2181995, @jtmott-intel wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

[erichkeane]

In D84049#2182011, @jfb wrote:

In D84049#2181995, @jtmott-intel wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

[jfb]

In D84049#2182310, @erichkeane wrote:

In D84049#2182011, @jfb wrote:

In D84049#2181995, @jtmott-intel wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

_Atomic and std::atomic ought to be compatible, they were designed to be. The original point I made was: "_ExtInt doesn't seem to be designed to work with atomic, please do something about it". It seems like disabling it for _Atomic is what you'd want at the moment, until a design for the entire thing is created?

We can ping @jwakely for libstdc++ 😃

[erichkeane]

In D84049#2182370, @jfb wrote:

In D84049#2182310, @erichkeane wrote:

In D84049#2182011, @jfb wrote:

In D84049#2181995, @jtmott-intel wrote:

Hi, @jfb.

_Atomic appears to already have special logic for _ExtInt in clang::Sema::BuildAtomicType. If _Atomic is used with an _ExtInt whose size is not a power of 2, or whose size is less then 8 bits, then it's disallowed. Those cases already have tests in clang/test/SemaCXX/ext-int.cpp.

Interesting. It seems like __atomic ought to be consistent with this, and we'd make it official ABI?

std::atomic appears to use and call the __atomic builtins, so it emits this patch's new diagnostic that disallows _ExtInt. Unfortunately I can't add the std::atomic<_ExtInt(N)> to a sema lit test because I can't #include the real atomic, but I did verify it locally.

Can you add the test to libc++? @ldionne WDYT?

IF we are going as far as modifying libc++, perhaps we can just add a SFINAE test to std::atomic to disallow the _ExtInt types? That said, it won't help with libstdc++.

Either way, I suspect that is better placed in a different patch, and should no longer hold up this patch.

_Atomic and std::atomic ought to be compatible, they were designed to be. The original point I made was: "_ExtInt doesn't seem to be designed to work with atomic, please do something about it". It seems like disabling it for _Atomic is what you'd want at the moment, until a design for the entire thing is created?

We can ping @jwakely for libstdc++ 😃

Ah, yes, a followup to disable this for the _Atomic types is probably a good idea. << @jtmott-intel
For context:
https://en.cppreference.com/w/c/language/atomic

It should be a lot like this diagnostic here: https://godbolt.org/z/K4Wrnf

[jtmott-intel]

Modified _Atomic behavior to disallow *all* _ExtInt types of any size, which matches the behavior of __atomic builtins and std::atomic.

[ldionne]

I think it makes sense to add a libc++ test (as libcxx/test/libcxx/atomics/ext-int.verify.cpp or similar). You can use the usual expected-error markup in those tests, they run under clang-verify.

But I'm not opposed to this patch getting in without the corresponding libc++ test (although it would be easy to add).

[jtmott-intel]

@ldionne @jfb Added libcxx test.

[jwakely]

In D84049#2182370, @jfb wrote:

We can ping @jwakely for libstdc++ 😃

Whaddya want?

I don't even know what _ExtInt is. If Clang's __atomic_xxx built-ins give a diagnostic for _ExtInt types then libstdc++'s std::atomic shouldn't be able to work with them, because we just use the built-ins. Does Clang give the diagnostic even in system headers? That might be the only caveat.

[jtmott-intel]

Great! sounds like this might be resolved to everyone's satisfaction? I'll give this until end of Monday for other comments. cc @jfb

[jfb]

@ldionne should sign off on the libcxx test, but the rest seems fine to me. Eventually we probably want to enable something, but need to be consistent about it.

[jtmott-intel]

Committed to master, and created bug to cherry pick into 11.0.

https://bugs.llvm.org/show_bug.cgi?id=47222

[hans]

In D84049#2224061, @jtmott-intel wrote:

Committed to master, and created bug to cherry pick into 11.0.

https://bugs.llvm.org/show_bug.cgi?id=47222

Pushed to 11.x as 82e48a579024d0ffbc352702ec0c52b47a6fe691. Thanks!

[ldionne]

libcxx/test/libcxx/atomics/ext-int.verify.cpp

1

This isn't great, since it won't run on clang-12, etc. I'll change it to:

// UNSUPPORTED: clang-4, clang-5, clang-6, clang-7, clang-8, clang-9, clang-10
// UNSUPPORTED: apple-clang-9, apple-clang-10, apple-clang-11

[jtmott-intel]

libcxx/test/libcxx/atomics/ext-int.verify.cpp

1

Thanks! That seems better.

[ldionne]

libcxx/test/libcxx/atomics/ext-int.verify.cpp

1

8df143c2cc0a3b49440cafe8a80b0f72cee6f166

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_return_block_has_expr : Error<"void block should not return a value">;
 def err_block_return_missing_expr : Error<
   "non-void block should return a value">;
 def err_func_def_incomplete_result : Error<
   "incomplete result type %0 in function definition">;
 def err_atomic_specifier_bad_type
     : Error<"_Atomic cannot be applied to "
             "%select{incomplete |array |function |reference |atomic |qualified "
-            "|sizeless ||integer |integer }0type "
-            "%1 %select{|||||||which is not trivially copyable|with less than "
-            "1 byte of precision|with a non power of 2 precision}0">;
+            "|sizeless ||integer }0type "
+            "%1 %select{|||||||which is not trivially copyable|}0">;
 
 // Expressions.
 def ext_sizeof_alignof_function_type : Extension<
   "invalid application of '%0' to a function type">, InGroup<PointerArith>;
 def ext_sizeof_alignof_void_type : Extension<
   "invalid application of '%0' to a void type">, InGroup<PointerArith>;
 def err_opencl_sizeof_alignof_type : Error<
   "invalid application of '%0' to a void type">;

 def err_atomic_builtin_pointer_size : Error<
   "address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte "
   "type (%0 invalid)">;
 def err_atomic_exclusive_builtin_pointer_size : Error<
   "address argument to load or store exclusive builtin must be a pointer to"
   " 1,2,4 or 8 byte type (%0 invalid)">;
 def err_atomic_builtin_ext_int_size : Error<
   "Atomic memory operand must have a power-of-two size">;
+def err_atomic_builtin_ext_int_prohibit : Error<
+  "argument to atomic builtin of type '_ExtInt' is not supported">;
 def err_atomic_op_needs_atomic : Error<
   "address argument to atomic operation must be a pointer to _Atomic "
   "type (%0 invalid)">;
 def err_atomic_op_needs_non_const_atomic : Error<
   "address argument to atomic operation must be a pointer to non-%select{const|constant}0 _Atomic "
   "type (%1 invalid)">;
 def err_atomic_op_needs_non_const_pointer : Error<
   "address argument to atomic operation must be a pointer to non-const "

clang/lib/Sema/SemaChecking.cpp

if ((Op == AtomicExpr::AO__c11_atomic_load ||
        Op == AtomicExpr::AO__opencl_atomic_store ) &&
       Context.AtomicUsesUnsupportedLibcall(AE))
     Diag(AE->getBeginLoc(), diag::err_atomic_load_store_uses_lib)
         << ((Op == AtomicExpr::AO__c11_atomic_load ||
              Op == AtomicExpr::AO__opencl_atomic_load)
                 ? 0
                 : 1);
 
+  if (ValType->isExtIntType()) {
+    Diag(Ptr->getExprLoc(), diag::err_atomic_builtin_ext_int_prohibit);
+    return ExprError();
+  }
+
   return AE;
 }
 
 /// checkBuiltinArgument - Given a call to a builtin function, perform
 /// normal type-checking on the given argument, updating the call in
 /// place.  This is useful when a builtin function requires custom
 /// type-checking for some of its arguments but not necessarily all of
 /// them.

clang/lib/Sema/SemaType.cpp

else if (T->isAtomicType())
       DisallowedKind = 4;
     else if (T.hasQualifiers())
       DisallowedKind = 5;
     else if (T->isSizelessType())
       DisallowedKind = 6;
     else if (!T.isTriviallyCopyableType(Context))
       // Some other non-trivially-copyable type (probably a C++ class)
       DisallowedKind = 7;
-    else if (auto *ExtTy = T->getAs<ExtIntType>()) {
-      if (ExtTy->getNumBits() < 8)
+    else if (T->isExtIntType()) {
         DisallowedKind = 8;
-      else if (!llvm::isPowerOf2_32(ExtTy->getNumBits()))
-        DisallowedKind = 9;
     }
 
     if (DisallowedKind != -1) {
       Diag(Loc, diag::err_atomic_specifier_bad_type) << DisallowedKind << T;
       return QualType();
     }
 
     // FIXME: Do we need any handling for ARC here?
   }
 
   // Build the pointer type.
   return Context.getAtomicType(T);
 }

clang/test/Sema/builtins.c

void test21(const int *ptr) {
   __sync_fetch_and_add(ptr, 1); // expected-error{{address argument to atomic builtin cannot be const-qualified ('const int *' invalid)}}
   __atomic_fetch_add(ptr, 1, 0);  // expected-error {{address argument to atomic operation must be a pointer to non-const type ('const int *' invalid)}}
 }
 
 void test_ei_i42i(_ExtInt(42) *ptr, int value) {
   __sync_fetch_and_add(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
   // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
   __sync_nand_and_fetch(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
+
+  __atomic_fetch_add(ptr, 1, 0); // expected-error {{argument to atomic builtin of type '_ExtInt' is not supported}}
 }
 
 void test_ei_i64i(_ExtInt(64) *ptr, int value) {
   __sync_fetch_and_add(ptr, value); // expect success
   // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
   __sync_nand_and_fetch(ptr, value); // expect success
+
+  __atomic_fetch_add(ptr, 1, 0); // expected-error {{argument to atomic builtin of type '_ExtInt' is not supported}}
 }
 
 void test_ei_ii42(int *ptr, _ExtInt(42) value) {
   __sync_fetch_and_add(ptr, value); // expect success
   // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
   __sync_nand_and_fetch(ptr, value); // expect success
 }
 

clang/test/SemaCXX/ext-int.cpp

 // Reject vector types:
 // expected-error@+1{{invalid vector element type '_ExtInt(32)'}}
 typedef _ExtInt(32) __attribute__((vector_size(16))) VecTy;
 
 // Allow _Complex:
 _Complex _ExtInt(3) Cmplx;
 
 // Reject cases of _Atomic:
-// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(4)' with less than 1 byte of precision}}
+// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(4)'}}
 _Atomic _ExtInt(4) TooSmallAtomic;
-// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(9)' with a non power of 2 precision}}
+// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(9)'}}
 _Atomic _ExtInt(9) NotPow2Atomic;
+// expected-error@+1{{_Atomic cannot be applied to integer type '_ExtInt(128)'}}
 _Atomic _ExtInt(128) JustRightAtomic;
 
 // Test result types of Unary/Bitwise/Binary Operations:
 void Ops() {
   _ExtInt(43) x43_s = 1, y43_s = 1;
   _ExtInt(sizeof(int) * 8) x32_s = 1, y32_s = 1;
   unsigned _ExtInt(sizeof(unsigned) * 8) x32_u = 1, y32_u = 1;
   _ExtInt(4) x4_s = 1, y4_s = 1;

libcxx/test/libcxx/atomics/ext-int.verify.cpp

+// REQUIRES: clang-11
+
+#include <atomic>
+
+int main(int, char**)
+{
+  // expected-error@atomic:*1 {{_Atomic cannot be applied to integer type '_ExtInt(32)'}}
+  std::atomic<_ExtInt(32)> x {42};
+
+  return 0;
+}