From 6fe4dfbc9e5a7018763b1d898876d9b2b8ec3425 Mon Sep 17 00:00:00 2001
From: Jordan Rose <jordan_rose@apple.com>
Date: Mon, 27 Aug 2012 18:39:22 +0000
Subject: [PATCH] [analyzer] Don't inline constructors for objects allocated
 with operator new.

Because the CXXNewExpr appears after the CXXConstructExpr in the CFG, we don't
actually have the correct region to construct into at the time we decide
whether or not to inline. The long-term fix (discussed in PR12014) might be to
introduce a new CFG node (CFGAllocator) that appears before the constructor.

Tracking the short-term fix in <rdar://problem/12180598>.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@162689 91177308-0d34-0410-b5e6-96231b3b80d8
---
 docs/analyzer/IPA.txt                         |  3 +-
 .../Core/ExprEngineCallAndReturn.cpp          | 11 ++++-
 test/Analysis/inline.cpp                      | 42 ++++++++++++++++++-
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/docs/analyzer/IPA.txt b/docs/analyzer/IPA.txt
index 5c722c7a0e..071727f055 100644
--- a/docs/analyzer/IPA.txt
+++ b/docs/analyzer/IPA.txt
@@ -103,7 +103,8 @@ some cases, however, where the analyzer chooses not to inline:
   See "C++ Caveats" below.
 
 - In C++, ExprEngine does not inline custom implementations of operator 'new'
-  or operator 'delete'. See "C++ Caveats" below.
+  or operator 'delete', nor does it inline the constructors and destructors
+  associated with these. See "C++ Caveats" below.
 
 - Calls resulting in "dynamic dispatch" are specially handled.  See more below.
 
diff --git a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
index 4ce434d198..4e4f103ea3 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -18,6 +18,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/ParentMap.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Support/SaveAndRestore.h"
@@ -350,6 +351,15 @@ bool ExprEngine::inlineCall(const CallEvent &Call, const Decl *D,
     if (Target && isa<ElementRegion>(Target))
       return false;
 
+    // FIXME: This is a hack. We don't use the correct region for a new
+    // expression, so if we inline the constructor its result will just be
+    // thrown away. This short-term hack is tracked in <rdar://problem/12180598>
+    // and the longer-term possible fix is discussed in PR12014.
+    const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr();
+    if (const Stmt *Parent = CurLC->getParentMap().getParent(CtorExpr))
+      if (isa<CXXNewExpr>(Parent))
+        return false;
+
     // If the destructor is trivial, it's always safe to inline the constructor.
     if (Ctor.getDecl()->getParent()->hasTrivialDestructor())
       break;
@@ -363,7 +373,6 @@ bool ExprEngine::inlineCall(const CallEvent &Call, const Decl *D,
 
     // FIXME: This is a hack. We don't handle temporary destructors
     // right now, so we shouldn't inline their constructors.
-    const CXXConstructExpr *CtorExpr = Ctor.getOriginExpr();
     if (CtorExpr->getConstructionKind() == CXXConstructExpr::CK_Complete)
       if (!Target || !isa<DeclRegion>(Target))
         return false;
diff --git a/test/Analysis/inline.cpp b/test/Analysis/inline.cpp
index 6590776266..573b1647a3 100644
--- a/test/Analysis/inline.cpp
+++ b/test/Analysis/inline.cpp
@@ -1,8 +1,18 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-ipa=inlining -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-ipa=inlining -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_checkInlined(bool);
 
+typedef __typeof__(sizeof(int)) size_t;
+extern "C" void *malloc(size_t);
+
+// This is the standard placement new.
+inline void* operator new(size_t, void* __p) throw()
+{
+  return __p;
+}
+
+
 class A {
 public:
   int getZero() { return 0; }
@@ -227,3 +237,33 @@ namespace DefaultArgs {
     clang_analyzer_eval(obj.get() == 42); // expected-warning{{UNKNOWN}}
   }
 }
+
+namespace OperatorNew {
+  class IntWrapper {
+  public:
+    int value;
+
+    IntWrapper(int input) : value(input) {
+      // We don't want this constructor to be inlined unless we can actually
+      // use the proper region for operator new.
+      // See PR12014 and <rdar://problem/12180598>.
+      clang_analyzer_checkInlined(false); // no-warning
+    }
+  };
+
+  void test() {
+    IntWrapper *obj = new IntWrapper(42);
+    // should be TRUE
+    clang_analyzer_eval(obj->value == 42); // expected-warning{{UNKNOWN}}
+  }
+
+  void testPlacement() {
+    IntWrapper *obj = static_cast<IntWrapper *>(malloc(sizeof(IntWrapper)));
+    IntWrapper *alias = new (obj) IntWrapper(42);
+
+    clang_analyzer_eval(alias == obj); // expected-warning{{TRUE}}
+
+    // should be TRUE
+    clang_analyzer_eval(obj->value == 42); // expected-warning{{UNKNOWN}}
+  }
+}