[PATCH] D41795: [analyzer] Inline destructors for non-array deletes.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs, mehdi_amini.

Similarly to how we allow (since https://reviews.llvm.org/D40560) inlining the constructor after `operator new` which isn't `operator new[]`, even if the target region is an `ElementRegion` (which doesn't necessarily represent an array element - it may represent a result of pointer arithmetic or a cast), we should allow inlining the destructors for non-array-but-still-element regions, but not when they are part of `operator delete[]`. There aren't any known issues in this situation. We still aren't attempting to model array new/delete because it requires an unknown amount of constructor calls to be modeled symbolically.

Before the patch, in `new.cpp` tests `testCallToDestructor()` and `test_delete_dtor_Arg()` started failing under `-analyzer-config c++-allocator-inlining=true`, because the new behavior of `operator new` is to return an `ElementRegion` surrounding the void pointer, which disables destructor inlining; the old behavior is to call the destructor over a raw void pointer, which kind of worked. Additionally, some fixmes in `new.cpp` were fixed in the new mode. The change in `testPlacementNew()` also seems correct, even if it wasn't marked as a FIXME.


Repository:
  rC Clang

https://reviews.llvm.org/D41795

Files:
  lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  test/Analysis/new.cpp


Index: test/Analysis/new.cpp
===================================================================
--- test/Analysis/new.cpp
+++ test/Analysis/new.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-store region -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-store region -analyzer-config c++-allocator-inlining=true -DALLOCATOR_INLINING=1 -std=c++11 -verify %s
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_eval(bool);
@@ -34,7 +35,12 @@
 
   void *y = new (x) int;
   clang_analyzer_eval(x == y); // expected-warning{{TRUE}};
-  clang_analyzer_eval(*x == 1); // expected-warning{{UNKNOWN}};
+  clang_analyzer_eval(*x == 1);
+#if ALLOCATOR_INLINING
+  // expected-warning at -2{{TRUE}};
+#else
+  // expected-warning at -4{{UNKNOWN}};
+#endif
 
   return y;
 }
@@ -201,7 +207,10 @@
   new (&n) int;
 
   // Should warn that n is uninitialized.
-  if (n) { // no-warning
+  if (n) {
+#if ALLOCATOR_INLINING
+    // expected-warning at -2{{Branch condition evaluates to a garbage value}}
+#endif
     return 0;
   }
   return 1;
@@ -311,8 +320,7 @@
 void testArrayDestr() {
   NoReturnDtor *p = new NoReturnDtor[2];
   delete[] p; // Calls the base destructor which aborts, checked below
-  //TODO: clang_analyzer_eval should not be called
-  clang_analyzer_eval(true); // expected-warning{{TRUE}}
+  clang_analyzer_eval(true);
 }
 
 // Invalidate Region even in case of default destructor
Index: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -687,8 +687,20 @@
 
     // FIXME: We don't handle constructors or destructors for arrays properly.
     const MemRegion *Target = Dtor.getCXXThisVal().getAsRegion();
-    if (Target && isa<ElementRegion>(Target))
-      return CIP_DisallowedOnce;
+    if (Target && isa<ElementRegion>(Target)) {
+      if (const Stmt *DtorExpr = Dtor.getOriginExpr())
+        if (const Stmt *ParentExpr =
+                CurLC->getParentMap().getParent(DtorExpr))
+          if (const CXXDeleteExpr *DeleteExpr =
+                  dyn_cast<CXXDeleteExpr>(ParentExpr))
+            if (DeleteExpr->isArrayForm())
+              return CIP_DisallowedOnce;
+
+      if (const TypedValueRegion *TR = dyn_cast<TypedValueRegion>(
+              cast<SubRegion>(Target)->getSuperRegion()))
+        if (TR->getValueType()->isArrayType())
+          return CIP_DisallowedOnce;
+    }
 
     break;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D41795.128827.patch
Type: text/x-patch
Size: 2660 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180106/62f20e7a/attachment.bin>