[PATCH] D77474: [analyzer][MallocChecker] Make NewDeleteLeaks depend on DynamicMemoryModeling rather than NewDelete

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, baloghadamsoftware, martong, balazske, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, steakhal, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.
Szelethus edited the summary of this revision.

If you remember the mail [1] I sent out about how I envision the future of the already existing checkers to look dependencywise, one my main points was that no checker that emits diagnostics should be a dependency. This is more problematic for some checkers (ahem, RetainCount [2]) more than for others, like this one.

The MallocChecker family is a mostly big monolithic modeling class some small reporting checkers that only come to action when we are constructing a warning message, //after// the actual bug was detected. The implication of this is that NewDeleteChecker doesn't really do anything to depend on, so this change was relatively simple.

The only thing that complicates this change is that `FreeMemAux` (MallocCheckers method that models general memory deallocation) returns after calling a bug reporting method, regardless whether the report was ever emitted (which may not always happen, for instance, if the checker responsible for the report isn't enabled). This return unfortunately happens before cleaning up the maps in the GDM keeping track of the state of symbols (whether they are released, whether that release was successful, etc). What this means is that upon disabling some checkers, we would never clean up the map and that could've lead to false positives, e.g.:

  error: 'warning' diagnostics seen but not expected: 
    File clang/test/Analysis/NewDelete-intersections.mm Line 66: Potential leak of memory pointed to by 'p'
    File clang/test/Analysis/NewDelete-intersections.mm Line 73: Potential leak of memory pointed to by 'p'
    File clang/test/Analysis/NewDelete-intersections.mm Line 77: Potential leak of memory pointed to by 'p'
  
  error: 'warning' diagnostics seen but not expected: 
    File clang/test/Analysis/NewDelete-checker-test.cpp Line 111: Undefined or garbage value returned to caller
    File clang/test/Analysis/NewDelete-checker-test.cpp Line 200: Potential leak of memory pointed to by 'p'
  
  error: 'warning' diagnostics seen but not expected: 
    File clang/test/Analysis/new.cpp Line 137: Potential leak of memory pointed to by 'x'

There two possible approaches I had in mind:

- Make bug reporting methods of MallocChecker returns whether they succeeded, and proceed with the rest of `FreeMemAux` if not,
- Halt execution with a sink node upon failure. I decided to go with this, as described in the code.

As you can see from the removed/changed test files, before the big checker dependency effort landed, there were tests to check for all the weird configurations of enabled/disabled checkers and their messy interactions, I largely repurposed these.

[1] http://lists.llvm.org/pipermail/cfe-dev/2019-August/063070.html
[2] http://lists.llvm.org/pipermail/cfe-dev/2019-August/063205.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77474

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/test/Analysis/Malloc+NewDelete_intersections.cpp
  clang/test/Analysis/NewDelete-checker-test.cpp
  clang/test/Analysis/NewDelete-intersections.mm
  clang/test/Analysis/new.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D77474.255070.patch
Type: text/x-patch
Size: 21176 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200404/d73f9e58/attachment-0001.bin>