Diagnose const atomics and allow more qualified arguments in __atomic builtins.

[EricWF]

Summary

This patch cleanup the sema checking around the atomic builtins. This patch does two main things:

1. Diagnose when a pointer to const T is used as the first argument in at atomic builtin unless that builtin is a load operation. This is already checked for C11 atomics builtins but not for __atomic ones.
2. For certain atomic builtins, if the second or third argument is a pointer, allow that argument to propagate extra cv-qualifiers on the pointee to the deduced type.

The following builtins are allowed to have a const qualified pointee type as the second argument:

1. __atomic_store
2. __atomic_exchange

The following builtins are allowed to have a volatile qualified pointee type as the second argument:

1. __atomic_store,
2. __atomic_load
3. __atomic_exchange
4. __atomic_compare_exchange
5. __atomic_compare_exchange_n
6. __c11_atomic_compare_exchange_strong.
7. __c11_atomic_compare_exchange_weak.

The following builtins are allowed to have a const qualified pointee type as the third argument:

1. __atomic_exchange
2. __atomic_compare_exchange.

The following builtin are allowed to have a volatile qualified pointee type as the third argument:

1. __atomic_compare_exchange.

[jroelofs]

It probably makes sense to extend this to all of the related builtins that can write to their pointer argument:

__atomic_store
__atomic_store_n
__atomic_exchange
__atomic_exchange
__atomic_compare_exchange
__atomic_compare_exchange_n
__atomic_test_and_set
__atomic_clear

[EricWF]

In D10407#187132, @jroelofs wrote:

It probably makes sense to extend this to all of the related builtins that can write to their pointer argument:

__atomic_store
__atomic_store_n
__atomic_exchange
__atomic_exchange
__atomic_compare_exchange
__atomic_compare_exchange_n
__atomic_test_and_set
__atomic_clear

I agree. I also think this is the wrong way to diagnose this one case where the second argument can't be const but the first can. I'll take another stab at this tomorrow.

[rsmith]

include/clang/Basic/DiagnosticSemaKinds.td

6130

I don't think the %0 here is really adding anything to the diagnostic; if anything, it seems to distract from the point by making the user wonder if there's a difference between their %1 and the expected %0.

lib/Sema/SemaChecking.cpp

1411–1415

It's wasteful to build a converted expression up here and throw it away. What you want seems to be something like:

QualType T = ArgPtr->getType();
auto *PtrType = T->getAs<PointerType>();
return PtrType ? PtrType->getPointeeType().getQualifiers() : T->getQualifiers();

1570–1577

This part LGTM; feel free to factor out this chunk and its tests, and commit it separately.

1682–1683

Please don't do this. The first half of this function is carefully classifying the different forms of atomic builtin so that we don't need to reason about the behavior of individual builtins down here.

It'd be better to add a bool PassInputsByValue = IsC11 || IsN; in place of the existing ByValType, and use either ValType or some pointer type built from it depending on the value of PassInputsByValue.

1689

&& should go at the end of the line, not the start.

1692–1693

} and else should go on the same line.

[EricWF]

Respond to @rsmith's comments.

lib/Sema/SemaChecking.cpp

1411–1415

I'll drop the "S.DefaultFunctionArrayLValueConversion" call and use T->getQualifiers() when the type is not a pointer.

1570–1577

I'll keep it in this patch. Hopefully I can get this in soon.

1682–1683

Ack. Although your formulation for PassInputsByValue is not quite correct. I'll update it with something similar.

1689

ack.

1692–1693

ack.

[EricWF]

Address richards comments.

[jroelofs]

What about __atomic_test_and_set, and __atomic_clear?

[EricWF]

In D10407#187613, @jroelofs wrote:

What about __atomic_test_and_set, and __atomic_clear?

They both have fixed pointer types of void* and bool* respectively so they aren't affected by this bug/change.

[jroelofs]

