[PATCH] D51084: Implement -Watomic-implicit-seq-cst

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:10668
+  if (Source->isAtomicType() || Target->isAtomicType())
+    S.Diag(E->getBeginLoc(), diag::warn_atomic_implicit_seq_cst);
+
----------------
rjmccall wrote:
> jfb wrote:
> > rjmccall wrote:
> > > Why would the target be an atomic type?  And if it is an atomic type, isn't that an initialization of a temporary?  In what situation does it make sense to order the initialization of a temporary?
> > In this case:
> > 
> > ```
> > void bad_assign_1(int i) {
> >   atom = i; // expected-warning {{implicit use of sequentially-consistent atomic may incur stronger memory barriers than necessary}}
> > }
> > ```
> > 
> > We want to warn on assignment to atomics being implicitly `seq_cst`.
> > 
> > Though you're right, initialization shouldn't warn because it isn't atomic. The issue is that `ATOMIC_VAR_INIT` is what needs to get used, and that's weird to test. I'll add a test that just assigns (which is what `ATOMIC_VAR_INIT` expands to for clang), and I'll need to update the code to detect that pattern and avoid warning in that case. I guess I have to look at the `Expr` and figure out if the LHS is a `Decl` or something like that.
> Do we really implicitly convert the RHS of that assignment to atomic type?  That seems wrong; `_Atomic` is really a type qualifier, and the RHS should not be converted to `_Atomic(T)` any more than it would be converted to `volatile T` for an assignment into a `volatile` l-value.
I don't make the rules man! I just enforce (and try to warn) on them!

C17:

> 6.5.16.1 Simple assignment
> **Constraints**
> One of the following shall hold: 114)
> — the left operand has atomic, qualified, or unqualified arithmetic type, and the right has arithmetic type;
> — the left operand has an atomic, qualified, or unqualified version of a structure or union type compatible with the type of the right;
> — the left operand has atomic, qualified, or unqualified pointer type, and (considering the type the left operand would have after lvalue conversion) both operands are pointers to qualified or unqualified versions of compatible types, and the type pointed to by the left has all the qualifiers of the type pointed to by the right;
> — the left operand has atomic, qualified, or unqualified pointer type, and (considering the type the left operand would have after lvalue conversion) one operand is a pointer to an object type, and the other is a pointer to a qualified or unqualified version of void, and the type pointed to by the left has all the qualifiers of the type pointed to by the right;
> — the left operand is an atomic, qualified, or unqualified pointer, and the right is a null pointer constant; or
> — the left operand has type atomic, qualified, or unqualified _Bool, and the right is a pointer.
>
> **Semantics**
> In simple assignment (=), the value of the right operand is converted to the type of the assignment expression and replaces the value stored in the object designated by the left operand.
>
> 114)The asymmetric appearance of these constraints with respect to type qualifiers is due to the conversion (specified in 6.3.2.1) that changes lvalues to "the value of the expression" and thus removes any type qualifiers that were applied to the typecategoryoftheexpression(forexample,itremovesconstbutnotvolatilefromthetypeint volatile * const).


Repository:
  rC Clang

https://reviews.llvm.org/D51084