Page MenuHomePhabricator

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

Authored by EricWF on Jun 11 2015, 5:50 PM.

Details

Reviewers
rsmith
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.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 27558.Jun 11 2015, 5:50 PM
EricWF retitled this revision from to Diagnose const destination to __atomic_load.
EricWF updated this object.
EricWF edited the test plan for this revision. (Show Details)
EricWF added a reviewer: rsmith.
EricWF added a subscriber: Unknown Object (MLST).

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

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.

EricWF updated this revision to Diff 27592.Jun 12 2015, 1:29 PM
EricWF retitled this revision from Diagnose const destination to __atomic_load to Diagnose const atomics and allow more qualified arguments in __atomic builtins..
EricWF updated this object.
rsmith added inline comments.Jun 12 2015, 3:17 PM
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.

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 updated this revision to Diff 27617.Jun 12 2015, 5:10 PM
EricWF updated this object.

Address richards comments.

What about __atomic_test_and_set, and __atomic_clear?

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.

Oh, right. Never mind.

EricWF abandoned this revision.Oct 3 2015, 5:14 PM

Abandoning. I bit off more that I could chew.

I committed the part @rsmith approved separably as r249252.