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

6 files changed, 313 insertions, 1 deletions

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<ReturnType>;
+def warn_falloff_nonvoid_function : Warning<
+  "control reaches end of non-void function">,
+  InGroup<ReturnType>;
+
 /// 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 <algorithm>
 #include <functional>
+#include <queue>
 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 (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<CFGBlock*> workq;
+  llvm::OwningArrayPtr<char> 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<ReturnStmt>(S)) {
+      HasLiveReturn = true;
+      continue;
+    }
+    if (isa<ObjCAtThrowStmt>(S)) {
+      HasFakeEdge = true;
+      continue;
+    }
+    if (isa<CXXThrowExpr>(S)) {
+      HasFakeEdge = true;
+      continue;
+    }
+    bool NoReturnEdge = false;
+    if (CallExpr *C = dyn_cast<CallExpr>(S))
+      {
+        Expr *CEE = C->getCallee()->IgnoreParenCasts();
+        if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(CEE)) {
+          if (FunctionDecl *FD = dyn_cast<FunctionDecl>(DRE->getDecl())) {
+            if (FD->hasAttr<NoReturnAttr>()) {
+              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<CompoundStmt>(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<Stmt>();
   if (FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(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<ObjCMethodDecl>(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;
+}