[PATCH] [Clang SA] Fix double delete warning reported at wrong location.

[Karthik Bhat]

Hi jordan_rose,

Hi Jordan,
Added a patch to fix warning being generated at incorrect location in case of double delete of a record type which has some code in it's destructor.
The reason for failure is since the destructor is called implicitly when we delte a record type there was no Stmt assosiated with it.
Added a patch which detects that if we are in a destructor which is implicitly called then get the stmt that triggered this implicit call (in this case deleteExpr) and emit the warning at the same location.
Please let me know your comments on this patch.
Thanks.

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

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/NewDelete-checker-test.cpp
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1710,6 +1710,15 @@
       return;
   }
 
+  if(const CXXDestructorCall *DC = dyn_cast<CXXDestructorCall>(&Call)) {
+    if (isa<CXXDeleteExpr>(DC->getTriggerStmt())) {
+      const CXXDeleteExpr *DE =  cast<CXXDeleteExpr>(DC->getTriggerStmt());
+      SymbolRef Sym = C.getSVal(DE->getArgument()).getAsSymbol();
+      if (!Sym || checkUseAfterFree(Sym, C, DE->getArgument()))
+        return;
+    }
+  }
+
   // Check if the callee of a method is deleted.
   if (const CXXInstanceCall *CC = dyn_cast<CXXInstanceCall>(&Call)) {
     SymbolRef Sym = CC->getCXXThisVal().getAsSymbol();
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===================================================================
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -706,6 +706,33 @@
   return 0;
 }
 
+const Stmt *PathDiagnosticLocation::getPrevStmt(const ExplodedNode *N) {
+  for (N = N->getFirstPred(); N; N = N->getFirstPred()) {
+    if (const Stmt *S = getStmt(N)) {
+      // Check if the statement is '?' or '&&'/'||'.  These are "merges",
+      // not actual statement points.
+      switch (S->getStmtClass()) {
+        case Stmt::ChooseExprClass:
+        case Stmt::BinaryConditionalOperatorClass:
+        case Stmt::ConditionalOperatorClass:
+          continue;
+        case Stmt::BinaryOperatorClass: {
+          BinaryOperatorKind Op = cast<BinaryOperator>(S)->getOpcode();
+          if (Op == BO_LAnd || Op == BO_LOr)
+            continue;
+          break;
+        }
+        default:
+          break;
+      }
+      // We found the statement, so return it.
+      return S;
+    }
+  }
+
+  return 0;
+}
+
 const Stmt *PathDiagnosticLocation::getNextStmt(const ExplodedNode *N) {
   for (N = N->getFirstSucc(); N; N = N->getFirstSucc()) {
     if (const Stmt *S = getStmt(N)) {
@@ -739,6 +766,14 @@
   assert(N && "Cannot create a location with a null node.");
   const Stmt *S = getStmt(N);
 
+  // If this is an implicit call find the stmt that triggered it.
+  if (N && !S) {
+    ProgramPoint P = N->getLocation();
+    if (P.getAs<ImplicitCallPoint>()) {
+      S = getPrevStmt(N);
+    }
+  }
+
   if (!S)
     S = getNextStmt(N);
 
Index: test/Analysis/NewDelete-checker-test.cpp
===================================================================
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -338,13 +338,13 @@
 public:
   int *x;
   DerefClass() {}
-  ~DerefClass() {*x = 1;} //expected-warning {{Use of memory after it is freed}}
+  ~DerefClass() {*x = 1;} 
 };
 
 void testDoubleDeleteClassInstance() {
   DerefClass *foo = new DerefClass();
   delete foo;
-  delete foo; // FIXME: We should ideally report warning here instead of inside the destructor.
+  delete foo; //expected-warning {{Use of memory after it is freed}}
 }
 
 class EmptyClass{
@@ -356,5 +356,5 @@
 void testDoubleDeleteEmptyClass() {
   EmptyClass *foo = new EmptyClass();
   delete foo;
-  delete foo;  //expected-warning {{Attempt to free released memory}}
+  delete foo;  //expected-warning {{Use of memory after it is freed}}
 }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===================================================================
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -124,6 +124,7 @@
   void operator=(const CallEvent &) LLVM_DELETED_FUNCTION;
 
 protected:
+  const Stmt *TStmt;
   // This is user data for subclasses.
   const void *Data;
 
@@ -151,7 +152,8 @@
   // DO NOT MAKE PUBLIC
   CallEvent(const CallEvent &Original)
     : State(Original.State), LCtx(Original.LCtx), Origin(Original.Origin),
-      Data(Original.Data), Location(Original.Location), RefCount(0) {}
+       TStmt(Original.TStmt), Data(Original.Data), Location(Original.Location),
+       RefCount(0){}
 
   /// Copies this CallEvent, with vtable intact, into a new block of memory.
   virtual void cloneTo(void *Dest) const = 0;
@@ -651,7 +653,6 @@
 /// of a full-expression (for temporaries), or as part of a delete.
 class CXXDestructorCall : public CXXInstanceCall {
   friend class CallEventManager;
-
 protected:
   typedef llvm::PointerIntPair<const MemRegion *, 1, bool> DtorDataTy;
 
@@ -667,6 +668,7 @@
                     ProgramStateRef St, const LocationContext *LCtx)
     : CXXInstanceCall(DD, St, LCtx) {
     Data = DtorDataTy(Target, IsBaseDestructor).getOpaqueValue();
+    TStmt = Trigger;
     Location = Trigger->getLocEnd();
   }
 
@@ -687,6 +689,10 @@
     return DtorDataTy::getFromOpaqueValue(Data).getInt();
   }
 
+  const Stmt *getTriggerStmt() const {
+    return TStmt;
+  }
+
   virtual Kind getKind() const { return CE_CXXDestructor; }
 
   static bool classof(const CallEvent *CA) {
Index: include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===================================================================
--- include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -296,6 +296,8 @@
 
   /// \brief Retrieve the statement corresponding to the successor node.
   static const Stmt *getNextStmt(const ExplodedNode *N);
+
+  static const Stmt *getPrevStmt(const ExplodedNode *N);
 };
 
 class PathDiagnosticLocationPair {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2441.1.patch
Type: text/x-patch
Size: 5573 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131219/ab3e66e9/attachment.bin>