Re: r229593 - [analyzer] Refactoring: clarified the way the proper check kind is chosen.

[Anton Yartsev]

On 05.03.2015 21:39, Anna Zaks wrote:

On Feb 17, 2015, at 4:39 PM, Anton Yartsev <anton.yart...@gmail.com 
<mailto:anton.yart...@gmail.com>> wrote:

Author: ayartsev
Date: Tue Feb 17 18:39:06 2015
New Revision: 229593

URL: http://llvm.org/viewvc/llvm-project?rev=229593&view=rev
Log:
[analyzer] Refactoring: clarified the way the proper check kind is 
chosen.


Anton, this doesn’t look like a simple refactoring. Also, the new API 
looks more confusing and difficult to use.

autoCheckKind = getCheckIfTracked(C, DeallocExpr);
vs
autoCheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
                          CK_MallocPessimistic,
                          CK_NewDeleteChecker),
            C, DeallocExpr);

Instead of checking if any of our checkers handle a specific family 
and returning the one that does, we now have to pass in the list of 
checkers we are interested in. Can you explain why this is needed?

I think this is a step in the wrong direction. My understanding is 
that some of the methods only work for specific checkers (regardless 
of the family being processed). Therefore, they returned early in case 
they were called on checkers, where they are useless. Looks like you 
are trying to fold that check into the API family check, which is 
unrelated. Though, I might be missing something..
Hi Anna!)
The old getCheckIfTracked() has two drawbacks: first, it does not 
considered CK_MismatchedDeallocatorChecker and CK_NewDeleteLeaksChecker 
checkers.
The second is that there is, in fact, unable to customize the set of 
checkers getCheckIfTracked() chooses from. For each family there are 
several checkers responsible for it. Without providing the set of 
checkers of interest getCheckIfTracked() is ugly in use. Consider 
changes in MallocChecker::reportLeak() below - the removed block of code 
(marked start and end of the code with "---------" for you). This piece 
was just added for situations (hard to guess looking at the code), when, 
for example, CK_MallocPessimistic and CK_NewDelete are 'on' and 
CK_NewDeleteLeaksChecker is 'off' and in this case getCheckIfTracked() 
returns CK_NewDelete checker as the checker, responsible for the 
AF_CXXNew/AF_CXXNewArray families. The code looks confusing in 
consideration of the fact that we rejected all the checkers responsible 
for AF_CXXNew/AF_CXXNewArray families, except CK_NewDeleteLeaksChecker, 
by writing 'if (... && !ChecksEnabled[CK_NewDeleteLeaksChecker]) 
return;' at the beginning of the method. In the current implementation 
getCheckIfTracked() returns only the checkers it was restricted for.

The second bonus of the current implementation is that it gets us rid of 
the check for specific checkers at the beginning.


Modified:
   cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=229593&r1=229592&r2=229593&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Tue Feb 
17 18:39:06 2015
@@ -184,6 +184,7 @@ public:

  DefaultBool ChecksEnabled[CK_NumCheckKinds];
  CheckName CheckNames[CK_NumCheckKinds];
+  typedef llvm::SmallVector<CheckKind, CK_NumCheckKinds> CKVecTy;

  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
  void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
@@ -327,12 +328,16 @@ private:

  ///@{
  /// Tells if a given family/call/symbol is tracked by the current 
