[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:

@steakhal @Xazax-hun @haoNoQ Did you know that the `EndFunction` has this unusual behavior? (It isn't mentioned in `CheckerDocumentation.cpp` and I didn't spot comments referencing it in `check::EndFunction` checkers.)

Should we preserve it or should we align this with the behavior of all other callbacks?

I briefly reviewed that we have roughly seven "real" `check::EndFunction` checkers and my impression is that most of them just emits leak reports, so the "activate in parallel" logic is mostly irrelevant for them.

However, the `UnconditionalVAArgChecker` which was written by me earlier this year uses an `EndFunction` callback to clean up a state trait that is no longer relevant (but not harmful); here the parallel checker execution is technically incorrect, because it can avoid the cleanup step when it follows a transition by another checker.

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