Oh, right. Never mind.

[EricWF]

Abandoning. I bit off more that I could chew.

I committed the part @rsmith approved separably as r249252.

include/clang/Basic/DiagnosticSemaKinds.td

 def err_atomic_op_needs_non_const_atomic : Error<
   "address argument to atomic operation must be a pointer to non-const _Atomic "
   "type (%0 invalid)">;
+def err_atomic_op_needs_non_const_pointer : Error<
+  "address argument to atomic operation must be a pointer to non-const "
+  "type %0 (%1 invalid)">;
 def err_atomic_op_needs_trivial_copy : Error<
   "address argument to atomic operation must be a pointer to a "
   "trivially-copyable type (%0 invalid)">;

lib/Sema/SemaChecking.cpp

   }
 }
 
+// If the ArgPtr is a pointer type return the qualifiers of the pointee type.
+// Otherwise return an empty set of qualifiers.
+static Qualifiers getPointeeQualifiers(Sema& S, Expr *ArgPtr) {
+  ArgPtr = S.DefaultFunctionArrayLvalueConversion(ArgPtr).get();
+  QualType Ty = ArgPtr->getType();
+  const PointerType *PointerTy = Ty->getAs<PointerType>();
+  return PointerTy ? PointerTy->getPointeeType().getQualifiers()
+                   : Qualifiers();
+}
+
 ExprResult Sema::SemaAtomicOpsOverloaded(ExprResult TheCallResult,
                                          AtomicExpr::AtomicOp Op) {
   CallExpr *TheCall = cast<CallExpr>(TheCallResult.get());

              Op == AtomicExpr::AO__atomic_exchange_n ||
              Op == AtomicExpr::AO__atomic_compare_exchange_n;
   bool IsAddSub = false;
+  // Allow extra const and volatile qualifiers on the pointee type of the
+  // second operand of these functions.
+  bool AllowConstCP = Op == AtomicExpr::AO__atomic_store ||
+                      Op == AtomicExpr::AO__atomic_exchange;
+  bool AllowVolatileCP = Op == AtomicExpr::AO__atomic_store ||
+                         Op == AtomicExpr::AO__atomic_load ||
+                         Op == AtomicExpr::AO__atomic_exchange ||
+                         Op == AtomicExpr::AO__atomic_compare_exchange ||
+                         Op == AtomicExpr::AO__atomic_compare_exchange_n;
 
   switch (Op) {
   case AtomicExpr::AO__c11_atomic_init:

       return ExprError();
     }
     ValType = AtomTy->getAs<AtomicType>()->getValueType();
+  } else if (Form != Load && Op != AtomicExpr::AO__atomic_load) {
+    if (ValType.isConstQualified()) {
+      Diag(DRE->getLocStart(), diag::err_atomic_op_needs_non_const_pointer)
+        << ValType.getUnqualifiedType() << Ptr->getType()
+        << Ptr->getSourceRange();
+      return ExprError();
+    }
   }
 
   // For an arithmetic operation, the implied arithmetic must be well-formed.

     return ExprError();
   }
 
-  // FIXME: For any builtin other than a load, the ValType must not be
-  // const-qualified.
-
   switch (ValType.getObjCLifetime()) {
   case Qualifiers::OCL_None:
   case Qualifiers::OCL_ExplicitNone:

         assert(Form != Load);
         if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()))
           Ty = ValType;
-        else if (Form == Copy || Form == Xchg)
-          Ty = ByValType;
         else if (Form == Arithmetic)
           Ty = Context.getPointerDiffType();
