[analyzer] Always derive a CallEvent's return type from its origin expr.

Previously, we preferred to get a result type by looking at the callee's
declared result type. This allowed us to handlereferences, which are
represented in the AST as lvalues of their pointee type. (That is, a call
to a function returning 'int &' has type 'int' and value kind 'lvalue'.)

However, this results in us preferring the original type of a function
over a casted type. This is a problem when a function  pointer is casted
to another type, because the conjured result value will have the wrong
type. AdjustedReturnValueChecker is supposed to handle this, but still
doesn't handle the case where there is no "original function" at all,
i.e. where the callee is unknown.

Now, we instead look at the call expression's value kind (lvalue, xvalue,
or prvalue), and adjust the expr's type accordingly. This will have no
effect when the function is inlined, and will conjure the value that will
actually be used when it is not.

This makes AdjustedReturnValueChecker /nearly/ unnecessary; unfortunately,
the cases where it would still be useful are where we need to cast the
result of an inlined function or a checker-evaluated function, and in these
cases we don't know what we're casting /from/ by the time we can do post-
call checks. In light of that, remove AdjustedReturnValueChecker, which
was already not checking quite a few calls.

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

6 files changed, 31 insertions, 133 deletions

diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 510293a104..d0d1585545 100644
--- a/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -168,8 +168,6 @@ protected:
   /// result of this call.
   virtual void getExtraInvalidatedRegions(RegionList &Regions) const {}
 
-  virtual QualType getDeclaredResultType() const = 0;
-
 public:
   virtual ~CallEvent() {}
 
@@ -357,8 +355,6 @@ protected:
     : CallEvent(D, St, LCtx) {}
   AnyFunctionCall(const AnyFunctionCall &Other) : CallEvent(Other) {}
 
-  virtual QualType getDeclaredResultType() const;
-
 public:
   // This function is overridden by subclasses, but they must return
   // a FunctionDecl.
@@ -453,8 +449,6 @@ protected:
 
   virtual void getExtraInvalidatedRegions(RegionList &Regions) const;
 
-  virtual QualType getDeclaredResultType() const;
-
 public:
   /// \brief Returns the region associated with this instance of the block.
   ///
@@ -772,8 +766,6 @@ protected:
 
   virtual void getExtraInvalidatedRegions(RegionList &Regions) const;
 
-  virtual QualType getDeclaredResultType() const;
-
   /// Check if the selector may have multiple definitions (may have overrides).
   virtual bool canBeOverridenInSubclass(ObjCInterfaceDecl *IDecl,
                                         Selector Sel) const;
