================
@@ -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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
