Disallow pointers to const in __sync_fetch_and_xxx

[aaron.ballman]

Summary

The following code is currently accepted without a diagnostic when it should be prohibited:

void f(const int *ptr) {
  __sync_fetch_and_add(ptr, 1);
}

NB: the above code is diagnosed by GCC and ICC. However, Clang attempts to modify the underlying object, which seems dangerous.

[lebedev.ri]

Please always upload all patches with full context (-U99999)

[aaron.ballman]

Updated with more context.

[aaron.ballman]

In D45944#1075055, @lebedev.ri wrote:

Please always upload all patches with full context (-U99999)

I'm not keen on uploading patches with that much context. For instance, it disables syntax highlighting in Phab for many of our files. However, perhaps 100 lines of context was a bit optimistic.

[aaron.ballman]

Ping

[aaron.ballman]

Adding a few more potential reviewers.

[rjmccall]

LGTM, although you might consider testing a broader set of builtins. Maybe at least one from the __atomic_* family?

[aaron.ballman]

Committed in r331598, with an additional test case.

(Accepting officially on behalf of @rjmccall so that I can close the review.)

include/clang/Basic/DiagnosticSemaKinds.td

 
 def warn_call_wrong_number_of_arguments : Warning<
   "too %select{few|many}0 arguments in call to %1">;
 def err_atomic_builtin_must_be_pointer : Error<
   "address argument to atomic builtin must be a pointer (%0 invalid)">;
 def err_atomic_builtin_must_be_pointer_intptr : Error<
   "address argument to atomic builtin must be a pointer to integer or pointer"
   " (%0 invalid)">;
+def err_atomic_builtin_cannot_be_const : Error<
+  "address argument to atomic builtin cannot be const-qualified (%0 invalid)">;
 def err_atomic_builtin_must_be_pointer_intfltptr : Error<
   "address argument to atomic builtin must be a pointer to integer,"
   " floating-point or pointer (%0 invalid)">;
 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"

lib/Sema/SemaChecking.cpp

Sema::SemaBuiltinAtomicOverloaded(ExprResult TheCallResult) {
   QualType ValType = pointerType->getPointeeType();
   if (!ValType->isIntegerType() && !ValType->isAnyPointerType() &&
       !ValType->isBlockPointerType()) {
     Diag(DRE->getLocStart(), diag::err_atomic_builtin_must_be_pointer_intptr)
       << FirstArg->getType() << FirstArg->getSourceRange();
     return ExprError();
   }
 
+  if (ValType.isConstQualified()) {
+    Diag(DRE->getLocStart(), diag::err_atomic_builtin_cannot_be_const)
+        << FirstArg->getType() << FirstArg->getSourceRange();
+    return ExprError();
+  }
+
   switch (ValType.getObjCLifetime()) {
   case Qualifiers::OCL_None:
   case Qualifiers::OCL_ExplicitNone:
     // okay
     break;
 
   case Qualifiers::OCL_Weak:
   case Qualifiers::OCL_Strong:

test/Sema/builtins.c

char * Test20(char *p, const char *in, unsigned n)
     __builtin___memcpy_chk (&buf[5], "abcde", 5, __builtin_object_size (&buf[5], 0));
 
     __builtin___memcpy_chk (&buf[5], "abcde", n, __builtin_object_size (&buf[5], 0));
 
     __builtin___memcpy_chk (&buf[6], "abcde", 5, __builtin_object_size (&buf[6], 0)); // expected-warning {{'__builtin___memcpy_chk' will always overflow destination buffer}}
 
     return buf;
 }
+
+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)}}
+}