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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2549-2552
+      FunctionStr = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(
+              {FD->getBeginLoc(), FD->getBody()->getBeginLoc()}),
+          C.getSourceManager(), C.getLangOpts());
----------------
NoQ wrote:
> 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.
Oh, I missed that, thanks! I wanted to check for everything, yes.


================
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;
----------------
NoQ wrote:
> If the string is empty, it clearly cannot contain `__isl_`, so the first check is redundant.
The first check is: "We got a body and its decl?", the second check is: "We got an ISL macro?". Yea, it is kind of redundant, just I like to pack one check in one IfStmt, and now that two question merges. Also I like to make them one-liners so they are self-explanatory.

Here is an example why I like it:
```
        // Escape pointers passed into the list, unless it's an ObjC boxed       
        // expression which is not a boxable C structure.                        
        if (!(isa<ObjCBoxedExpr>(Ex) &&                                          
              !cast<ObjCBoxedExpr>(Ex)->getSubExpr()                             
                                      ->getType()->isRecordType()))
```
- from `ExprEngine::Visit()` - `Expr::ObjCBoxedExprClass` case.


================
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));
----------------
NoQ wrote:
> `remove` is unnecessary, we overwrite it anyway.
I believe in so as well, just the official code base has this semantic. I have rewritten that, see below in `checkPointerEscapeAux()`.


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

https://reviews.llvm.org/D64680