checker.
-  /// Sets CheckKind to the kind of the checker responsible for this
-  /// family/call/symbol.
-  Optional<CheckKind> getCheckIfTracked(AllocationFamily Family) const;
-  Optional<CheckKind> getCheckIfTracked(CheckerContext &C,
+  /// Looks through incoming CheckKind(s) and returns the kind of 
the checker
+  /// responsible for this family/call/symbol.

Is it possible for more than one checker to be responsible for the 
same family?
Yes, it is possible, e.g. NewDelete, NewDeleteLeaks and 
MismatchedDeallocator are responsible for AF_CXXNew/AF_CXXNewArray families.

This returns the first checker that handles the family from the given 
list.
Yes, that is how getCheckIfTracked() was designed before, but the order 
of the checkers was hardcoded:
    if (ChecksEnabled[CK_MallocOptimistic]) {
      return CK_MallocOptimistic;
    } else if (ChecksEnabled[CK_MallocPessimistic]) {
      return CK_MallocPessimistic;
    }

Now it is possible to customize the order in which the checkers are 
checked and returned.


+  Optional<CheckKind> getCheckIfTracked(CheckKind CK,
+                                        AllocationFamily Family) const;

This always returns either the input checker or an empty one. Looks 
like it should just return a bool...
I left this to be consistent with other overloads, and also the name of 
the method implies that the checker is returned. Do you think the return 
value should be changed to bool? And, if yes, do you think the method 
should be renamed?


+  Optional<CheckKind> getCheckIfTracked(CKVecTy CKVec,

Hard to tell what this argument is from documentation/name.
I'll address this!


+                                        AllocationFamily Family) const;
+  Optional<CheckKind> getCheckIfTracked(CKVecTy CKVec, 
CheckerContext &C,
                                        const Stmt *AllocDeallocStmt) 
const;
-  Optional<CheckKind> getCheckIfTracked(CheckerContext &C, SymbolRef 
Sym) const;
+  Optional<CheckKind> getCheckIfTracked(CKVecTy CKVec, 
CheckerContext &C,
+                                        SymbolRef Sym) const;
  ///@}
  static bool SummarizeValue(raw_ostream &os, SVal V);
  static bool SummarizeRegion(raw_ostream &os, const MemRegion *MR);
@@ -1310,21 +1315,32 @@ ProgramStateRef MallocChecker::FreeMemAu
}

