This is an archive of the discontinued LLVM Phabricator instance.

Implement -Watomic-implicit-seq-cst
ClosedPublic

Authored by jfb on Aug 21 2018, 10:02 PM.

Details

Summary

_Atomic and __sync_* operations are implicitly sequentially-consistent. Some
codebases want to force explicit usage of memory order instead. This warning
allows them to know where implicit sequentially-consistent memory order is used.
The warning isn't on by default because _Atomic was purposefully designed to
have seq_cst as the default: the idea was that it's the right thing to use most
of the time. This warning allows developers who disagree to enforce explicit
usage instead.

A follow-up patch will take care of C++'s std::atomic. It'll be different enough
from this patch that I think it should be separate: for C++ the atomic
operations all have a memory order parameter (or two), but it's defaulted. I
believe this warning should trigger when the default is used, but not when
seq_cst is used explicitly (or implicitly as the failure order for cmpxchg).

rdar://problem/28172966

Diff Detail

Event Timeline

jfb created this revision.Aug 21 2018, 10:02 PM
kubamracek added a subscriber: dcoughlin.
rjmccall added inline comments.Aug 23 2018, 10:09 AM
lib/Sema/SemaChecking.cpp
4921

Why is the diagnostic at the end location? And why are you separately checking whether it's ignored at the begin location?

9997

isIgnored is not actually a cheap operation; you should *definitely* be checking for atomic types first. And usually we just fire off the diagnostic without checking isIgnored because the setup cost of a diagnostic is not assumed to be that high.

jfb updated this revision to Diff 162484.Aug 24 2018, 2:40 PM
jfb marked 2 inline comments as done.
  • Address John's comments: diagnose at beginning, and don't check isIgnored manually.
rjmccall added inline comments.Aug 24 2018, 5:39 PM
lib/Sema/SemaChecking.cpp
10672

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?

jfb added inline comments.Aug 28 2018, 5:02 PM
lib/Sema/SemaChecking.cpp
10672

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.

rjmccall added inline comments.Aug 28 2018, 5:05 PM
lib/Sema/SemaChecking.cpp
10672

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.

jfb added inline comments.Aug 28 2018, 5:12 PM
lib/Sema/SemaChecking.cpp
10672

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).

It says the type of the assignment expression, not the type of the LHS.

C11 [6.5.16]p2: "The type of an assignment expression is the type the left operand would have after lvalue conversion."

C11 [6.3.2.1]p2: "...this is called lvalue conversion. If the lvalue has qualified type, the value has the unqualified version of the type of the lvalue; additionally, if the lvalue has atomic type, the value has the non-atomic version of the type of the lvalue; otherwise, the value has the type of the lvalue."

The RHS is not converted to have atomic type.

jfb added a comment.Aug 29 2018, 9:36 AM

It says the type of the assignment expression, not the type of the LHS.

C11 [6.5.16]p2: "The type of an assignment expression is the type the left operand would have after lvalue conversion."

C11 [6.3.2.1]p2: "...this is called lvalue conversion. If the lvalue has qualified type, the value has the unqualified version of the type of the lvalue; additionally, if the lvalue has atomic type, the value has the non-atomic version of the type of the lvalue; otherwise, the value has the type of the lvalue."

The RHS is not converted to have atomic type.

Right that would be nonsensical because you'd need to load it atomically. The C _Atomic semantics match what C++ std::atomic does:

_Atomic(int) atom;
void ass(int i) {
    atom = i;
}

is the same as:

#include <atomic>
std::atomic<int> atom;
void ass(int i) {
    atom = i;
}

They're meant to be interchangeable.

This warning is meant to warn on C's implicit seq_cst. When I get to writing the C++ version of the warning I'll warn about C++'s implicit seq_cst.

jfb updated this revision to Diff 163577.Aug 31 2018, 12:38 PM
  • Don't diagnose initialization, only assignment.
rjmccall added inline comments.Aug 31 2018, 11:15 PM
lib/Sema/SemaChecking.cpp
4921

I guess all the other places in this function are diagnosing at the end location, so we should probably just be consistent. Sorry for the run-around.

10407

Why are the operator-driven diagnostics here all pointing at the LHS instead of at the operator?

10671

When pointing at an arbitrary expression, it's generally consider best to point at its caret location — the result of getLoc() — and then highlight its source range.

10974

Can you explain this one?

jfb updated this revision to Diff 164235.Sep 6 2018, 10:09 AM
jfb marked 4 inline comments as done.
  • Address comments.
lib/Sema/SemaChecking.cpp
10974

It would produce duplicate warnings of !, &&, ||, and condition for ? :. Bool-like conversion is a special case of implicit conversion, which we already check elsewhere.

rjmccall accepted this revision.Sep 10 2018, 9:02 AM

LGTM.

lib/Sema/SemaChecking.cpp
10974

I see, makes sense.

This revision is now accepted and ready to land.Sep 10 2018, 9:02 AM
This revision was automatically updated to reflect the committed changes.