[all-commits] [llvm/llvm-project] c01914: [analyzer][NFC] Split the main logic of NoStoreFun...

[Kristóf Umann via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: c019142a89b477cd247434c1d8f571662d26e19d
      https://github.com/llvm/llvm-project/commit/c019142a89b477cd247434c1d8f571662d26e19d
  Author: Kristóf Umann <dkszelethus at gmail.com>
  Date:   2021-08-16 (Mon, 16 Aug 2021)

  Changed paths:
    M clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
    M clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

  Log Message:
  -----------
  [analyzer][NFC] Split the main logic of NoStoreFuncVisitor to an abstract NoStateChangeVisitor class

Preceding discussion on cfe-dev: https://lists.llvm.org/pipermail/cfe-dev/2021-June/068450.html

NoStoreFuncVisitor is a rather unique visitor. As VisitNode is invoked on most
other visitors, they are looking for the point where something changed -- change
on a value, some checker-specific GDM trait, a new constraint.
NoStoreFuncVisitor, however, looks specifically for functions that *didn't*
write to a MemRegion of interesting. Quoting from its comments:

/// Put a diagnostic on return statement of all inlined functions
/// for which  the region of interest \p RegionOfInterest was passed into,
/// but not written inside, and it has caused an undefined read or a null
/// pointer dereference outside.

It so happens that there are a number of other similar properties that are
worth checking. For instance, if some memory leaks, it might be interesting why
a function didn't take ownership of said memory:

void sink(int *P) {} // no notes

void f() {
  sink(new int(5)); // note: Memory is allocated
                    // Well hold on, sink() was supposed to deal with
                    // that, this must be a false positive...
} // warning: Potential memory leak [cplusplus.NewDeleteLeaks]

In here, the entity of interest isn't a MemRegion, but a symbol. The property
that changed here isn't a change of value, but rather liveness and GDM traits
managed by MalloChecker.

This patch moves some of the logic of NoStoreFuncVisitor to a new abstract
class, NoStateChangeFuncVisitor. This is mostly calculating and caching the
stack frames in which the entity of interest wasn't changed.

Descendants of this interface have to define 3 things:

* What constitutes as a change to an entity (this is done by overriding
wasModifiedBeforeCallExit)
* What the diagnostic message should be (this is done by overriding
maybeEmitNoteFor.*)
* What constitutes as the entity of interest being passed into the function (this
is also done by overriding maybeEmitNoteFor.*)

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