diff --git a/lib/StaticAnalyzer/Checkers/AdjustedReturnValueChecker.cpp b/lib/StaticAnalyzer/Checkers/AdjustedReturnValueChecker.cpp
deleted file mode 100644
index 84ea8c709d..0000000000
--- a/lib/StaticAnalyzer/Checkers/AdjustedReturnValueChecker.cpp
+++ /dev/null
@@ -1,92 +0,0 @@
-//== AdjustedReturnValueChecker.cpp -----------------------------*- C++ -*--==//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-//
-// This file defines AdjustedReturnValueChecker, a simple check to see if the
-// return value of a function call is different than the one the caller thinks
-// it is.
-//
-//===----------------------------------------------------------------------===//
-
-#include "ClangSACheckers.h"
-#include "clang/StaticAnalyzer/Core/Checker.h"
-#include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
-
-using namespace clang;
-using namespace ento;
-
-namespace {
-class AdjustedReturnValueChecker : 
-    public Checker< check::PostStmt<CallExpr> > {
-public:
-  void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
-};
-}
-
-void AdjustedReturnValueChecker::checkPostStmt(const CallExpr *CE,
-                                               CheckerContext &C) const {
-  
-  // Get the result type of the call.
-  QualType expectedResultTy = CE->getType();
-
-  // Fetch the signature of the called function.
-  ProgramStateRef state = C.getState();
-  const LocationContext *LCtx = C.getLocationContext();
-
-  SVal V = state->getSVal(CE, LCtx);
-  
-  if (V.isUnknown())
-    return;
-  
-  // Casting to void?  Discard the value.
-  if (expectedResultTy->isVoidType()) {
-    C.addTransition(state->BindExpr(CE, LCtx, UnknownVal()));
-    return;
-  }                   
-
-  const MemRegion *callee = state->getSVal(CE->getCallee(), LCtx).getAsRegion();
-  if (!callee)
-    return;
-
-  QualType actualResultTy;
-  
-  if (const FunctionTextRegion *FT = dyn_cast<FunctionTextRegion>(callee)) {
-    const FunctionDecl *FD = FT->getDecl();
-    actualResultTy = FD->getResultType();
-  }
-  else if (const BlockDataRegion *BD = dyn_cast<BlockDataRegion>(callee)) {
-    const BlockTextRegion *BR = BD->getCodeRegion();
-    const BlockPointerType *BT=BR->getLocationType()->getAs<BlockPointerType>();
-    const FunctionType *FT = BT->getPointeeType()->getAs<FunctionType>();
-    actualResultTy = FT->getResultType();
-  }
-
-  // Can this happen?
-  if (actualResultTy.isNull())
-    return;
-
-  // For now, ignore references.
-  if (actualResultTy->getAs<ReferenceType>())
-    return;
-  
-
-  // Are they the same?
-  if (expectedResultTy != actualResultTy) {
-    // FIXME: Do more checking and actual emit an error. At least performing
-    // the cast avoids some assertion failures elsewhere.
-    SValBuilder &svalBuilder = C.getSValBuilder();
-    V = svalBuilder.evalCast(V, expectedResultTy, actualResultTy);
-    C.addTransition(state->BindExpr(CE, LCtx, V));
-  }
-}
-
-void ento::registerAdjustedReturnValueChecker(CheckerManager &mgr) {
-  mgr.registerChecker<AdjustedReturnValueChecker>();
-}
diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt
index 7fe51d3a4c..ba39ccc456 100644
--- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -4,7 +4,6 @@ clang_tablegen(Checkers.inc -gen-clang-sa-checkers
   TARGET ClangSACheckers)
 
 add_clang_library(clangStaticAnalyzerCheckers
-  AdjustedReturnValueChecker.cpp
   AnalyzerStatsChecker.cpp
   ArrayBoundChecker.cpp
   ArrayBoundCheckerV2.cpp
diff --git a/lib/StaticAnalyzer/Checkers/Checkers.td b/lib/StaticAnalyzer/Checkers/Checkers.td
index 7949637ec6..92b5a16d70 100644
--- a/lib/StaticAnalyzer/Checkers/Checkers.td
+++ b/lib/StaticAnalyzer/Checkers/Checkers.td
@@ -60,10 +60,6 @@ def CallAndMessageChecker : Checker<"CallAndMessage">,
   HelpText<"Check for logical errors for function calls and Objective-C message expressions (e.g., uninitialized arguments, null function pointers)">,
   DescFile<"CallAndMessageChecker.cpp">;
 
-def AdjustedReturnValueChecker : Checker<"AdjustedReturnValue">,
-  HelpText<"Check to see if the return value of a function call is different than the caller expects (e.g., from calls through function pointers)">,
-  DescFile<"AdjustedReturnValueChecker.cpp">;
-
 def AttrNonNullChecker : Checker<"AttributeNonNull">,
   HelpText<"Check for null pointers passed as arguments to a function whose arguments are marked with the 'nonnull' attribute">,
   DescFile<"AttrNonNullChecker.cpp">;
diff --git a/lib/StaticAnalyzer/Core/CallEvent.cpp b/lib/StaticAnalyzer/Core/CallEvent.cpp
index 32a66c3b27..aed0f55982 100644
--- a/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -23,10 +23,26 @@ using namespace clang;
 using namespace ento;
 
 QualType CallEvent::getResultType() const {
-  QualType ResultTy = getDeclaredResultType();
+  const Expr *E = getOriginExpr();
+  assert(E && "Calls without origin expressions do not have results");
+  QualType ResultTy = E->getType();
 
-  if (ResultTy.isNull())
-    ResultTy = getOriginExpr()->getType();
+  ASTContext &Ctx = getState()->getStateManager().getContext();
+
+  // A function that returns a reference to 'int' will have a result type
+  // of simply 'int'. Check the origin expr's value kind to recover the
+  // proper type.
+  switch (E->getValueKind()) {
+  case VK_LValue:
+    ResultTy = Ctx.getLValueReferenceType(ResultTy);
+    break;
+  case VK_XValue:
+    ResultTy = Ctx.getRValueReferenceType(ResultTy);
+    break;
+  case VK_RValue:
+    // No adjustment is necessary.
+    break;
+  }
 
   return ResultTy;
 }
@@ -285,14 +301,6 @@ void AnyFunctionCall::getInitialStackFrameContents(
                                D->param_begin(), D->param_end());
 }
 
-QualType AnyFunctionCall::getDeclaredResultType() const {
-  const FunctionDecl *D = getDecl();
-  if (!D)
-    return QualType();
-
-  return D->getResultType();
-}
-
 bool AnyFunctionCall::argumentsMayEscape() const {
   if (hasNonZeroCallbackArg())
     return true;
@@ -501,15 +509,6 @@ void BlockCall::getInitialStackFrameContents(const StackFrameContext *CalleeCtx,
 }
 
 
-QualType BlockCall::getDeclaredResultType() const {
-  const BlockDataRegion *BR = getBlockRegion();
-  if (!BR)
-    return QualType();
-  QualType BlockTy = BR->getCodeRegion()->getLocationType();
-  return cast<FunctionType>(BlockTy->getPointeeType())->getResultType();
-}
-
-
 SVal CXXConstructorCall::getCXXThisVal() const {
   if (Data)
     return loc::MemRegionVal(static_cast<const MemRegion *>(Data));
@@ -566,14 +565,6 @@ ObjCMethodCall::getExtraInvalidatedRegions(RegionList &Regions) const {
     Regions.push_back(R);
 }
 
-QualType ObjCMethodCall::getDeclaredResultType() const {
-  const ObjCMethodDecl *D = getDecl();
-  if (!D)
-    return QualType();
-
-  return D->getResultType();
-}
-
 SVal ObjCMethodCall::getSelfSVal() const {
   const LocationContext *LCtx = getLocationContext();
   const ImplicitParamDecl *SelfDecl = LCtx->getSelfDecl();
diff --git a/test/Analysis/reference.cpp b/test/Analysis/reference.cpp
index e46016b762..4a2cbb8e25 100644
--- a/test/Analysis/reference.cpp
+++ b/test/Analysis/reference.cpp
@@ -121,6 +121,18 @@ void testReferenceAddress(int &x) {
 }
 
 
+void testFunctionPointerReturn(void *opaque) {
+  typedef int &(*RefFn)();
+
+  RefFn getRef = (RefFn)opaque;
+
+  // Don't crash writing to or reading from this reference.
+  int &x = getRef();
+  x = 42;
+  clang_analyzer_eval(x == 42); // expected-warning{{TRUE}}
+}
+
+
 // ------------------------------------
 // False negatives
 // ------------------------------------