[all-commits] [llvm/llvm-project] efd1a8: [analyzer][MallocChecker] Make NewDeleteLeaks depe...

[Kristóf Umann via All-commits]

  Branch: refs/heads/newdelete_dependency_revamp
  Home:   https://github.com/llvm/llvm-project
  Commit: efd1a8e66eaa13afff709ebf16ff6280caa82ead
      https://github.com/llvm/llvm-project/commit/efd1a8e66eaa13afff709ebf16ff6280caa82ead
  Author: Kristóf Umann <dkszelethus at gmail.com>
  Date:   2020-05-27 (Wed, 27 May 2020)

  Changed paths:
    M clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
    M clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
    R clang/test/Analysis/Malloc+NewDelete_intersections.cpp
    M clang/test/Analysis/NewDelete-checker-test.cpp
    M clang/test/Analysis/NewDelete-intersections.mm
    M clang/test/Analysis/new.cpp

  Log Message:
  -----------
  [analyzer][MallocChecker] Make NewDeleteLeaks depend on DynamicMemoryModeling rather than NewDelete

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

Differential Revision: https://reviews.llvm.org/D77474