From cfd8ea930a119dc8a1e9a343d2a5cfe142b3d964 Mon Sep 17 00:00:00 2001
From: Ted Kremenek <kremenek@apple.com>
Date: Fri, 26 Mar 2010 22:57:13 +0000
Subject: [PATCH] Fix NoReturnFunctionChecker to properly look at a function's
 type when determining if it returns.  Fixes <rdar://problem/7796563>.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@99663 91177308-0d34-0410-b5e6-96231b3b80d8
---
 lib/Checker/NoReturnFunctionChecker.cpp | 82 ++++++++++++-------------
 test/Analysis/retain-release.m          |  9 ++-
 2 files changed, 48 insertions(+), 43 deletions(-)

diff --git a/lib/Checker/NoReturnFunctionChecker.cpp b/lib/Checker/NoReturnFunctionChecker.cpp
index 1455d87665..80fea99c36 100644
--- a/lib/Checker/NoReturnFunctionChecker.cpp
+++ b/lib/Checker/NoReturnFunctionChecker.cpp
@@ -13,17 +13,17 @@
 //===----------------------------------------------------------------------===//
 
 #include "GRExprEngineInternalChecks.h"
-#include "clang/Checker/PathSensitive/Checker.h"
+#include "clang/Checker/PathSensitive/CheckerVisitor.h"
 #include "llvm/ADT/StringSwitch.h"
 
 using namespace clang;
 
 namespace {
 
-class NoReturnFunctionChecker : public Checker {
+class NoReturnFunctionChecker : public CheckerVisitor<NoReturnFunctionChecker> {
 public:
   static void *getTag() { static int tag = 0; return &tag; }
-  virtual bool EvalCallExpr(CheckerContext &C, const CallExpr *CE);
+  void PostVisitCallExpr(CheckerContext &C, const CallExpr *CE);
 };
 
 }
@@ -32,48 +32,48 @@ void clang::RegisterNoReturnFunctionChecker(GRExprEngine &Eng) {
   Eng.registerCheck(new NoReturnFunctionChecker());
 }
 
-bool NoReturnFunctionChecker::EvalCallExpr(CheckerContext &C, 
-                                           const CallExpr *CE) {
+void NoReturnFunctionChecker::PostVisitCallExpr(CheckerContext &C,
+                                                const CallExpr *CE) {
   const GRState *state = C.getState();
   const Expr *Callee = CE->getCallee();
-  SVal L = state->getSVal(Callee);
-  const FunctionDecl *FD = L.getAsFunctionDecl();
-  if (!FD)
-    return false;
 
-  bool BuildSinks = false;
+  bool BuildSinks = Callee->getType().getNoReturnAttr();
 
-  if (FD->getAttr<NoReturnAttr>() || FD->getAttr<AnalyzerNoReturnAttr>())
-    BuildSinks = true;
-  else if (const IdentifierInfo *II = FD->getIdentifier()) {
-    // HACK: Some functions are not marked noreturn, and don't return.
-    //  Here are a few hardwired ones.  If this takes too long, we can
-    //  potentially cache these results.
-    BuildSinks 
-      = llvm::StringSwitch<bool>(llvm::StringRef(II->getName()))
-          .Case("exit", true)
-          .Case("panic", true)
-          .Case("error", true)
-          .Case("Assert", true)
-          // FIXME: This is just a wrapper around throwing an exception.
-          //  Eventually inter-procedural analysis should handle this easily.
-          .Case("ziperr", true)
-          .Case("assfail", true)
-          .Case("db_error", true)
-          .Case("__assert", true)
-          .Case("__assert_rtn", true)
-          .Case("__assert_fail", true)
-          .Case("dtrace_assfail", true)
-          .Case("yy_fatal_error", true)
-          .Case("_XCAssertionFailureHandler", true)
-          .Case("_DTAssertionFailureHandler", true)
-          .Case("_TSAssertionFailureHandler", true)
-          .Default(false);
+  if (!BuildSinks) {
+    SVal L = state->getSVal(Callee);
+    const FunctionDecl *FD = L.getAsFunctionDecl();
+    if (!FD)
+      return;
+
+    if (FD->getAttr<AnalyzerNoReturnAttr>())
+      BuildSinks = true;
+    else if (const IdentifierInfo *II = FD->getIdentifier()) {
+      // HACK: Some functions are not marked noreturn, and don't return.
+      //  Here are a few hardwired ones.  If this takes too long, we can
+      //  potentially cache these results.
+      BuildSinks
+        = llvm::StringSwitch<bool>(llvm::StringRef(II->getName()))
+            .Case("exit", true)
+            .Case("panic", true)
+            .Case("error", true)
+            .Case("Assert", true)
+            // FIXME: This is just a wrapper around throwing an exception.
+            //  Eventually inter-procedural analysis should handle this easily.
+            .Case("ziperr", true)
+            .Case("assfail", true)
+            .Case("db_error", true)
+            .Case("__assert", true)
+            .Case("__assert_rtn", true)
+            .Case("__assert_fail", true)
+            .Case("dtrace_assfail", true)
+            .Case("yy_fatal_error", true)
+            .Case("_XCAssertionFailureHandler", true)
+            .Case("_DTAssertionFailureHandler", true)
+            .Case("_TSAssertionFailureHandler", true)
+            .Default(false);
+    }
   }
-  
-  if (!BuildSinks)
-    return false;
 
-  C.GenerateSink(CE);
-  return true;
+  if (BuildSinks)
+    C.GenerateSink(CE);
 }
diff --git a/test/Analysis/retain-release.m b/test/Analysis/retain-release.m
index 675e9a6702..3f79c0c7f4 100644
--- a/test/Analysis/retain-release.m
+++ b/test/Analysis/retain-release.m
@@ -1268,6 +1268,7 @@ CFDateRef returnsRetainedCFDate()  {
 //===----------------------------------------------------------------------===//
 
 void panic() __attribute__((noreturn));
+void panic_not_in_hardcoded_list() __attribute__((noreturn));
 
 void test_panic_negative() {
   signed z = 1;
@@ -1291,9 +1292,13 @@ void test_panic_pos_2(int x) {
   signed z = 1;
   CFNumberRef value = CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &z); // no-warning
   if (x)
-    panic();  
-  if (!x)
     panic();
+  if (!x) {
+    // This showed up in <rdar://problem/7796563>, where we silently missed checking
+    // the function type for noreturn.  "panic()" is a hard-coded known panic function
+    // that isn't always noreturn.
+    panic_not_in_hardcoded_list();
+  }
 }
 
 //===----------------------------------------------------------------------===//
-- 
2.40.0