[cfe-commits] r162689 - in /cfe/trunk: docs/analyzer/IPA.txt lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp test/Analysis/inline.cpp

[Jordan Rose]

Author: jrose
Date: Mon Aug 27 13:39:22 2012
New Revision: 162689

URL: http://llvm.org/viewvc/llvm-project?rev=162689&view=rev
Log:
[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>.

Modified:
    cfe/trunk/docs/analyzer/IPA.txt
    cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
    cfe/trunk/test/Analysis/inline.cpp

Modified: cfe/trunk/docs/analyzer/IPA.txt
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/analyzer/IPA.txt?rev=162689&r1=162688&r2=162689&view=diff
==============================================================================
--- cfe/trunk/docs/analyzer/IPA.txt (original)
+++ cfe/trunk/docs/analyzer/IPA.txt Mon Aug 27 13:39:22 2012
@@ -103,7 +103,8 @@
   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.
 

Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp?rev=162689&r1=162688&r2=162689&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp Mon Aug 27 13:39:22 2012
@@ -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 @@
     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 @@
 
     // 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;

Modified: cfe/trunk/test/Analysis/inline.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inline.cpp?rev=162689&r1=162688&r2=162689&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/inline.cpp (original)
+++ cfe/trunk/test/Analysis/inline.cpp Mon Aug 27 13:39:22 2012
@@ -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 @@
     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}}
+  }
+}