From 2ce2d86e049bedbed920d53b4eeb539c89cf73a9 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Sat, 15 Mar 2014 05:47:06 +0000 Subject: [PATCH] Further refine -Wunreachable-code groups so that -Wno-unreachable-code-break doesn't turn off all unreachable code warnings. Also relax unreachable 'break' and 'return' to not check for being preceded by a call to 'noreturn'. That turns out to not be so interesting in practice. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@204000 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Basic/DiagnosticGroups.td | 11 ++-- include/clang/Basic/DiagnosticSemaKinds.td | 4 +- lib/Analysis/ReachableCode.cpp | 61 ++++++++++------------ lib/Sema/AnalysisBasedWarnings.cpp | 29 ++++++---- test/Sema/warn-unreachable.c | 58 ++++++++++++++++---- test/SemaCXX/warn-unreachable.cpp | 4 +- 6 files changed, 104 insertions(+), 63 deletions(-) diff --git a/include/clang/Basic/DiagnosticGroups.td b/include/clang/Basic/DiagnosticGroups.td index 6bb101cfb63f..c7ea65fac7af 100644 --- a/include/clang/Basic/DiagnosticGroups.td +++ b/include/clang/Basic/DiagnosticGroups.td @@ -431,13 +431,12 @@ def DuplicateArgDecl : DiagGroup<"duplicate-method-arg">; // under separate flags. // def UnreachableCode : DiagGroup<"unreachable-code">; -def UnreachableCodeBreak : DiagGroup<"unreachable-code-break", - [UnreachableCode]>; -def UnreachableCodeTrivialReturn : DiagGroup<"unreachable-code-trivial-return", - [UnreachableCode]>; +def UnreachableCodeBreak : DiagGroup<"unreachable-code-break">; +def UnreachableCodeReturn : DiagGroup<"unreachable-code-return">; def UnreachableCodeAggressive : DiagGroup<"unreachable-code-aggressive", - [UnreachableCodeBreak, - UnreachableCodeTrivialReturn]>; + [UnreachableCode, + UnreachableCodeBreak, + UnreachableCodeReturn]>; // Aggregation warning settings. diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 6cc177215fbe..a4792d41e8da 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -367,9 +367,9 @@ def warn_unreachable : Warning< def warn_unreachable_break : Warning< "'break' will never be executed">, InGroup, DefaultIgnore; -def warn_unreachable_trivial_return : Warning< +def warn_unreachable_return : Warning< "'return' will never be executed">, - InGroup, DefaultIgnore; + InGroup, DefaultIgnore; /// Built-in functions. def ext_implicit_lib_function_decl : ExtWarn< diff --git a/lib/Analysis/ReachableCode.cpp b/lib/Analysis/ReachableCode.cpp index 09b0efd0e68a..d297d03775dd 100644 --- a/lib/Analysis/ReachableCode.cpp +++ b/lib/Analysis/ReachableCode.cpp @@ -136,8 +136,7 @@ static bool isTrivialExpression(const Expr *Ex) { isEnumConstant(Ex); } -static bool isTrivialReturnOrDoWhile(const CFGBlock *B, const Stmt *S, - reachable_code::UnreachableKind &UK) { +static bool isTrivialDoWhile(const CFGBlock *B, const Stmt *S) { // Check if the block ends with a do...while() and see if 'S' is the // condition. if (const Stmt *Term = B->getTerminator()) { @@ -146,7 +145,10 @@ static bool isTrivialReturnOrDoWhile(const CFGBlock *B, const Stmt *S, return Cond == S && isTrivialExpression(Cond); } } + return false; +} +static bool isTrivialReturn(const CFGBlock *B, const Stmt *S) { // 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. @@ -156,30 +158,11 @@ static bool isTrivialReturnOrDoWhile(const CFGBlock *B, const Stmt *S, if (const ReturnStmt *RS = dyn_cast(CS->getStmt())) { // Determine if we need to lock at the body of the block // before the dead return. - bool LookAtBody = false; - if (RS == S) { - LookAtBody = true; - UK = reachable_code::UK_TrivialReturn; - } - else { - const Expr *RE = RS->getRetValue(); - if (RE) { - RE = stripExprSugar(RE->IgnoreParenCasts()); - if (RE == S) { - UK = reachable_code::UK_TrivialReturn; - LookAtBody = isTrivialExpression(RE); - } - } - } - - 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()); + if (RS == S) + return true; + if (const Expr *RE = RS->getRetValue()) { + RE = stripExprSugar(RE->IgnoreParenCasts()); + return RE == S && isTrivialExpression(RE); } } break; @@ -606,15 +589,25 @@ void DeadCodeScan::reportDeadCode(const CFGBlock *B, // 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, UK)) - return; + do { + // 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 (isa(S)) { + UK = reachable_code::UK_Break; + break; + } - // Suppress trivial 'return' statements that are dead. - if (UK == reachable_code::UK_Other && isTrivialReturnOrDoWhile(B, S, UK)) - return; + if (isTrivialDoWhile(B, S)) + return; + + // Suppress trivial 'return' statements that are dead. + if (isTrivialReturn(B, S)) { + UK = reachable_code::UK_TrivialReturn; + break; + } + + } while(false); SourceRange R1, R2; SourceLocation Loc = GetUnreachableLoc(S, R1, R2); diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index d49dfd58459a..32e40ab2ea9c 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -74,7 +74,7 @@ namespace { diag = diag::warn_unreachable_break; break; case reachable_code::UK_TrivialReturn: - diag = diag::warn_unreachable_trivial_return; + diag = diag::warn_unreachable_return; break; case reachable_code::UK_Other: break; @@ -1665,6 +1665,11 @@ clang::sema::AnalysisBasedWarnings::Policy::Policy() { enableConsumedAnalysis = 0; } +static unsigned isEnabled(DiagnosticsEngine &D, unsigned diag) { + return (unsigned) D.getDiagnosticLevel(diag, SourceLocation()) != + DiagnosticsEngine::Ignored; +} + clang::sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s) : S(s), NumFunctionsAnalyzed(0), @@ -1676,16 +1681,20 @@ clang::sema::AnalysisBasedWarnings::AnalysisBasedWarnings(Sema &s) MaxUninitAnalysisVariablesPerFunction(0), NumUninitAnalysisBlockVisits(0), MaxUninitAnalysisBlockVisitsPerFunction(0) { + + using namespace diag; DiagnosticsEngine &D = S.getDiagnostics(); - DefaultPolicy.enableCheckUnreachable = (unsigned) - (D.getDiagnosticLevel(diag::warn_unreachable, SourceLocation()) != - DiagnosticsEngine::Ignored); - DefaultPolicy.enableThreadSafetyAnalysis = (unsigned) - (D.getDiagnosticLevel(diag::warn_double_lock, SourceLocation()) != - DiagnosticsEngine::Ignored); - DefaultPolicy.enableConsumedAnalysis = (unsigned) - (D.getDiagnosticLevel(diag::warn_use_in_invalid_state, SourceLocation()) != - DiagnosticsEngine::Ignored); + + DefaultPolicy.enableCheckUnreachable = + isEnabled(D, warn_unreachable) || + isEnabled(D, warn_unreachable_break) || + isEnabled(D, warn_unreachable_return); + + DefaultPolicy.enableThreadSafetyAnalysis = + isEnabled(D, warn_double_lock); + + DefaultPolicy.enableConsumedAnalysis = + isEnabled(D, warn_use_in_invalid_state); } static void flushDiagnostics(Sema &S, sema::FunctionScopeInfo *fscope) { diff --git a/test/Sema/warn-unreachable.c b/test/Sema/warn-unreachable.c index 663fab0f3c36..9cd87a7343b0 100644 --- a/test/Sema/warn-unreachable.c +++ b/test/Sema/warn-unreachable.c @@ -151,10 +151,10 @@ int test_break_preceded_by_noreturn(int i) { switch (i) { case 1: raze(); - break; // no-warning + break; // expected-warning {{'break' will never be executed}} case 2: raze(); - break; // no-warning + break; // expected-warning {{'break' will never be executed}} warn_here(); // expected-warning {{will never be executed}} case 3: return 1; @@ -194,19 +194,17 @@ void unreachable_in_default(MyEnum e) { // Don't warn about trivial dead returns. int trivial_dead_return() { raze(); - // Use the '()' to test that we unwrap such stuff - // when looking for dead code. - return ((0)); // no-warning + return ((0)); // expected-warning {{'return' will never be executed}} } void trivial_dead_return_void() { raze(); - return; // no-warning + return; // expected-warning {{'return' will never be executed}} } MyEnum trival_dead_return_enum() { raze(); - return Value1; // no-warning + return Value1; // expected-warning {{'return' will never be executed}} } MyEnum trivial_dead_return_enum_2(int x) { @@ -222,12 +220,12 @@ MyEnum trivial_dead_return_enum_2(int x) { const char *trivial_dead_return_cstr() { raze(); - return ""; // no-warning + return ""; // expected-warning {{return' will never be executed}} } char trivial_dead_return_char() { raze(); - return ' '; // no-warning + return ' '; // expected-warning {{return' will never be executed}} } MyEnum nontrivial_dead_return_enum_2(int x) { @@ -325,3 +323,45 @@ int test_do_while_nontrivial_cond(int x) { return x; } +// Diagnostic control: -Wunreachable-code-return. + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wunreachable-code-return" + +void trivial_dead_return_void_SUPPRESSED() { + raze(); + return; // no-warning +} + +MyEnum trival_dead_return_enum_SUPPRESSED() { + raze(); + return Value1; // no-warning +} + +#pragma clang diagnostic pop + +// Diagnostic control: -Wunreachable-code-break. + +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wunreachable-code-break" + +int test_break_preceded_by_noreturn_SUPPRESSED(int i) { + switch (i) { + case 1: + raze(); + break; // no-warning + case 2: + raze(); + break; // no-warning + warn_here(); // expected-warning {{will never be executed}} + case 3: + return 1; + break; // no-warning + default: + break; + break; // no-warning + } + return i; +} + +#pragma clang diagnostic pop diff --git a/test/SemaCXX/warn-unreachable.cpp b/test/SemaCXX/warn-unreachable.cpp index 843720fa5fd3..17f0b9bf528b 100644 --- a/test/SemaCXX/warn-unreachable.cpp +++ b/test/SemaCXX/warn-unreachable.cpp @@ -142,7 +142,7 @@ typedef basic_string string; std::string testStr() { raze(); - return ""; // no-warning + return ""; // expected-warning {{'return' will never be executed}} } std::string testStrWarn(const char *s) { @@ -152,7 +152,7 @@ std::string testStrWarn(const char *s) { bool testBool() { raze(); - return true; // no-warning + return true; // expected-warning {{'return' will never be executed}} } static const bool ConditionVar = 1;