Make atomic non-member functions as nonnull

[jfb]

Summary

As a companion to libc++ patch https://reviews.llvm.org/D47225, mark builtin atomic non-member functions which accept pointers as nonnull.

The atomic non-member functions accept pointers to std::atomic / std::atomic_flag as well as to the non-atomic value. These are all dereferenced unconditionally when lowered, and therefore will fault if null. It's a tiny gotcha for new users, especially when they pass in NULL as expected value (instead of passing a pointer to a NULL value).

rdar://problem/18473124

[arphaman]

lib/Sema/SemaChecking.cpp

3497 ↗

(On Diff #148104)

Nit: might make more sense to check if ByValType is a pointer here instead of duplicating the if condition from above.

[jfb]

• Address nit.

[jfb]

Addressed nit.

[rsmith]

Either the previous version of this patch or a version with a bool factored out seem OK to me.

lib/Sema/SemaChecking.cpp

3497 ↗

(On Diff #148104)

I think the suggestion was to check ByValType->isPointerType() here. But... that doesn't work, because we could have a pointer value being passed by value (where a null is allowed), rather than a value being passed by address (where a null is disallowed). I think this comparison is actually less clear than the !IsC11 && !IsN check; factoring out a bool IsPassedByAddress or similar instead would aid readability here.

[jfb]

• Address nit.
• Change suggested by Richard

[jfb]

lib/Sema/SemaChecking.cpp

3497 ↗

(On Diff #148104)

I went with the bool as you suggested.

[jakehehrlich]

This is causing breaks in fuchsia,

Code that looks like this

uintptr_t last_unlogged =
     atomic_load_explicit(&unlogged_tail, memory_order_acquire);
 do { 
     if (last_unlogged == 0)
         return;
 } while (!atomic_compare_exchange_weak_explicit(&unlogged_tail,
                                                 &last_unlogged, 0,
                                                 memory_order_acq_rel,
                                                 memory_order_relaxed));

Where unlogged_tail is somewhere on the stack. And 'atomic_compare_exchange_weak_explicit' is an #define alias for '__c11_atomic_compare_exchange_weak'

Full context here: https://fuchsia.googlesource.com/zircon/+/master/third_party/ulib/musl/ldso/dynlink.c#822

[jfb]

In D47229#1112549, @jakehehrlich wrote:

This is causing breaks in fuchsia,

Code that looks like this

uintptr_t last_unlogged =
     atomic_load_explicit(&unlogged_tail, memory_order_acquire);
 do { 
     if (last_unlogged == 0)
         return;
 } while (!atomic_compare_exchange_weak_explicit(&unlogged_tail,
                                                 &last_unlogged, 0,
                                                 memory_order_acq_rel,
                                                 memory_order_relaxed));

Where unlogged_tail is somewhere on the stack. And 'atomic_compare_exchange_weak_explicit' is an #define alias for '__c11_atomic_compare_exchange_weak'

Full context here: https://fuchsia.googlesource.com/zircon/+/master/third_party/ulib/musl/ldso/dynlink.c#822

Here's a repro for what seems to be happening:

#include <stdatomic.h>
void foo() {
  atomic_int s;
  int a;
  atomic_compare_exchange_weak_explicit(&s, &a, 0, memory_order_acq_rel, memory_order_relaxed);
}

Yields:

./foo.c:5:3: warning: null passed to a callee that requires a non-null argument [-Wnonnull]
  atomic_compare_exchange_weak_explicit(&s, &a, 0, memory_order_acq_rel, memory_order_relaxed);
  ^                                             ~
/s/llvm1/llvm/debug/lib/clang/7.0.0/include/stdatomic.h:144:47: note: expanded from macro 'atomic_compare_exchange_weak_explicit'
#define atomic_compare_exchange_weak_explicit __c11_atomic_compare_exchange_weak
                                              ^

The problem is that my patch checks that the "desired" value is non-null, but that's incorrect because it's not a pointer. I'll do a follow-up fix. Thanks for catching this!

[jfb]

Fixed by r333290.

cfe/trunk/lib/Sema/SemaChecking.cpp

ExprResult Sema::SemaAtomicOpsOverloaded(ExprResult TheCallResult,
   case Qualifiers::OCL_Autoreleasing:
     // FIXME: Can this happen? By this point, ValType should be known
     // to be trivially copyable.
     Diag(DRE->getLocStart(), diag::err_arc_atomic_ownership)
       << ValType << Ptr->getSourceRange();
     return ExprError();
   }
 
-  // atomic_fetch_or takes a pointer to a volatile 'A'.  We shouldn't let the
-  // volatile-ness of the pointee-type inject itself into the result or the
-  // other operands. Similarly atomic_load can take a pointer to a const 'A'.
+  // All atomic operations have an overload which takes a pointer to a volatile
+  // 'A'.  We shouldn't let the volatile-ness of the pointee-type inject itself
+  // into the result or the other operands. Similarly atomic_load takes a
+  // pointer to a const 'A'.
   ValType.removeLocalVolatile();
   ValType.removeLocalConst();
   QualType ResultType = ValType;
   if (Form == Copy || Form == LoadCopy || Form == GNUXchg ||
       Form == Init)
     ResultType = Context.VoidTy;
   else if (Form == C11CmpXchg || Form == GNUCmpXchg)
     ResultType = Context.BoolTy;
 
   // The type of a parameter passed 'by value'. In the GNU atomics, such
   // arguments are actually passed as pointers.
   QualType ByValType = ValType; // 'CP'
-  if (!IsC11 && !IsN)
+  bool IsPassedByAddress = false;
+  if (!IsC11 && !IsN) {
     ByValType = Ptr->getType();
+    IsPassedByAddress = true;
+  }
 
-  // The first argument --- the pointer --- has a fixed type; we
-  // deduce the types of the rest of the arguments accordingly.  Walk
-  // the remaining arguments, converting them to the deduced value type.
-  for (unsigned i = 1; i != TheCall->getNumArgs(); ++i) {
+  // The first argument's non-CV pointer type is used to deduce the type of
+  // subsequent arguments, except for:
+  //  - weak flag (always converted to bool)
+  //  - memory order (always converted to int)
+  //  - scope  (always converted to int)
+  for (unsigned i = 0; i != TheCall->getNumArgs(); ++i) {
     QualType Ty;
     if (i < NumVals[Form] + 1) {
       switch (i) {
+      case 0:
+        // The first argument is always a pointer. It has a fixed type.
+        // It is always dereferenced, a nullptr is undefined.
+        CheckNonNullArgument(*this, TheCall->getArg(i), DRE->getLocStart());
+        // Nothing else to do: we already know all we want about this pointer.
+        continue;
       case 1:
         // The second argument is the non-atomic operand. For arithmetic, this
         // is always passed by value, and for a compare_exchange it is always
         // passed by address. For the rest, GNU uses by-address and C11 uses
         // by-value.
         assert(Form != Load);
         if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()))
           Ty = ValType;
-        else if (Form == Copy || Form == Xchg)
+        else if (Form == Copy || Form == Xchg) {
+          if (IsPassedByAddress)
+            // The value pointer is always dereferenced, a nullptr is undefined.
+            CheckNonNullArgument(*this, TheCall->getArg(i), DRE->getLocStart());
           Ty = ByValType;
-        else if (Form == Arithmetic)
+        } else if (Form == Arithmetic)
           Ty = Context.getPointerDiffType();
         else {
           Expr *ValArg = TheCall->getArg(i);
-          // Treat this argument as _Nonnull as we want to show a warning if
-          // NULL is passed into it.
+          // The value pointer is always dereferenced, a nullptr is undefined.
           CheckNonNullArgument(*this, ValArg, DRE->getLocStart());
           LangAS AS = LangAS::Default;
           // Keep address space of non-atomic pointer type.
           if (const PointerType *PtrTy =
                   ValArg->getType()->getAs<PointerType>()) {
             AS = PtrTy->getPointeeType().getAddressSpace();
           }
           Ty = Context.getPointerType(
               Context.getAddrSpaceQualType(ValType.getUnqualifiedType(), AS));
         }
         break;
       case 2:
-        // The third argument to compare_exchange / GNU exchange is a
-        // (pointer to a) desired value.
+        // The third argument to compare_exchange / GNU exchange is the desired
+        // value, either by-value (for the *_n variant) or as a pointer.
+        if (!IsN)
+          CheckNonNullArgument(*this, TheCall->getArg(i), DRE->getLocStart());
         Ty = ByValType;
         break;
       case 3:
         // The fourth argument to GNU compare_exchange is a 'weak' flag.
         Ty = Context.BoolTy;
         break;
       }
     } else {
#undef BUILTIN_ROW
   case Builtin::BI__sync_bool_compare_and_swap_4:
   case Builtin::BI__sync_bool_compare_and_swap_8:
   case Builtin::BI__sync_bool_compare_and_swap_16:
     BuiltinIndex = 13;
     NumFixed = 2;
     ResultType = Context.BoolTy;
     break;
       
-  case Builtin::BI__sync_lock_test_and_set: 
+  case Builtin::BI__sync_lock_test_and_set:
   case Builtin::BI__sync_lock_test_and_set_1:
   case Builtin::BI__sync_lock_test_and_set_2:
   case Builtin::BI__sync_lock_test_and_set_4:
   case Builtin::BI__sync_lock_test_and_set_8:
   case Builtin::BI__sync_lock_test_and_set_16:
     BuiltinIndex = 14; 
     break;
       

cfe/trunk/test/Sema/atomic-ops.c

void memory_checks(_Atomic(int) *Ap, int *p, int val) {
   (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_acquire, memory_order_relaxed);
   (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_consume, memory_order_relaxed);
   (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_release, memory_order_relaxed);
   (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_acq_rel, memory_order_relaxed);
   (void)__atomic_compare_exchange_n(p, p, val, 0, memory_order_seq_cst, memory_order_relaxed);
 }
 
 void nullPointerWarning(_Atomic(int) *Ap, int *p, int val) {
-  // The 'expected' pointer shouldn't be NULL.
-  (void)__c11_atomic_compare_exchange_strong(Ap, NULL, val, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)atomic_compare_exchange_weak(Ap, ((void*)0), val); // expected-warning {{null passed to a callee that requires a non-null argument}}
-  (void)__atomic_compare_exchange_n(p, NULL, val, 0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  volatile _Atomic(int) vai;
+  _Atomic(int) ai;
+  volatile int vi = 42;
+  int i = 42;
+
+  __c11_atomic_init((volatile _Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_init((_Atomic(int)*)0, 42); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __c11_atomic_store((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((volatile _Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_load((_Atomic(int)*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_exchange((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((volatile _Atomic(int)*)0, &i, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak((_Atomic(int)*)0, &i, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(&vai, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_weak(&ai, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((volatile _Atomic(int)*)0, &i, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong((_Atomic(int)*)0, &i, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(&vai, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_compare_exchange_strong(&ai, (int*)0, 42, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_add((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_sub((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_and((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_or((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_or((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_xor((volatile _Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__c11_atomic_fetch_xor((_Atomic(int)*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+
+  __atomic_store_n((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_store_n((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_store((volatile int*)0, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_store((int*)0, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_store(&vi, (int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_store(&i, (int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_load_n((volatile int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_load_n((int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_load((volatile int*)0, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_load((int*)0, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_load(&vi, (int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_load(&i, (int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_exchange_n((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_exchange_n((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_exchange((volatile int*)0, &i, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_exchange((int*)0, &i, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_exchange(&vi, (int*)0, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_exchange(&i, (int*)0, &i, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_exchange(&vi, &i, (int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  __atomic_exchange(&i, &i, (int*)0, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange_n((volatile int*)0, &i, 42, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange_n((int*)0, &i, 42, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange_n(&vi, (int*)0, 42, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange_n(&i, (int*)0, 42, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange((volatile int*)0, &i, &i, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange((int*)0, &i, &i, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange(&vi, (int*)0, &i, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange(&i, (int*)0, &i, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange(&vi, &i, (int*)0, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_compare_exchange(&i, &i, (int*)0, /*weak=*/0, memory_order_relaxed, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_add((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_add((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_sub((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_sub((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_and((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_and((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_or((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_or((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_xor((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_xor((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_min((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_min((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_max((volatile int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
+  (void)__atomic_fetch_max((int*)0, 42, memory_order_relaxed); // expected-warning {{null passed to a callee that requires a non-null argument}}
 }