Use DiagRuntimeBehavior for -Wunsequenced to weed out false positives

where either the modification or the other access is unreachable.

This reverts r359984 (which reverted r359962). The bug in clang-tidy's
test suite exposed by the original commit was fixed in r360009.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@360010 91177308-0d34-0410-b5e6-96231b3b80d8

7 files changed, 43 insertions, 23 deletions

diff --git a/include/clang/Sema/ScopeInfo.h b/include/clang/Sema/ScopeInfo.h
index b4bcef7fa6..2aa1caf699 100644
--- a/include/clang/Sema/ScopeInfo.h
+++ b/include/clang/Sema/ScopeInfo.h
@@ -84,11 +84,11 @@ class PossiblyUnreachableDiag {
 public:
   PartialDiagnostic PD;
   SourceLocation Loc;
-  const Stmt *stmt;
+  llvm::TinyPtrVector<const Stmt*> Stmts;
 
   PossiblyUnreachableDiag(const PartialDiagnostic &PD, SourceLocation Loc,
-                          const Stmt *stmt)
-      : PD(PD), Loc(Loc), stmt(stmt) {}
+                          ArrayRef<const Stmt *> Stmts)
+      : PD(PD), Loc(Loc), Stmts(Stmts) {}
 };
 
 /// Retains information about a function, method, or block that is
diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h
index 138f985ffc..a98670cf09 100644
--- a/include/clang/Sema/Sema.h
+++ b/include/clang/Sema/Sema.h
@@ -4252,6 +4252,10 @@ public:
   /// If it is unreachable, the diagnostic will not be emitted.
   bool DiagRuntimeBehavior(SourceLocation Loc, const Stmt *Statement,
                            const PartialDiagnostic &PD);
+  /// Similar, but diagnostic is only produced if all the specified statements
+  /// are reachable.
+  bool DiagRuntimeBehavior(SourceLocation Loc, ArrayRef<const Stmt*> Stmts,
+                           const PartialDiagnostic &PD);
 
   // Primary Expressions.
   SourceRange getExprRange(Expr *E) const;
diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp
index 7afe44658e..6c95b60003 100644
--- a/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2089,16 +2089,16 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
 
     // Register the expressions with the CFGBuilder.
     for (const auto &D : fscope->PossiblyUnreachableDiags) {
-      if (D.stmt)
-        AC.registerForcedBlockExpression(D.stmt);
+      for (const Stmt *S : D.Stmts)
+        AC.registerForcedBlockExpression(S);
     }
 
     if (AC.getCFG()) {
       analyzed = true;
       for (const auto &D : fscope->PossiblyUnreachableDiags) {
-        bool processed = false;
-        if (D.stmt) {
-          const CFGBlock *block = AC.getBlockForRegisteredExpression(D.stmt);
+        bool AllReachable = true;
+        for (const Stmt *S : D.Stmts) {
+          const CFGBlock *block = AC.getBlockForRegisteredExpression(S);
           CFGReverseBlockReachabilityAnalysis *cra =
               AC.getCFGReachablityAnalysis();
           // FIXME: We should be able to assert that block is non-null, but
@@ -2106,15 +2106,17 @@ AnalysisBasedWarnings::IssueWarnings(sema::AnalysisBasedWarnings::Policy P,
           // edge cases; see test/Sema/vla-2.c.
           if (block && cra) {
             // Can this block be reached from the entrance?
-            if (cra->isReachable(&AC.getCFG()->getEntry(), block))
-              S.Diag(D.Loc, D.PD);
-            processed = true;
+            if (!cra->isReachable(&AC.getCFG()->getEntry(), block)) {
+              AllReachable = false;
+              break;
+            }
           }
+          // If we cannot map to a basic block, assume the statement is
+          // reachable.
         }
-        if (!processed) {
-          // Emit the warning anyway if we cannot map to a basic block.
+
+        if (AllReachable)
           S.Diag(D.Loc, D.PD);
-        }
       }
     }
 
diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 4c2d1dc768..d0479b832f 100644
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -12155,10 +12155,11 @@ class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
     if (OtherKind == UK_Use)
       std::swap(Mod, ModOrUse);
 
-    SemaRef.Diag(Mod->getExprLoc(),
-                 IsModMod ? diag::warn_unsequenced_mod_mod
-                          : diag::warn_unsequenced_mod_use)
-      << O << SourceRange(ModOrUse->getExprLoc());
+    SemaRef.DiagRuntimeBehavior(
+        Mod->getExprLoc(), {Mod, ModOrUse},
+        SemaRef.PDiag(IsModMod ? diag::warn_unsequenced_mod_mod
+                               : diag::warn_unsequenced_mod_use)
+            << O << SourceRange(ModOrUse->getExprLoc()));
     UI.Diagnosed = true;
   }
 
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 2d9dbdd342..a06e7dc51e 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -16079,7 +16079,7 @@ void Sema::MarkDeclarationsReferencedInExpr(Expr *E,
 /// behavior of a program, such as passing a non-POD value through an ellipsis.
 /// Failure to do so will likely result in spurious diagnostics or failures
 /// during overload resolution or within sizeof/alignof/typeof/typeid.
-bool Sema::DiagRuntimeBehavior(SourceLocation Loc, const Stmt *Statement,
+bool Sema::DiagRuntimeBehavior(SourceLocation Loc, ArrayRef<const Stmt*> Stmts,
                                const PartialDiagnostic &PD) {
   switch (ExprEvalContexts.back().Context) {
   case ExpressionEvaluationContext::Unevaluated:
@@ -16095,9 +16095,9 @@ bool Sema::DiagRuntimeBehavior(SourceLocation Loc, const Stmt *Statement,
 
   case ExpressionEvaluationContext::PotentiallyEvaluated:
   case ExpressionEvaluationContext::PotentiallyEvaluatedIfUsed:
-    if (Statement && getCurFunctionOrMethodDecl()) {
+    if (!Stmts.empty() && getCurFunctionOrMethodDecl()) {
       FunctionScopes.back()->PossiblyUnreachableDiags.
-        push_back(sema::PossiblyUnreachableDiag(PD, Loc, Statement));
+        push_back(sema::PossiblyUnreachableDiag(PD, Loc, Stmts));
       return true;
     }
 
@@ -16122,6 +16122,12 @@ bool Sema::DiagRuntimeBehavior(SourceLocation Loc, const Stmt *Statement,
   return false;
 }
 
+bool Sema::DiagRuntimeBehavior(SourceLocation Loc, const Stmt *Statement,
+                               const PartialDiagnostic &PD) {
+  return DiagRuntimeBehavior(
+      Loc, Statement ? llvm::makeArrayRef(Statement) : llvm::None, PD);
+}
+
 bool Sema::CheckCallReturnType(QualType ReturnType, SourceLocation Loc,
                                CallExpr *CE, FunctionDecl *FD) {
   if (ReturnType->isVoidType() || !ReturnType->isIncompleteType())
diff --git a/test/Sema/warn-unsequenced.c b/test/Sema/warn-unsequenced.c
index 247a121941..9654cda924 100644
--- a/test/Sema/warn-unsequenced.c
+++ b/test/Sema/warn-unsequenced.c
@@ -95,4 +95,11 @@ void test() {
   _Alignof(++a) + ++a; // expected-warning {{extension}}
 
   __builtin_constant_p(f(++a, 0)) ? f(f(++a, 0), f(++a, 0)) : 0;
+
+  if (0) ++a + ++a; // ok, unreachable
+}
+
+void g(const char *p, int n) {
+  // This resembles code produced by some macros in glibc's <string.h>.
+  __builtin_constant_p(p) && __builtin_constant_p(++n) && (++n + ++n);
 }
diff --git a/test/SemaCXX/warn-unsequenced.cpp b/test/SemaCXX/warn-unsequenced.cpp
index 1dd2f6d74d..bb8fd8be3d 100644
--- a/test/SemaCXX/warn-unsequenced.cpp
+++ b/test/SemaCXX/warn-unsequenced.cpp
@@ -486,8 +486,8 @@ int Foo<X>::Run() {
   // cxx17-warning@-2 {{unsequenced modification and access to 'num'}}
 
   foo(num++, num++);
-  // cxx11-warning@-1 2{{multiple unsequenced modifications to 'num'}}
-  // cxx17-warning@-2 2{{multiple unsequenced modifications to 'num'}}
+  // cxx11-warning@-1 {{multiple unsequenced modifications to 'num'}}
+  // cxx17-warning@-2 {{multiple unsequenced modifications to 'num'}}
   return 1;
 }