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

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1299
+    // StdLibraryFunctionsChecker.
+    ExplodedNode *Pred = const_cast<ExplodedNode *>(Node);
+    if (!Case.getNote().empty()) {
----------------
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).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1309
+            [Node, Note](PathSensitiveBugReport &BR) -> std::string {
+              if (Node->succ_size() > 1)
+                return Note.str();
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153612/new/

https://reviews.llvm.org/D153612