[PATCH] [StaticAnalyzer]Handle Destructor call generated by C++ delete expr

[Karthik Bhat]

  Hi Jordan,
  Thanks for the review. Yes you are right that in case of delete[] the array elements are getting invalidated. Added a test case for the same. Ideally destructor should have been called for each element. I will look into it in next revision.

  Ah..deleting a null pointer is calling the destructor which is wrong.
  Added a patch to fix the same. Roughly what i'm doing is if the memory region to be invalidated is null just return a node with the same state as previous node of the exploded graph. Does this approach look good?

  Implemented test case modifications highlighted by you.
  I would like to use 2 different class in test_delte_dtor_Arg and testCallToDestructor so that the results don't interfere with each other.
  For example if i use the same class(IntPair) in both test cases i would not be able to catch cases were one of them passes and other one fails to call the destructor.

  Thanks again for your time.
  Karthik Bhat

Hi jordan_rose,

http://llvm-reviews.chandlerc.com/D1594

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D1594?vs=4055&id=4090#toc

Files:
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  test/Analysis/new.cpp
  include/clang/Analysis/CFG.h

Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -569,7 +569,14 @@
 void ExprEngine::ProcessDeleteDtor(const CFGDeleteDtor Dtor,
                                    ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst) {
-  //TODO: Handle DeleteDtor
+  const LocationContext *LCtx = Pred->getLocationContext();
+  const CXXDeleteExpr *DE = Dtor.getDeleteExpr();
+  const Stmt *Arg = cast<Stmt>(DE->getArgument());
+  SVal ArgVal = Pred->getState()->getSVal(Arg, LCtx);
+  VisitCXXDestructor(DE->getDestroyedType(),
+                     ArgVal.getAsRegion(),
+                     DE, /*IsBase=*/ false,
+                     Pred, Dst);
 }
 
 void ExprEngine::ProcessBaseDtor(const CFGBaseDtor D,
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===================================================================
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -957,6 +957,8 @@
   const Stmt *Trigger;
   if (Optional<CFGAutomaticObjDtor> AutoDtor = E.getAs<CFGAutomaticObjDtor>())
     Trigger = AutoDtor->getTriggerStmt();
+  else if (Optional<CFGDeleteDtor> DeleteDtor = E.getAs<CFGDeleteDtor>())
+    Trigger = cast<Stmt>(DeleteDtor->getDeleteExpr());
   else
     Trigger = Dtor->getBody();
 
Index: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
===================================================================
--- lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
@@ -300,6 +300,15 @@
   DestVal = makeZeroElementRegion(State, DestVal, ObjectType);
   Dest = DestVal.getAsRegion();
 
+  // If the memory region to be invalidated is null do not call the
+  // destructors. Return a node with state same as previous node.
+  if (!Dest) {
+    StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
+    Bldr.generateNode(S, Pred, State);
+    return;
+  }
+  
+
   const CXXRecordDecl *RecordDecl = ObjectType->getAsCXXRecordDecl();
   assert(RecordDecl && "Only CXXRecordDecls should have destructors");
   const CXXDestructorDecl *DtorDecl = RecordDecl->getDestructor();
Index: test/Analysis/new.cpp
===================================================================
--- test/Analysis/new.cpp
+++ test/Analysis/new.cpp
@@ -206,3 +206,106 @@
   }
   return 1;
 }
+
+// Test modelling destructor call on call to delete
+class IntPair{
+  public:
+    int x;
+    int y;
+    IntPair() {};
+    ~IntPair() {x = x/y;}; //expected-warning {{Division by zero}}
+};
+
+void testCallToDestructor() {
+  IntPair* b = new IntPair();
+  b->x = 1;
+  b->y = 0;
+  delete b; // This results in divide by zero in destructor
+}
+
+// Test Deleting a value that's passed as an argument.
+class DerefClass{
+  public:
+    int* x;
+    DerefClass() {};
+    ~DerefClass() {*x = 1;}; //expected-warning {{Dereference of null pointer (loaded from field 'x')}}
+};
+
+void Fn(DerefClass* arg) {
+  delete arg; 
+}
+
+void test_delte_dtor_Arg() {
+  DerefClass* pair = new DerefClass();
+  pair->x = 0;
+  Fn(pair); 
+}
+
+//Deleting the address of a local variable, null pointer
+void abort(void) __attribute__((noreturn));
+
+class NoReturnDtor {
+ public:
+  NoReturnDtor() {}
+  ~NoReturnDtor() {abort();}
+};
+
+void test_delte_dtor_LocalVar() {
+  NoReturnDtor test;
+  delete &test; // no warn or crash
+}
+
+//Deleting a non class pointer should not crash/warn
+void test_var_delete() {
+  int *v = new int;
+  delete v;  // no crash/warn
+  clang_analyzer_eval(true); // expected-warning{{TRUE}}
+}
+
+void testDeleteNull() {
+  NoReturnDtor *foo = 0;
+  delete foo; // should not call destructor, checked below
+  clang_analyzer_eval(true); // expected-warning{{TRUE}}
+}
+
+void testDeleteUnknown(NoReturnDtor *foo) {
+  delete foo; // should assume non-null and call noreturn destructor
+  clang_analyzer_eval(true); // no-warning
+}
+
+// Invalidate Region even in case of default destructor
+class InvalidateDestTest {
+  public:
+   int x;
+   int* y;
+   ~InvalidateDestTest();
+};
+
+int test_member_invalidation() {
+
+  //test invalidation of member variable
+  InvalidateDestTest* test = new InvalidateDestTest();
+  test->x = 5;
+  int* k = &(test->x);
+  clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
+  delete test;
+  clang_analyzer_eval(*k == 5); // expected-warning{{UNKNOWN}}
+  
+  //test invalidation of member pointer
+  int localVar = 5;
+  test = new InvalidateDestTest();
+  test->y = &localVar;
+  delete test;
+  clang_analyzer_eval(localVar == 5); // expected-warning{{UNKNOWN}}
+
+  // Test aray elements are invalidated.
+  int Var1 = 5;
+  int Var2 = 5;
+  InvalidateDestTest *a = new InvalidateDestTest[2];
+  a[0].y = &Var1;
+  a[1].y = &Var2;
+  delete[] a;
+  clang_analyzer_eval(Var1 == 5); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(Var2 == 5); // expected-warning{{UNKNOWN}}
+  return 0;
+}
Index: include/clang/Analysis/CFG.h
===================================================================
--- include/clang/Analysis/CFG.h
+++ include/clang/Analysis/CFG.h
@@ -200,7 +200,7 @@
   }
 
   // Get Delete expression which triggered the destructor call.
-  const CXXDeleteExpr *getDeleteExpr() {
+  const CXXDeleteExpr *getDeleteExpr() const {
     return static_cast<CXXDeleteExpr *>(Data2.getPointer());
   }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D1594.3.patch
Type: text/x-patch
Size: 5411 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130905/3eccb851/attachment.bin>