[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

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