donat.nagy added inline comments.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+ // StdLibraryFunctionsChecker.
+ ExplodedNode *Pred = const_cast<ExplodedNode *>(Node);
+ if (!Case.getNote().empty()) {
----------------
balazske wrote:
> donat.nagy wrote:
> > Can you explain why is it safe to use `const_cast` here? (I don't see any
> > concrete issue, but the engine has lots of invariants / unwritten rules and
> > I fear that this might break one of them.)
> The node `Pred` should be modified only later when a successor is added
> (`addTransition` has non-const parameter).
I understood that you //need// a non-const `ExplodedNode *` because
`addTransition` expects it; I want to understand why you are //allowed to//
`const_cast` it (why doesn't this confuse the engine logic).
Equivalent question from the other direction: Why did the author of
`CheckerContext::getPredecessor()` specify that its return value is a //const//
pointer to `ExplodedNode`?
If we can conclude that `const_cast` is valid in this kind of situation, then
I'd also consider simply removing the "const" from the return type of
`getPredecessor`.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1309
+ [Node, Note](PathSensitiveBugReport &BR) -> std::string {
+ if (Node->succ_size() > 1)
+ return Note.str();
----------------
balazske wrote:
> donat.nagy wrote:
> > It's surprising to see this check inside the lambda, as its result does not
> > depend on `BR`. My best guess is that it's performed here because the
> > successors of `Node` will appear between the execution of the surrounding
> > code and the execution of this lambda.
> >
> > However, CheckerContext.h line 69-70 claims that "checkers should not
> > retain the node in their state since the nodes might get invalidated."
> > which would imply that the captured `Node` might be invalid when the lambda
> > is called.
> This check is to decide if multiple cases could be applied, the same as if we
> count how many times this place in the loop is executed (add a transition for
> a case, constraints could be applied). This check is problematic because
> other checkers can apply state splits before this checker is executed or
> after it, in this way `StreamChecker` interferes with this code (it has a
> state split for success/failure cases of same function, and here we see only
> that a single case is applied on one branch). This is why this check is only
> used in the `EvalCallAsPure` case (theoretically still another checker can
> make a state split in PostCall before this where the same constraint is
> applied, then the problem occurs again).
>
> I made a solution that does not have this check but has 2 case loops instead,
> but the mentioned problem (which exists when `if (Summary.getInvalidationKd()
> == EvalCallAsPure)` is not used) did not go away. And it may not work to
> search backwards for the first node with the same statement, because maybe
> not the first one is where a state split is done.
>
> I only think that if this lambda is called with the saved node, that node is
> not invalid because it is part of a bug report call sequence.
> This check is problematic because [...details...]
Thanks for the explanation, now I see why this roundabout solution is needed.
> I only think that if this lambda is called with the saved node, that node is
> not invalid because it is part of a bug report call sequence.
This is a good point, I think we can keep this "check successors in the lambda"
solution. Please add a comment like "This check is performed inside the lambda,
after other checkers had a chance to add other successors. Dereferencing the
saved node object is valid because it's part of a bug report call sequence." to
record the the reasoning that we discussed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153612/new/
https://reviews.llvm.org/D153612
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
