[OpenMP] Emit warning message about undefined number of evaluation for atomic update/capture

[cchen]

Summary

The total number of evaluation of X is undefined for atomic construct,
therefore, we emit warning message for them. BTW, Clang is always
evaluate X once.

[ABataev]

I don't think this kind of analysis must be here. clang has a special analysis phase, maybe better to implement it there? Currently, it may produce false-positive warnings. Also, why only check for array accesses? Are they the only constructs that may lead to undefined behavior?

clang/include/clang/Basic/DiagnosticSemaKinds.td

9269

Hmm, the message does not say anything about the real problem.

[cchen]

Don't know there is an analysis phase in Clang, I'll investigate later, thanks.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_omp_linear_expected_int_or_ptr : Error<
   "argument of a linear clause should be of integral or pointer "
   "type, not %0">;
 def warn_omp_linear_step_zero : Warning<
   "zero linear step (%0 %select{|and other variables in clause }1should probably be const)">,
   InGroup<OpenMPClauses>;
 def warn_omp_alignment_not_power_of_two : Warning<
   "aligned clause will be ignored because the requested alignment is not a power of 2">,
   InGroup<OpenMPClauses>;
+def warn_omp_atomic_evaluation_times_undefined : Warning<
+  "the number of times the expression evaluated is unspecified">;
 def err_omp_invalid_target_decl : Error<
   "%0 used in declare target directive is not a variable or a function name">;
 def err_omp_declare_target_multiple : Error<
   "%0 appears multiple times in clauses on the same declare target directive">;
 def err_omp_declare_target_to_and_link : Error<
   "%0 must not appear in both clauses 'to' and 'link'">;
 def warn_omp_not_in_target_context : Warning<
   "declaration is not declared in any declare target region">,

clang/lib/Sema/SemaOpenMP.cpp

public:
 
   /// true if the source expression is a postfix unary operation, false
   /// if it is a prefix unary operation.
   bool isPostfixUpdate() const { return IsPostfixUpdate; }
 
 private:
   bool checkBinaryOperation(BinaryOperator *AtomicBinOp, unsigned DiagId = 0,
                             unsigned NoteId = 0);
+  void traverseBinOp(const Expr *E);
+  void checkArraySubscriptSideEffect(const Expr *X);
 };
 } // namespace
 
 bool OpenMPAtomicUpdateChecker::checkBinaryOperation(
     BinaryOperator *AtomicBinOp, unsigned DiagId, unsigned NoteId) {
   ExprAnalysisErrorCode ErrorFound = NoError;
   SourceLocation ErrorLoc, NoteLoc;
   SourceRange ErrorRange, NoteRange;
if (ErrorFound != NoError && DiagId != 0 && NoteId != 0) {
     SemaRef.Diag(NoteLoc, NoteId) << ErrorFound << NoteRange;
     return true;
   }
   if (SemaRef.CurContext->isDependentContext())
     E = X = UpdateExpr = nullptr;
   return ErrorFound != NoError;
 }
 
+void OpenMPAtomicUpdateChecker::traverseBinOp(const Expr *E) {
+  if (!E)
+    return;
+  if (auto *BinOp = dyn_cast<BinaryOperator>(E)) {
+    traverseBinOp(BinOp->getLHS());
+    traverseBinOp(BinOp->getRHS());
+  } else {
+    if (auto *Call = dyn_cast<CallExpr>(E)) {
+      SemaRef.Diag(E->getExprLoc(),
+                   diag::warn_omp_atomic_evaluation_times_undefined);
+    }
+  }
+}
+
+void OpenMPAtomicUpdateChecker::checkArraySubscriptSideEffect(const Expr *X) {
+  if (!X)
+    return;
+  // If X is an ArraySubscript and the expression has side effect, then we'll
+  // have wrong result since we only evaluate the expression once.
+  if (X->getStmtClass() == Expr::ArraySubscriptExprClass) {
+    auto E = cast<ArraySubscriptExpr>(X);
+    if (E->getLHS() != E->getIdx()) {
+      assert(E->getRHS() == E->getIdx() && "index was neither LHS nor RHS");
+      traverseBinOp(E->getIdx());
+    }
+  }
+}
+
 bool OpenMPAtomicUpdateChecker::checkStatement(Stmt *S, unsigned DiagId,
                                                unsigned NoteId) {
   ExprAnalysisErrorCode ErrorFound = NoError;
   SourceLocation ErrorLoc, NoteLoc;
   SourceRange ErrorRange, NoteRange;
   // Allowed constructs are:
   //  x++;
   //  x--;
if (ErrorFound == NoError && E && X) {
     if (Update.isInvalid())
       return true;
     Update = SemaRef.PerformImplicitConversion(Update.get(), X->getType(),
                                                Sema::AA_Casting);
     if (Update.isInvalid())
       return true;
     UpdateExpr = Update.get();
   }
+  checkArraySubscriptSideEffect(X);
   return ErrorFound != NoError;
 }
 
 StmtResult Sema::ActOnOpenMPAtomicDirective(ArrayRef<OMPClause *> Clauses,
                                             Stmt *AStmt,
                                             SourceLocation StartLoc,
                                             SourceLocation EndLoc) {
   if (!AStmt)

clang/test/OpenMP/atomic_messages.cpp

 // expected-error@+2 {{directive '#pragma omp atomic' cannot contain more than one 'read', 'write', 'update' or 'capture' clause}}
 // expected-note@+1 {{'write' clause used here}}
 #pragma omp atomic write capture
   a = b;
   // expected-note@+1 {{in instantiation of function template specialization 'mixed<int>' requested here}}
   return mixed<int>();
 }
 
+int cnt = 0;
+int add() {
+  return ++cnt;
+}
+
+template <class T>
+T warning() {
+  T arr[100], b = T();
+
+// expected-warning@+2 2 {{the number of times the expression evaluated is unspecified}}
+#pragma omp atomic update
+  arr[add(), add(), 0] = arr[add(), add(), 0] - 10;
+
+// expected-warning@+3 4 {{expression result unused}}
+// expected-warning@+2 2 {{the number of times the expression evaluated is unspecified}}
+#pragma omp atomic capture
+  b = arr[add () + add (), 0] = arr[add () + add (), 0] + 3;
+
+// expected-warning@+3 {{the number of times the expression evaluated is unspecified}}
+#pragma omp atomic capture
+  {
+    arr[foo (), 0] += 6;
+    b = arr[foo (), 0];
+  }
+
+// expected-warning@+3 {{the number of times the expression evaluated is unspecified}}
+#pragma omp atomic capture
+  {
+    arr[foo (), 0] = arr[foo (), 0] + 6;
+    b = arr[foo (), 0];
+  }
+
+  return T();
+}
+
+int warning() {
+  // expected-note@+1 {{in instantiation of function template specialization 'warning<int>' requested here}}
+  return warning<int>();
+}

clang/test/OpenMP/atomic_update_codegen.cpp

 long long llv, llx;
 unsigned long long ullv, ullx;
 float fv, fx;
 double dv, dx;
 long double ldv, ldx;
 _Complex int civ, cix;
 _Complex float cfv, cfx;
 _Complex double cdv, cdx;
+int iarr[100];
+int cnt = 0;
+
+int foo() {
+  return cnt++;
+}
 
 typedef int int4 __attribute__((__vector_size__(16)));
 int4 int4x;
 
 struct BitFields {
   int : 32;
   int a : 31;
 } bfx;