From: Mike Stump Date: Wed, 22 Jul 2009 23:56:57 +0000 (+0000) Subject: Add warning for falling off the end of a function that should return a X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b1682c50d26e0f12130d35b7b9b77d40542c4540;p=clang Add warning for falling off the end of a function that should return a value. This is on by default, and controlled by -Wreturn-type (-Wmost -Wall). I believe there should be very few false positives, though the most interesting case would be: int() { bar(); } when bar does: bar() { while (1) ; } Here, we assume functions return, unless they are marked with the noreturn attribute. I can envision a fixit note for functions that never return normally that don't have a noreturn attribute to add a noreturn attribute. If anyone spots other false positives, let me know! git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@76821 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 76d069fd8a..a8c4de57ca 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -104,6 +104,13 @@ def err_thread_non_global : Error< def err_thread_unsupported : Error< "thread-local storage is unsupported for the current target">; +def warn_maybe_falloff_nonvoid_function : Warning< + "control may reach end of non-void function">, + InGroup; +def warn_falloff_nonvoid_function : Warning< + "control reaches end of non-void function">, + InGroup; + /// Built-in functions. def ext_implicit_lib_function_decl : ExtWarn< "implicitly declaring C library function '%0' with type %1">; diff --git a/lib/Sema/Sema.h b/lib/Sema/Sema.h index 78d64b53c0..3bb763a13f 100644 --- a/lib/Sema/Sema.h +++ b/lib/Sema/Sema.h @@ -801,10 +801,14 @@ public: SourceLocation MemberLoc, IdentifierInfo &Member); - /// Helpers for dealing with function parameters. + /// Helpers for dealing with functions. + void CheckFallThroughForFunctionDef(Decl *D, Stmt *Body); bool CheckParmsForFunctionDef(FunctionDecl *FD); void CheckCXXDefaultArguments(FunctionDecl *FD); void CheckExtraCXXDefaultArguments(Declarator &D); + enum ControlFlowKind { NeverFallThrough = 0, MaybeFallThrough = 1, + AlwaysFallThrough = 2 }; + ControlFlowKind CheckFallThrough(Stmt *); Scope *getNonFieldDeclScope(Scope *S); diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp index d9c1224b43..3d795f60a5 100644 --- a/lib/Sema/SemaDecl.cpp +++ b/lib/Sema/SemaDecl.cpp @@ -16,10 +16,12 @@ #include "clang/AST/APValue.h" #include "clang/AST/ASTConsumer.h" #include "clang/AST/ASTContext.h" +#include "clang/Analysis/CFG.h" #include "clang/AST/DeclObjC.h" #include "clang/AST/DeclTemplate.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/StmtCXX.h" +#include "clang/AST/StmtObjc.h" #include "clang/Parse/DeclSpec.h" #include "clang/Basic/TargetInfo.h" #include "clang/Basic/SourceManager.h" @@ -30,6 +32,7 @@ #include "llvm/ADT/STLExtras.h" #include #include +#include using namespace clang; /// getDeclName - Return a pretty name for the specified decl if possible, or @@ -1011,6 +1014,114 @@ void Sema::MergeVarDecl(VarDecl *New, Decl *OldD) { New->setPreviousDeclaration(Old); } +Sema::ControlFlowKind Sema::CheckFallThrough(Stmt *Root) { + llvm::OwningPtr cfg (CFG::buildCFG(Root, &Context)); + + // FIXME: They should never return 0, fix that, delete this code. + if (cfg == 0) + return NeverFallThrough; + // The CFG leaves in dead things, run a simple mark and sweep on it + // to weed out the trivially dead things. + std::queue workq; + llvm::OwningArrayPtr live(new char[cfg->getNumBlockIDs()]); + // Prep work queue + workq.push(&cfg->getEntry()); + // Solve + while (!workq.empty()) { + CFGBlock *item = workq.front(); + workq.pop(); + live[item->getBlockID()] = 1; + CFGBlock::succ_iterator i; + for (i=item->succ_begin(); i != item->succ_end(); ++i) { + if ((*i) && ! live[(*i)->getBlockID()]) { + live[(*i)->getBlockID()] = 1; + workq.push(*i); + } + } + } + + CFGBlock::succ_iterator i; + bool HasLiveReturn = false; + bool HasFakeEdge = false; + bool HasPlainEdge = false; + for (i=cfg->getExit().pred_begin(); i != cfg->getExit().pred_end(); ++i) { + if (!live[(*i)->getBlockID()]) + continue; + if ((*i)->size() == 0) { + // A labeled empty statement, or the entry block... + HasPlainEdge = true; + continue; + } + Stmt *S = (**i)[(*i)->size()-1]; + if (isa(S)) { + HasLiveReturn = true; + continue; + } + if (isa(S)) { + HasFakeEdge = true; + continue; + } + if (isa(S)) { + HasFakeEdge = true; + continue; + } + bool NoReturnEdge = false; + if (CallExpr *C = dyn_cast(S)) + { + Expr *CEE = C->getCallee()->IgnoreParenCasts(); + if (DeclRefExpr *DRE = dyn_cast(CEE)) { + if (FunctionDecl *FD = dyn_cast(DRE->getDecl())) { + if (FD->hasAttr()) { + NoReturnEdge = true; + HasFakeEdge = true; + } + } + } + } + if (NoReturnEdge == false) + HasPlainEdge = true; + } + if (HasPlainEdge) { + if (HasFakeEdge|HasLiveReturn) + return MaybeFallThrough; + // This says never for calls to functions that are not marked noreturn, that + // don't return. For people that don't like this, we encourage marking the + // functions as noreturn. + return AlwaysFallThrough; + } + return NeverFallThrough; +} + +/// CheckFallThroughForFunctionDef - Check that we don't fall off the end of a +/// function that should return a value. +/// +/// \returns Never iff we can never alter control flow (we always fall off the +/// end of the statement, Conditional iff we might or might not alter +/// control-flow and Always iff we always alter control flow (we never fall off +/// the end of the statement). +void Sema::CheckFallThroughForFunctionDef(Decl *D, Stmt *Body) { + // FIXME: Would be nice if we had a better way to control cascding errors, but + // for now, avoid them. + if (getDiagnostics().hasErrorOccurred()) + return; + if (Diags.getDiagnosticLevel(diag::warn_maybe_falloff_nonvoid_function) + == Diagnostic::Ignored) + return; + // FIXME: Funtion try block + if (CompoundStmt *Compound = dyn_cast(Body)) { + switch (CheckFallThrough(Body)) { + case MaybeFallThrough: + Diag(Compound->getRBracLoc(), diag::warn_maybe_falloff_nonvoid_function); + break; + case AlwaysFallThrough: + Diag(Compound->getRBracLoc(), diag::warn_falloff_nonvoid_function); + break; + case NeverFallThrough: + break; + } + } +} + /// CheckParmsForFunctionDef - Check that the parameters of the given /// function are appropriate for the definition of a function. This /// takes care of any checks that cannot be performed on the @@ -3246,6 +3357,10 @@ Sema::DeclPtrTy Sema::ActOnFinishFunctionBody(DeclPtrTy D, StmtArg BodyArg, Stmt *Body = BodyArg.takeAs(); if (FunctionDecl *FD = dyn_cast_or_null(dcl)) { FD->setBody(Body); + if (!FD->getResultType()->isVoidType() + // C and C++ allow for main to automagically return 0. + && !FD->isMain()) + CheckFallThroughForFunctionDef(FD, Body); if (!FD->isInvalidDecl()) DiagnoseUnusedParameters(FD->param_begin(), FD->param_end()); @@ -3260,6 +3375,8 @@ Sema::DeclPtrTy Sema::ActOnFinishFunctionBody(DeclPtrTy D, StmtArg BodyArg, } else if (ObjCMethodDecl *MD = dyn_cast_or_null(dcl)) { assert(MD == getCurMethodDecl() && "Method parsing confused"); MD->setBody(Body); + if (!MD->getResultType()->isVoidType()) + CheckFallThroughForFunctionDef(MD, Body); MD->setEndLoc(Body->getLocEnd()); if (!MD->isInvalidDecl()) diff --git a/test/Sema/return.c b/test/Sema/return.c index d96cede68a..fd50d0792a 100644 --- a/test/Sema/return.c +++ b/test/Sema/return.c @@ -10,3 +10,176 @@ int t14() { void t15() { return 1; // expected-warning {{void function 't15' should not return a value}} } + +int unknown(); + +void test0() { +} + +int test1() { +} // expected-warning {{control reaches end of non-void function}} + +int test2() { + a: goto a; +} + +int test3() { + goto a; + a: ; +} // expected-warning {{control reaches end of non-void function}} + + +void halt() { + a: goto a; +} + +void halt2() __attribute__((noreturn)); + +int test4() { + halt2(); +} + +int test5() { + halt2(), (void)1; +} + +int test6() { + 1, halt2(); +} + +int j; +int unknown_nohalt() { + return j; +} + +int test7() { + unknown(); +} // expected-warning {{control reaches end of non-void function}} + +int test8() { + (void)(1 + unknown()); +} // expected-warning {{control reaches end of non-void function}} + +int halt3() __attribute__((noreturn)); + +int test9() { + (void)(halt3() + unknown()); +} + +int test10() { + (void)(unknown() || halt3()); +} // expected-warning {{control may reach end of non-void function}} + +int test11() { + (void)(unknown() && halt3()); +} // expected-warning {{control may reach end of non-void function}} + +int test12() { + (void)(halt3() || unknown()); +} + +int test13() { + (void)(halt3() && unknown()); +} + +int test14() { + (void)(1 || unknown()); +} // expected-warning {{control reaches end of non-void function}} + +int test15() { + (void)(0 || unknown()); +} // expected-warning {{control reaches end of non-void function}} + +int test16() { + (void)(0 && unknown()); +} // expected-warning {{control reaches end of non-void function}} + +int test17() { + (void)(1 && unknown()); +} // expected-warning {{control reaches end of non-void function}} + +int test18() { + (void)(unknown_nohalt() && halt3()); +} // expected-warning {{control may reach end of non-void function}} + +int test19() { + (void)(unknown_nohalt() && unknown()); +} // expected-warning {{control reaches end of non-void function}} + +int test20() { + int i; + if (i) + return 0; + else if (0) + return 2; +} // expected-warning {{control may reach end of non-void function}} + +int test21() { + int i; + if (i) + return 0; + else if (1) + return 2; +} + +int test22() { + int i; + switch (i) default: ; +} // expected-warning {{control reaches end of non-void function}} + +int test23() { + int i; + switch (i) { + case 0: + return 0; + case 2: + return 2; + } +} // expected-warning {{control may reach end of non-void function}} + +int test24() { + int i; + switch (i) { + case 0: + return 0; + case 2: + return 2; + default: + return -1; + } +} + +int test25() { + 1 ? halt3() : unknown(); +} + +int test26() { + 0 ? halt3() : unknown(); +} // expected-warning {{control reaches end of non-void function}} + +int j; +int test27() { + switch (j) { + case 1: + do { } while (1); + break; + case 2: + for (;;) ; + break; + case 3: + for (;1;) ; + for (;0;) { + goto done; + } + return 1; + case 4: + while (0) { goto done; } + return 1; + case 5: + while (1) { return 1; } + break; + default: + return 1; + } + done: ; +} diff --git a/test/SemaCXX/return.cpp b/test/SemaCXX/return.cpp new file mode 100644 index 0000000000..56bc71f519 --- /dev/null +++ b/test/SemaCXX/return.cpp @@ -0,0 +1,5 @@ +// RUN: clang-cc %s -fsyntax-only -verify + +int test1() { + throw; +} diff --git a/test/SemaObjC/return.m b/test/SemaObjC/return.m new file mode 100644 index 0000000000..9acf470799 --- /dev/null +++ b/test/SemaObjC/return.m @@ -0,0 +1,6 @@ +// RUN: clang-cc %s -fsyntax-only -verify + +int test1() { + id a; + @throw a; +}