[PATCH] D105553: [analyzer][NFC] Split the main logic of NoStoreFuncVisitor to an abstract NoStateChangeVisitor class

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

Szelethus created this revision.
Szelethus added reviewers: NoQ, vsavchenko, martong, steakhal, ASDenysPetrov.
Szelethus added a project: clang.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, sunfish, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity.
Szelethus requested review of this revision.
Herald added subscribers: cfe-commits, aheejin.

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 `maybeEmitNote`)
- What constitutes as the entity of interest being passed into the function (this is also done by overriding `maybeEmitNote`, to be factored out a bit more in a later patch)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105553

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105553.356955.patch
Type: text/x-patch
Size: 19806 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20210707/215515cc/attachment-0001.bin>