-        else
-          Ty = Context.getPointerType(ValType.getUnqualifiedType());
+        // Handle the Copy and Xchg atomics that don't take a pointer as the
+        // second argument.
+        else if ((Form == Copy && (IsN || IsC11)) || Form == Xchg)
+          Ty = ValType;
+        else {
+          // If the second type is a pointer propagate the cv-qualifiers from
+          // the pointee type to the deduced type wherever we are allowed to.
+          Qualifiers SecondParamQuals = getPointeeQualifiers(*this, TheCall->getArg(1));
+          Ty = ValType.getUnqualifiedType();
+          if (SecondParamQuals.hasVolatile() && AllowVolatileCP)
+            Ty.addVolatile();
+          if (SecondParamQuals.hasConst() && AllowConstCP)
+            Ty.addConst();
+          Ty = Context.getPointerType(Ty);
+        }
         break;
       case 2:
-        // The third argument to compare_exchange / GNU exchange is a
-        // (pointer to a) desired value.
-        Ty = ByValType;
+        // Handle the Xchg and CmpXchg builtins that take a pointer
+        // as their third argument.
+        if (Op == AtomicExpr::AO__atomic_exchange ||
+            Op == AtomicExpr::AO__atomic_compare_exchange) {
+          Qualifiers ThirdParamQuals = getPointeeQualifiers(*this, TheCall->getArg(2));
+          Ty = ValType.getUnqualifiedType();
+          if (ThirdParamQuals.hasVolatile())
+            Ty.addVolatile();
+          if (Op == AtomicExpr::AO__atomic_compare_exchange
+              && ThirdParamQuals.hasConst())
+            Ty.addConst();
+          Ty = Context.getPointerType(Ty);
+        }
+        else
+          Ty = ByValType;
         break;
       case 3:
         // The fourth argument to GNU compare_exchange is a 'weak' flag.

test/Sema/atomic-ops.c

 _Static_assert(!__atomic_always_lock_free(8, &i32), "");
 _Static_assert(__atomic_always_lock_free(8, &i64), "");
 
