[PATCH] D64680: [analyzer] MallocChecker: Prevent Integer Set Library false positives

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2547-2548
+  StringRef FunctionStr = "";
+  if (const Decl *D = C.getStackFrame()->getDecl())
+    if (const FunctionDecl *FD = D->getAsFunction())
+      FunctionStr = Lexer::getSourceText(
----------------
`const auto *FD = dyn_cast<FunctionDecl>(C.getStackFrame()->getDecl())`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2549-2552
+      FunctionStr = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(
+              {FD->getBeginLoc(), FD->getBody()->getBeginLoc()}),
+          C.getSourceManager(), C.getLangOpts());
----------------
I'm slightly worried that it'll crash when `free()` is being called from within a body farm.

For now it probably cannot happen because none of the bodyfarmed functions can call `free()` directly, but i'd anyway rather add a check that the source locations we're taking are valid.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2554-2559
+  if (FunctionStr.equals(""))
+    return false;
+
+  // We do not model the Integer Set Library's retain-count based allocation.
+  if (!FunctionStr.contains("__isl_"))
+    return false;
----------------
If the string is empty, it clearly cannot contain `__isl_`, so the first check is redundant.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2564-2566
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(Arg->IgnoreImpCasts())) {
+      if (const auto *VD = dyn_cast<ParmVarDecl>(DRE->getDecl())) {
+        SVal V = State->getSVal(State->getLValue(VD, C.getLocationContext()));
----------------
`C.getSVal(Arg)`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2569
+          if (const RefState *RS = State->get<RegionState>(Sym)) {
+            State = State->remove<RegionState>(Sym);
+            State = State->set<RegionState>(Sym, RefState::getEscaped(RS));
----------------
`remove` is unnecessary, we overwrite it anyway.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64680/new/

https://reviews.llvm.org/D64680