[PATCH] D108695: [analyzer][NFCI] Allow clients of NoStateChangeFuncVisitor to check entire function calls, rather than each ExplodedNode in it

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D108695#2967885 <https://reviews.llvm.org/D108695#2967885>, @Szelethus wrote:

> It should! But you have a point, I don't have the code to prove it right away. Maybe if I factored out the symbol part into NoStateChangeToSymbolVisitor, as teased in D105553#2864318 <https://reviews.llvm.org/D105553#2864318>, it'd have an easier time to see it. With that said, I don't currently see how the current implementation would be faulty, but even if it is, the patch adds an option, and doesn't the take old one (that NoStoreFuncVisitor still uses) away.

I mean, your implementation of `findModifyingFrames()` mostly boils down to:

  return CallEnterN->getState()->get<State>(X) !=
       CallExitEndN->getState()->get<State>(X);

So if, say, X is a mutex and we're wondering whether it was unlocked in the function, but in reality it was unlocked and immediately locked again, then the above test would say "not modified" and you can't notice that it was modified temporarily without scanning all intermediate nodes. So if your note needs to make a distinction between "was not touched" and "was touched but reverted" you'll still need to do it the old way. I think this should be documented loudly in the comments.



================
Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:369
+static const ExplodedNode *getCallExitEnd(const ExplodedNode *N) {
+  while (N && !N->getLocationAs<CallExitEnd>())
+    N = N->getFirstSucc();
----------------
Wait, no, I don't think this test is sufficient. You may run into a `CallExitEnd` of a nested stack frame before you reach the `CallExitEnd` of the stack frame in question:
```
-> CallEnter foo()       --->
  -> CallEnter bar()        |
  <- CallExitEnd bar()      <--- ???
<- CallExitEnd foo()
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108695