-void f(_Atomic(int) *i, _Atomic(int*) *p, _Atomic(float) *d,
-       int *I, int **P, float *D, struct S *s1, struct S *s2) {
+void f(_Atomic(int) *i, const _Atomic(int) *ci, volatile _Atomic(int) *vi,
+       const volatile _Atomic(int) *cvi, _Atomic(int*) *p, _Atomic(float) *d,
+       int *I, const int *CI, volatile int *VI, const volatile int* CVI,
+       int **P, float *D, struct S *s1, struct S *s2) {
   __c11_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
+  __c11_atomic_init(ci, 5); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
+  __c11_atomic_init(vi, 5);
+  __c11_atomic_init(cvi, 5);  // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const volatile _Atomic(int) *' invalid)}}
   __c11_atomic_load(0); // expected-error {{too few arguments to function}}
   __c11_atomic_load(0,0,0); // expected-error {{too many arguments to function}}
   __c11_atomic_store(0,0,0); // expected-error {{address argument to atomic builtin must be a pointer}}
   __c11_atomic_store((int*)0,0,0); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
+  __c11_atomic_store(i, 0, memory_order_relaxed);
+  __c11_atomic_store(vi, 0, memory_order_relaxed);
+  __c11_atomic_store(ci, 0, memory_order_relaxed); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
+  __c11_atomic_store(cvi, 0, memory_order_relaxed); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const volatile _Atomic(int) *' invalid)}}
 
   __c11_atomic_load(i, memory_order_seq_cst);
   __c11_atomic_load(p, memory_order_seq_cst);
   __c11_atomic_load(d, memory_order_seq_cst);
+  __c11_atomic_load(vi, memory_order_seq_cst);
+  __c11_atomic_load(ci, memory_order_seq_cst); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(int) *' invalid)}}
+  __c11_atomic_load(cvi, memory_order_seq_cst); // expected-error {{address argument to atomic operation must be a pointer to non-const _Atomic type ('const volatile _Atomic(int) *' invalid)}}
 
   int load_n_1 = __atomic_load_n(I, memory_order_relaxed);
   int *load_n_2 = __atomic_load_n(P, memory_order_relaxed);
   float load_n_3 = __atomic_load_n(D, memory_order_relaxed); // expected-error {{must be a pointer to integer or pointer}}
   __atomic_load_n(s1, memory_order_relaxed); // expected-error {{must be a pointer to integer or pointer}}
+  load_n_1 = __atomic_load_n(VI, memory_order_relaxed);
+  load_n_1 = __atomic_load_n(CI, memory_order_relaxed);
+  load_n_1 = __atomic_load_n(CVI, memory_order_relaxed);
 
   __atomic_load(i, I, memory_order_relaxed); // expected-error {{must be a pointer to a trivially-copyable type}}
+  __atomic_load(CI, I, memory_order_relaxed);
+  __atomic_load(VI, I, memory_order_relaxed);
+  __atomic_load(CVI, I, memory_order_relaxed);
+  __atomic_load(VI, VI, memory_order_relaxed);
+  __atomic_load(CVI, VI, memory_order_relaxed);
+
   __atomic_load(I, i, memory_order_relaxed); // expected-warning {{passing '_Atomic(int) *' to parameter of type 'int *'}}
   __atomic_load(I, *P, memory_order_relaxed);
   __atomic_load(I, *P, memory_order_relaxed, 42); // expected-error {{too many arguments}}
   (int)__atomic_load(I, I, memory_order_seq_cst); // expected-error {{operand of type 'void'}}
   __atomic_load(s1, s2, memory_order_acquire);
-
+  (void)__atomic_load(CI, CI, memory_order_relaxed); // expected-warning {{passing 'const int *' to parameter of type 'int *' discards qualifiers}}
+  (void)__atomic_load(CVI, VI, memory_order_relaxed);
   __c11_atomic_store(i, 1, memory_order_seq_cst);
   __c11_atomic_store(p, 1, memory_order_seq_cst); // expected-warning {{incompatible integer to pointer conversion}}
   (int)__c11_atomic_store(d, 1, memory_order_seq_cst); // expected-error {{operand of type 'void'}}
 
   __atomic_store_n(I, 4, memory_order_release);
   __atomic_store_n(I, 4.0, memory_order_release);
+  __atomic_store_n(VI, 4, memory_order_release);
+  __atomic_store_n(CI, 4, memory_order_release); // expected-error {{address argument to atomic operation must be a pointer to non-const type 'int' ('const int *' invalid)}}
+  __atomic_store_n(CVI, 4, memory_order_release); // expected-error {{address argument to atomic operation must be a pointer to non-const type 'int' ('const volatile int *' invalid)}}
   __atomic_store_n(I, P, memory_order_release); // expected-warning {{parameter of type 'int'}}
   __atomic_store_n(i, 1, memory_order_release); // expected-error {{must be a pointer to integer or pointer}}
   __atomic_store_n(s1, *s2, memory_order_release); // expected-error {{must be a pointer to integer or pointer}}
+  __atomic_store_n(I, I, memory_order_release); // expected-warning {{incompatible pointer to integer conversion passing 'int *' to parameter of type 'int'; dereference with *}}
 
   __atomic_store(I, *P, memory_order_release);
+  __atomic_store(VI, CVI, memory_order_release);
+  __atomic_store(VI, I, memory_order_release);
+  __atomic_store(VI, VI, memory_order_release);
+  __atomic_store(CI, I, memory_order_release); // expected-error {{address argument to atomic operation must be a pointer to non-const type 'int' ('const int *' invalid)}}
+  __atomic_store(CVI, I, memory_order_release); // expected-error {{address argument to atomic operation must be a pointer to non-const type 'int' ('const volatile int *' invalid)}}
   __atomic_store(s1, s2, memory_order_release);
   __atomic_store(i, I, memory_order_release); // expected-error {{trivially-copyable}}
 

   __atomic_exchange(s1, s2, s2, memory_order_seq_cst);
   __atomic_exchange(s1, I, P, memory_order_seq_cst); // expected-warning 2{{parameter of type 'struct S *'}}
   (int)__atomic_exchange(s1, s2, s2, memory_order_seq_cst); // expected-error {{operand of type 'void'}}
+  __atomic_exchange(I, I, I, memory_order_seq_cst);
+  __atomic_exchange(VI, I, I, memory_order_seq_cst);
+  __atomic_exchange(VI, CI, VI, memory_order_seq_cst);
+  __atomic_exchange(CI, I, I, memory_order_seq_cst); // expected-error {{address argument to atomic operation must be a pointer to non-const type 'int' ('const int *' invalid)}}
+  __atomic_exchange(I, I, CI, memory_order_seq_cst); // expected-warning {{passing 'const int *' to parameter of type 'int *' discards qualifiers}}
 
   __c11_atomic_fetch_add(i, 1, memory_order_seq_cst);
   __c11_atomic_fetch_add(p, 1, memory_order_seq_cst);

   _Bool cmpexch_4 = __atomic_compare_exchange_n(I, I, 5, 1, memory_order_seq_cst, memory_order_seq_cst);
   _Bool cmpexch_5 = __atomic_compare_exchange_n(I, P, 5, 0, memory_order_seq_cst, memory_order_seq_cst); // expected-warning {{; dereference with *}}
   _Bool cmpexch_6 = __atomic_compare_exchange_n(I, I, P, 0, memory_order_seq_cst, memory_order_seq_cst); // expected-warning {{passing 'int **' to parameter of type 'int'}}
+  (void)__atomic_compare_exchange_n(VI, I, 5, 1, memory_order_seq_cst, memory_order_seq_cst);
+  (void)__atomic_compare_exchange_n(I, VI, 5, 1, memory_order_seq_cst, memory_order_seq_cst);
+  (void)__atomic_compare_exchange_n(VI, VI, 5, 1, memory_order_seq_cst, memory_order_seq_cst);
+  (void)__atomic_compare_exchange_n(CI, I, 5, 1, memory_order_seq_cst, memory_order_seq_cst); // expected-error {{address argument to atomic operation must be a pointer to non-const type 'int' ('const int *' invalid)}}
+  (void)__atomic_compare_exchange_n(I, CI, 5, 1, memory_order_seq_cst, memory_order_seq_cst); // expected-warning {{passing 'const int *' to parameter of type 'int *' discards qualifiers}}
 
   _Bool cmpexch_7 = __atomic_compare_exchange(I, I, 5, 1, memory_order_seq_cst, memory_order_seq_cst); // expected-warning {{passing 'int' to parameter of type 'int *'}}
   _Bool cmpexch_8 = __atomic_compare_exchange(I, P, I, 0, memory_order_seq_cst, memory_order_seq_cst); // expected-warning {{; dereference with *}}
   _Bool cmpexch_9 = __atomic_compare_exchange(I, I, I, 0, memory_order_seq_cst, memory_order_seq_cst);
+  (void)__atomic_compare_exchange(VI, I, I, 0, memory_order_seq_cst, memory_order_seq_cst);
+  (void)__atomic_compare_exchange(I, VI, CVI, 0, memory_order_seq_cst, memory_order_seq_cst);
+  (void)__atomic_compare_exchange(VI, VI, CVI, 0, memory_order_seq_cst, memory_order_seq_cst);
+  (void)__atomic_compare_exchange(CI, I, I, 0, memory_order_seq_cst, memory_order_seq_cst); // expected-error {{address argument to atomic operation must be a pointer to non-const type 'int' ('const int *' invalid)}}
+  (void)__atomic_compare_exchange(I, CI, I, 0, memory_order_seq_cst, memory_order_seq_cst); // expected-warning {{passing 'const int *' to parameter of type 'int *' discards qualifiers}}
 
   const volatile int flag_k = 0;
   volatile int flag = 0;