Optional<MallocChecker::CheckKind>
-MallocChecker::getCheckIfTracked(AllocationFamily Family) const {
+MallocChecker::getCheckIfTracked(MallocChecker::CheckKind CK,
+                                 AllocationFamily Family) const {
+
+  if (CK == CK_NumCheckKinds || !ChecksEnabled[CK])
+    return Optional<MallocChecker::CheckKind>();
+
+  // C/C++ checkers.
+  if (CK == CK_MismatchedDeallocatorChecker)
+    return CK;
+
  switch (Family) {
  case AF_Malloc:
  case AF_IfNameIndex: {
-    if (ChecksEnabled[CK_MallocOptimistic]) {
-      return CK_MallocOptimistic;
-    } else if (ChecksEnabled[CK_MallocPessimistic]) {
-      return CK_MallocPessimistic;
+    // C checkers.
+    if (CK == CK_MallocOptimistic ||
+        CK == CK_MallocPessimistic) {
+      return CK;
    }
    return Optional<MallocChecker::CheckKind>();
  }
  case AF_CXXNew:
  case AF_CXXNewArray: {
-    if (ChecksEnabled[CK_NewDeleteChecker]) {
-      return CK_NewDeleteChecker;
+    // C++ checkers.
+    if (CK == CK_NewDeleteChecker ||
+        CK == CK_NewDeleteLeaksChecker) {
+      return CK;
    }
    return Optional<MallocChecker::CheckKind>();
  }
@@ -1335,18 +1351,45 @@ MallocChecker::getCheckIfTracked(Allocat
  llvm_unreachable("unhandled family");
}

+static MallocChecker::CKVecTy MakeVecFromCK(MallocChecker::CheckKind 
CK1,
+               MallocChecker::CheckKind CK2 = 
MallocChecker::CK_NumCheckKinds,
+               MallocChecker::CheckKind CK3 = 
MallocChecker::CK_NumCheckKinds,
+               MallocChecker::CheckKind CK4 = 
MallocChecker::CK_NumCheckKinds) {
+  MallocChecker::CKVecTy CKVec;
+  CKVec.push_back(CK1);
+  if (CK2 != MallocChecker::CK_NumCheckKinds) {
+    CKVec.push_back(CK2);
+    if (CK3 != MallocChecker::CK_NumCheckKinds) {
+      CKVec.push_back(CK3);
+      if (CK4 != MallocChecker::CK_NumCheckKinds)
+        CKVec.push_back(CK4);
+    }
+  }
+  return CKVec;
+}
+
Optional<MallocChecker::CheckKind>
-MallocChecker::getCheckIfTracked(CheckerContext &C,
-                                 const Stmt *AllocDeallocStmt) const {
-  return getCheckIfTracked(getAllocationFamily(C, AllocDeallocStmt));
+MallocChecker::getCheckIfTracked(CKVecTy CKVec, AllocationFamily 
Family) const {
+  for (auto CK: CKVec) {
+    auto RetCK = getCheckIfTracked(CK, Family);
+    if (RetCK.hasValue())
+      return RetCK;
+  }
+  return Optional<MallocChecker::CheckKind>();
}

Optional<MallocChecker::CheckKind>
-MallocChecker::getCheckIfTracked(CheckerContext &C, SymbolRef Sym) 
const {
+MallocChecker::getCheckIfTracked(CKVecTy CKVec, CheckerContext &C,
+                                 const Stmt *AllocDeallocStmt) const {
+  return getCheckIfTracked(CKVec, getAllocationFamily(C, 
AllocDeallocStmt));
+}

+Optional<MallocChecker::CheckKind>
+MallocChecker::getCheckIfTracked(CKVecTy CKVec, CheckerContext &C,
+                                 SymbolRef Sym) const {
  const RefState *RS = C.getState()->get<RegionState>(Sym);
  assert(RS);
-  return getCheckIfTracked(RS->getAllocationFamily());
+  return getCheckIfTracked(CKVec, RS->getAllocationFamily());
}

bool MallocChecker::SummarizeValue(raw_ostream &os, SVal V) {
@@ -1440,13 +1483,10 @@ void MallocChecker::ReportBadFree(Checke
                                  SourceRange Range,
                                  const Expr *DeallocExpr) const {

-  if (!ChecksEnabled[CK_MallocOptimistic] &&
-      !ChecksEnabled[CK_MallocPessimistic] &&
-      !ChecksEnabled[CK_NewDeleteChecker])
-    return;
-
-  Optional<MallocChecker::CheckKind> CheckKind =
-      getCheckIfTracked(C, DeallocExpr);
+  auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
+                                                   CK_MallocPessimistic,
+                                                   CK_NewDeleteChecker),
+                                     C, DeallocExpr);
  if (!CheckKind.hasValue())
    return;

@@ -1546,13 +1586,11 @@ void MallocChecker::ReportOffsetFree(Che
                                     SourceRange Range, const Expr 
*DeallocExpr,
                                     const Expr *AllocExpr) const {

-  if (!ChecksEnabled[CK_MallocOptimistic] &&
-      !ChecksEnabled[CK_MallocPessimistic] &&
-      !ChecksEnabled[CK_NewDeleteChecker])
-    return;

-  Optional<MallocChecker::CheckKind> CheckKind =
-      getCheckIfTracked(C, AllocExpr);
+  auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
+                                                   CK_MallocPessimistic,
+                                                   CK_NewDeleteChecker),
+                                     C, AllocExpr);
  if (!CheckKind.hasValue())
    return;

@@ -1602,12 +1640,10 @@ void MallocChecker::ReportOffsetFree(Che
void MallocChecker::ReportUseAfterFree(CheckerContext &C, SourceRange 
Range,
                                       SymbolRef Sym) const {

-  if (!ChecksEnabled[CK_MallocOptimistic] &&
-      !ChecksEnabled[CK_MallocPessimistic] &&
-      !ChecksEnabled[CK_NewDeleteChecker])
-    return;
-
-  Optional<MallocChecker::CheckKind> CheckKind = 
getCheckIfTracked(C, Sym);
+  auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
+                                                   CK_MallocPessimistic,
+                                                   CK_NewDeleteChecker),
+                                     C, Sym);
  if (!CheckKind.hasValue())
    return;

@@ -1630,12 +1666,10 @@ void MallocChecker::ReportDoubleFree(Che
                                     bool Released, SymbolRef Sym,
                                     SymbolRef PrevSym) const {

-  if (!ChecksEnabled[CK_MallocOptimistic] &&
-      !ChecksEnabled[CK_MallocPessimistic] &&
-      !ChecksEnabled[CK_NewDeleteChecker])
-    return;
-
-  Optional<MallocChecker::CheckKind> CheckKind = 
getCheckIfTracked(C, Sym);
+  auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
+                                                   CK_MallocPessimistic,
+                                                   CK_NewDeleteChecker),
+                                     C, Sym);
  if (!CheckKind.hasValue())
    return;

@@ -1660,13 +1694,10 @@ void MallocChecker::ReportDoubleFree(Che

void MallocChecker::ReportDoubleDelete(CheckerContext &C, SymbolRef 
Sym) const {

-  if (!ChecksEnabled[CK_NewDeleteChecker])
-    return;
-
-  Optional<MallocChecker::CheckKind> CheckKind = 
getCheckIfTracked(C, Sym);
+  auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_NewDeleteChecker),
+                                     C, Sym);
  if (!CheckKind.hasValue())
    return;
-  assert(*CheckKind == CK_NewDeleteChecker && "invalid check kind");

  if (ExplodedNode *N = C.generateSink()) {
    if (!BT_DoubleDelete)
@@ -1851,24 +1882,13 @@ MallocChecker::getAllocationSite(const E
void MallocChecker::reportLeak(SymbolRef Sym, ExplodedNode *N,
                               CheckerContext &C) const {

-  if (!ChecksEnabled[CK_MallocOptimistic] &&
-      !ChecksEnabled[CK_MallocPessimistic] &&
-      !ChecksEnabled[CK_NewDeleteLeaksChecker])
-    return;
-
-  const RefState *RS = C.getState()->get<RegionState>(Sym);
-  assert(RS && "cannot leak an untracked symbol");
-  AllocationFamily Family = RS->getAllocationFamily();
-  Optional<MallocChecker::CheckKind> CheckKind = 
getCheckIfTracked(Family);
+  auto CheckKind = getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
+                                                   CK_MallocPessimistic,
+ 
                                                  CK_NewDeleteLeaksChecker),
+                                     C, Sym);
  if (!CheckKind.hasValue())
    return;

-----------------------------------
-  // Special case for new and new[]; these are controlled by a 
separate checker
-  // flag so that they can be selectively disabled.
-  if (Family == AF_CXXNew || Family == AF_CXXNewArray)
-    if (!ChecksEnabled[CK_NewDeleteLeaksChecker])
-      return;
-
-----------------------------------
  assert(N);
  if (!BT_Leak[*CheckKind]) {
    BT_Leak[*CheckKind].reset(
@@ -2479,8 +2499,10 @@ void MallocChecker::printState(raw_ostre
    for (RegionStateTy::iterator I = RS.begin(), E = RS.end(); I != 
E; ++I) {
      const RefState *RefS = State->get<RegionState>(I.getKey());
      AllocationFamily Family = RefS->getAllocationFamily();
-      Optional<MallocChecker::CheckKind> CheckKind = 
getCheckIfTracked(Family);
-
+      auto CheckKind = 
getCheckIfTracked(MakeVecFromCK(CK_MallocOptimistic,
+ 
                                                      CK_MallocPessimistic,
+ 
                                                      CK_NewDeleteChecker),
+                                         Family);

This is a generic printing routine, which is used for debugging. Why 
is this restricted to the specific checkers?
This particular branch handles leak detecting checkers which are 
CK_MallocOptimistic, CK_MallocPessimistic, and CK_NewDeleteChecker.


      I.getKey()->dumpToStream(Out);
      Out << " : ";
      I.getData().dump(Out);


_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu <mailto:cfe-commits@cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

--
Anton

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits