[clang] [NFC][analyzer] Remove class 'NodeBuilder' (PR #217319)

[Donát Nagy via cfe-commits]

================
@@ -538,21 +536,35 @@ void CheckerManager::runCheckersForBeginFunction(ExplodedNodeSet &Dst,
   expandGraphWithCheckers(C, Dst, Src);
 }
 
-/// Run checkers for end of path.
-// Note, We do not chain the checker output (like in expandGraphWithCheckers)
-// for this callback since end of path nodes are expected to be final.
+/// Run checkers for end of a function (either the entrypoint or another
+/// function that was inlined). Note that this function places the
+/// checker activations on separate execution paths:
+///        /-[checker1]-> N1 ...
+///   Pred --[checker2]-> N2 ...
+///        \-[checker3]-> N3 ...
+/// (If none of the checkers produce a transition, we continue with 'Pred'.)
+///
+/// This differs from the handling of all the other checker callbacks, where
+/// the checker activations are chained sequentially on a single path:
+///   Pred --[checker1]-> N1 --[checker2]-> N2 --[checker3]-> N3 ...
+///
+/// This difference has historical reasons: originally this callback was called
+/// 'EndPath' and only activated at the end of an execution paths, and
+/// (according to an old comment) those 'EndPath' checkers expected that they
+/// create an "end of path" node which will be final.
+/// TODO: Check whether this exceptional behavior is still justified.
 void CheckerManager::runCheckersForEndFunction(ExplodedNodeSet &Dst,
                                                ExplodedNode *Pred,
                                                ExprEngine &Eng,
                                                const ReturnStmt *RS) {
----------------
NagyDonat wrote:

> Could you trace it back to when this bug was introduced [...]

This checker callback always had this behavior. The name `EndFunction was introduced in 2013 by commit 3fdcc0bda3c486c371159acdbc370933860dbe42 which renamed the callback `EndPath` to `EndFunction` without changing its behavior. (The commit message of that commit says "This better reflects when callback is called and what the checkers are relying on. (Both names meant the same pre-IPA.)" where IPA presumably means interprocedural analysis i.e. inlining function bodies.)

The implementation of the "execute checkers in parallel" logic can be traced back to commit 3eae33412d18c4a4a4a8592898b3e65ad5946a89 from 2011 -- but that commit just refactors an earlier implementation (by eliminating a class called `EndOfFunctionNodeBuilder` from the codebase). I'm pretty sure that earlier implementations also had this behavior and I don't think that we need to dig deeper.

> [...] and how many checks used this callback to judge the blast radius?

The following checkers (8 "real" + 3 dummy) have `check::EndFunction` callbacks:
- `ObjCDeallocChecker`  calls `diagnoseMissingReleases()` which creates non-fatal error nodes and tweaks the state in checker-specific and apparently uninteresting ways (to suppress duplicate reporting of the same error),
- `MIGChecker` generates fatal error nodes,
- `MallocChecker` calls `CheckEscapeOnReturn` to emit fatal use-after-free error nodes
- `RetainCountChecker` can emit non-fatal error nodes and tweaks the checker-specific state in a complex way that I don't (want to) understand,
- `StackAddrEscapeChecker` has a `checkEndFunction` callback that consist of 150+ lines with a locally defined class -- I don't know what does it do,
- `TestAfterDivZeroChecker` and `UnconditionalVAArgChecker clean up their checker-specific state by removing entries that "belong to" the function that is being ended
- there are three dummy checkers: `AnalysisOrderChecker`, `CheckerDocumentation`, `TraversalDumper`

> How did we not notice this earlier?

- Answer 1: We don't read the source code where this unusual behavior was proudly highlighted by fat comment blocks.
- Answer 2: As far as I see the existing `EndFunction` callbacks all do things that usually keep working if they are "banished" to a parallel execution path instead of the primary one:
  - Error nodes are still created, the associated bug report still appears (although the analysis can continue "after" a fatal error, if the parallel execution of another checker provides a path forward).
  - The cleanup of checker-specific state is a "nice to have" thing, and failure to remove irrelevant checker-specific information is almost completely irrelevant (but theoretically it could prevent merging execution paths).

With our current set of checkers, this exceptional logic only introduces a few redundant execution paths and other minor irregularities of the `ExplodedGraph`. As the growth of the graph is a poorly understood, almost magical process, it is not surprising that we did not spot these.

In general it is important to note that **the existing checkers only exercise a small fraction of the engine logic** so logic errors like this are probably widespread in the engine.

In fact, one major motivation for this `NodeBuilder` removal commit series was that this is a good framework for reviewing many parts of the engine to improve my knowledge and spot issues like this one. (This is roughly the fifth logic error uncovered this way.) The removal of `class NodeBuilder` is a good thing in itself, but these side effects are perhaps even more important.

https://github.com/llvm/llvm-project/pull/217319