Start breaking -Wunreachable-code up into different diagnostic groups.

Recent work on -Wunreachable-code has focused on suppressing uninteresting
unreachable code that center around "configuration values", but
there are still some set of cases that are sometimes interesting
or uninteresting depending on the codebase.  For example, a dead
"break" statement may not be interesting for a particular codebase,
potentially because it is auto-generated or simply because code
is written defensively.

To address these workflow differences, -Wunreachable-code is now
broken into several diagnostic groups:

-Wunreachable-code: intended to be a reasonable "default" for
most users.

and then other groups that turn on more aggressive checking:

-Wunreachable-code-break: warn about dead break statements

-Wunreachable-code-trivial-return: warn about dead return statements
that return "trivial" values (e.g., return 0).  Other return
statements that return non-trivial values are still reported
under -Wunreachable-code (this is an area subject to more refinement).

-Wunreachable-code-aggressive: supergroup that enables all these
groups.

The goal is to eventually make -Wunreachable-code good enough to
either be in -Wall or on-by-default, thus finessing these warnings
into different groups helps achieve maximum signal for more users.

TODO: the tests need to be updated to reflect this extra control
via diagnostic flags.

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

diff --git a/include/clang/Analysis/Analyses/ReachableCode.h b/include/clang/Analysis/Analyses/ReachableCode.h
index 14967ba2981d39b20fb6f8a83053eef58b0e9e73..08ee20930f161ac771b0ef6f11f531003df1245c 100644 (file)
--- a/include/clang/Analysis/Analyses/ReachableCode.h
+++ b/include/clang/Analysis/Analyses/ReachableCode.h
@@ -37,11 +37,19 @@ namespace clang {
 namespace clang {
 namespace reachable_code {
 
+/// Classifications of unreachable code.
+enum UnreachableKind {
+  UK_TrivialReturn,
+  UK_Break,
+  UK_Other
+};
+
 class Callback {
   virtual void anchor();
 public:
   virtual ~Callback() {}
-  virtual void HandleUnreachable(SourceLocation L, SourceRange R1,
+  virtual void HandleUnreachable(UnreachableKind UK,
+                                 SourceLocation L, SourceRange R1,
                                  SourceRange R2) = 0;
 };
 

diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td
index 97f0b3ed619ceebe028af7804622584e5db0083b..6bb101cfb63f341073f8b6a3270d2f6ec6bdcdef 100644 (file)
--- a/include/clang/Basic/DiagnosticGroups.td
+++ b/include/clang/Basic/DiagnosticGroups.td
@@ -424,6 +424,21 @@ def CharSubscript : DiagGroup<"char-subscripts">;
 def LargeByValueCopy : DiagGroup<"large-by-value-copy">;
 def DuplicateArgDecl : DiagGroup<"duplicate-method-arg">;
 
+// Unreachable code warning groups.
+//
+//  The goal is make -Wunreachable-code on by default, in -Wall, or at
+//  least actively used, with more noisy versions of the warning covered
+//  under separate flags.
+//
+def UnreachableCode : DiagGroup<"unreachable-code">;
+def UnreachableCodeBreak : DiagGroup<"unreachable-code-break",
+                            [UnreachableCode]>;
+def UnreachableCodeTrivialReturn : DiagGroup<"unreachable-code-trivial-return",
+                                    [UnreachableCode]>;
+def UnreachableCodeAggressive : DiagGroup<"unreachable-code-aggressive",
+                                    [UnreachableCodeBreak,
+                                     UnreachableCodeTrivialReturn]>;
+
 // Aggregation warning settings.
 
 // -Widiomatic-parentheses contains warnings about 'idiomatic'

diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 7ac2fc1ec97a3186c33aba73c424f322a0ea1253..6cc177215fbe3ae6f4e27f0c8f3f2b1c38587341 100644 (file)
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -359,8 +359,17 @@ def warn_suggest_noreturn_function : Warning<
 def warn_suggest_noreturn_block : Warning<
   "block could be declared with attribute 'noreturn'">,
   InGroup<MissingNoreturn>, DefaultIgnore;
-def warn_unreachable : Warning<"will never be executed">,
-  InGroup<DiagGroup<"unreachable-code">>, DefaultIgnore;
+
+// Unreachable code.
+def warn_unreachable : Warning<
+  "will never be executed">,
+  InGroup<UnreachableCode>, DefaultIgnore;
+def warn_unreachable_break : Warning<
+  "'break' will never be executed">,
+  InGroup<UnreachableCodeBreak>, DefaultIgnore;
+def warn_unreachable_trivial_return : Warning<
+  "'return' will never be executed">,
+  InGroup<UnreachableCodeTrivialReturn>, DefaultIgnore;
 
 /// Built-in functions.
 def ext_implicit_lib_function_decl : ExtWarn<

diff --git a/lib/Analysis/ReachableCode.cpp b/lib/Analysis/ReachableCode.cpp
index 994f9d2a12fb8f7ef9920691127b7a14e6c7e1bc..09b0efd0e68aee99216e67ee51817d6ca17e3c21 100644 (file)
--- a/lib/Analysis/ReachableCode.cpp
+++ b/lib/Analysis/ReachableCode.cpp
@@ -59,9 +59,14 @@ static bool bodyEndsWithNoReturn(const CFGBlock::AdjacentBlock &AB) {
   return bodyEndsWithNoReturn(Pred);
 }
 
-static bool isBreakPrecededByNoReturn(const CFGBlock *B,
-                                      const Stmt *S) {
-  if (!isa<BreakStmt>(S) || B->pred_empty())
+static bool isBreakPrecededByNoReturn(const CFGBlock *B, const Stmt *S,
+                                      reachable_code::UnreachableKind &UK) {
+  if (!isa<BreakStmt>(S))
+    return false;
+
+  UK = reachable_code::UK_Break;
+
+  if (B->pred_empty())
     return false;
 
   assert(B->empty());
@@ -131,23 +136,17 @@ static bool isTrivialExpression(const Expr *Ex) {
          isEnumConstant(Ex);
 }
 
-static bool isTrivialReturnOrDoWhile(const CFGBlock *B, const Stmt *S) {
-  const Expr *Ex = dyn_cast<Expr>(S);
-
-  if (Ex && !isTrivialExpression(Ex))
-    return false;
-
+static bool isTrivialReturnOrDoWhile(const CFGBlock *B, const Stmt *S,
+                                     reachable_code::UnreachableKind &UK) {
   // Check if the block ends with a do...while() and see if 'S' is the
   // condition.
   if (const Stmt *Term = B->getTerminator()) {
-    if (const DoStmt *DS = dyn_cast<DoStmt>(Term))
-      if (DS->getCond() == S)
-        return true;
+    if (const DoStmt *DS = dyn_cast<DoStmt>(Term)) {
+      const Expr *Cond = DS->getCond();
+      return Cond == S && isTrivialExpression(Cond);
+    }
   }
 
-  if (B->pred_size() != 1)
-    return false;
-
   // Look to see if the block ends with a 'return', and see if 'S'
   // is a substatement.  The 'return' may not be the last element in
   // the block because of destructors.
@@ -155,17 +154,33 @@ static bool isTrivialReturnOrDoWhile(const CFGBlock *B, const Stmt *S) {
        I != E; ++I) {
     if (Optional<CFGStmt> CS = I->getAs<CFGStmt>()) {
       if (const ReturnStmt *RS = dyn_cast<ReturnStmt>(CS->getStmt())) {
+        // Determine if we need to lock at the body of the block
+        // before the dead return.
         bool LookAtBody = false;
-        if (RS == S)
+        if (RS == S) {
           LookAtBody = true;
+          UK = reachable_code::UK_TrivialReturn;
+        }
         else {
           const Expr *RE = RS->getRetValue();
-          if (RE && stripExprSugar(RE->IgnoreParenCasts()) == Ex)
-            LookAtBody = true;
+          if (RE) {
+            RE = stripExprSugar(RE->IgnoreParenCasts());
+            if (RE == S) {
+              UK = reachable_code::UK_TrivialReturn;
+              LookAtBody = isTrivialExpression(RE);
+            }
+          }
         }
 
-        if (LookAtBody)
+        if (LookAtBody) {
+          // More than one predecessor?  Restrict the heuristic
+          // to looking at return statements directly dominated
+          // by a noreturn call.
+          if (B->pred_size() != 1)
+            return false;
+
           return bodyEndsWithNoReturn(*B->pred_begin());
+        }
       }
       break;
     }
@@ -588,19 +603,22 @@ static SourceLocation GetUnreachableLoc(const Stmt *S,
 void DeadCodeScan::reportDeadCode(const CFGBlock *B,
                                   const Stmt *S,
                                   clang::reachable_code::Callback &CB) {
+  // The kind of unreachable code found.
+  reachable_code::UnreachableKind UK = reachable_code::UK_Other;
+
   // Suppress idiomatic cases of calling a noreturn function just
   // before executing a 'break'.  If there is other code after the 'break'
   // in the block then don't suppress the warning.
-  if (isBreakPrecededByNoReturn(B, S))
+  if (isBreakPrecededByNoReturn(B, S, UK))
     return;
 
   // Suppress trivial 'return' statements that are dead.
-  if (isTrivialReturnOrDoWhile(B, S))
+  if (UK == reachable_code::UK_Other && isTrivialReturnOrDoWhile(B, S, UK))
     return;
 
   SourceRange R1, R2;
   SourceLocation Loc = GetUnreachableLoc(S, R1, R2);
-  CB.HandleUnreachable(Loc, R1, R2);
+  CB.HandleUnreachable(UK, Loc, R1, R2);
 }
 
 //===----------------------------------------------------------------------===//

diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp
index dabac1e6c4c19a0873e3d1e535a315a81839dc10..d49dfd58459a2e032613840e516f44b351387a0f 100644 (file)
--- a/lib/Sema/AnalysisBasedWarnings.cpp
+++ b/lib/Sema/AnalysisBasedWarnings.cpp
@@ -65,9 +65,22 @@ namespace {
   public:
     UnreachableCodeHandler(Sema &s) : S(s) {}
 
-    void HandleUnreachable(SourceLocation L, SourceRange R1,
+    void HandleUnreachable(reachable_code::UnreachableKind UK,
+                           SourceLocation L, SourceRange R1,
                            SourceRange R2) override {
-      S.Diag(L, diag::warn_unreachable) << R1 << R2;
+      unsigned diag = diag::warn_unreachable;
+      switch (UK) {
+        case reachable_code::UK_Break:
+          diag = diag::warn_unreachable_break;
+          break;
+        case reachable_code::UK_TrivialReturn:
+          diag = diag::warn_unreachable_trivial_return;
+          break;
+        case reachable_code::UK_Other:
+          break;
+      }
+
+      S.Diag(L, diag) << R1 << R2;
     }
   };
 }

diff --git a/test/Sema/warn-unreachable.c b/test/Sema/warn-unreachable.c
index 2334c3738136506dcaa6d806f9ac019e04efe26b..663fab0f3c36b4914433873f359d5e5b6c813302 100644 (file)
--- a/test/Sema/warn-unreachable.c
+++ b/test/Sema/warn-unreachable.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -fsyntax-only -verify -fblocks -Wunreachable-code -Wno-unused-value -Wno-covered-switch-default -I %S/Inputs
+// RUN: %clang_cc1 %s -fsyntax-only -verify -fblocks -Wunreachable-code-aggressive -Wno-unused-value -Wno-covered-switch-default -I %S/Inputs
 
 #include "warn-unreachable.h"
 

diff --git a/test/SemaCXX/unreachable-code.cpp b/test/SemaCXX/unreachable-code.cpp
index b6b8ab42093ece98d60dfa4ca5ca7d6ba1b30c31..5016c3f24be98acc62fa51a2bd866d23a29a0899 100644 (file)
--- a/test/SemaCXX/unreachable-code.cpp
+++ b/test/SemaCXX/unreachable-code.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -Wunreachable-code -fblocks -verify %s
+// RUN: %clang_cc1 -fcxx-exceptions -fexceptions -fsyntax-only -Wunreachable-code-aggressive -fblocks -verify %s
 
 int j;
 int bar();

diff --git a/test/SemaCXX/warn-unreachable.cpp b/test/SemaCXX/warn-unreachable.cpp
index 49d26daa2f62ad0a2792931da21f4400ccbc3975..843720fa5fd34593f4be7c296a9f6429f2e3c530 100644 (file)
--- a/test/SemaCXX/warn-unreachable.cpp
+++ b/test/SemaCXX/warn-unreachable.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -fcxx-exceptions -fexceptions -fsyntax-only -verify -fblocks -Wunreachable-code -Wno-unused-value
+// RUN: %clang_cc1 %s -fcxx-exceptions -fexceptions -fsyntax-only -verify -fblocks -Wunreachable-code-aggressive -Wno-unused-value
 
 int &halt() __attribute__((noreturn));
 int &live();

diff --git a/test/SemaObjC/warn-unreachable.m b/test/SemaObjC/warn-unreachable.m
index e2ce8a711070b24c912260e9cd75e881b29698f2..979b9053b3d92e0451fef36abf7439b3dde91bbb 100644 (file)
--- a/test/SemaObjC/warn-unreachable.m
+++ b/test/SemaObjC/warn-unreachable.m
@@ -1,4 +1,4 @@
-// RUN: %clang %s -fsyntax-only -Xclang -verify -fblocks -Wunreachable-code -Wno-unused-value -Wno-covered-switch-default
+// RUN: %clang %s -fsyntax-only -Xclang -verify -fblocks -Wunreachable-code-aggressive -Wno-unused-value -Wno-covered-switch-default
 
 // This previously triggered a warning from -Wunreachable-code because of
 // a busted CFG.