[PATCH] D106644: [clang][analyzer] Add standard streams to alpha.unix.Stream checker.

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Let's see if we can cache the stream symbols across top-level analyses.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:476
+
+  for (Decl *D : LookupRes) {
+    D = D->getCanonicalDecl();
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:480
+      continue;
+    if (auto *VD = dyn_cast<VarDecl>(D)) {
+      if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))
----------------



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:521
+
+  // These can be nullptr if not found.
+  StdinSym = getStdStreamSym(StdinDecl, C);
----------------
This should be probably at the definition of `StdinSym` and `getStdStreamSym()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:227-236
+  for (Decl *D : LookupRes) {
+    D = D->getCanonicalDecl();
+    if (!C.getSourceManager().isInSystemHeader(D->getLocation()))
+      continue;
+    if (auto *VD = dyn_cast<VarDecl>(D)) {
+      if (FILEType && !ACtx.hasSameType(*FILEType, VD->getType()))
+        continue;
----------------
balazske wrote:
> steakhal wrote:
> > It will early return and uses one fewer `continue`.
> The intent was that if no `FILEType` is found we find just the first `VarDecl` and do not check the type. This recommended change returns always null if no `FILEType` is found.
Hm, it was not immediately clear to me.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);
----------------
balazske wrote:
> steakhal wrote:
> > martong wrote:
> > > We should be careful, to cache the results (either here, or deeper in the call stack).
> > > I mean, we certainly don't want to do a lookup of "stdin" every time a function is evaluated. We should do this lazily, only once.
> > I agree. We should do this only for the first top-level function, instead of doing this for every top-level function.
> I am not sure if the `SymbolRef` values are safe to store between top-level function analyses. The `FILE` type and std stream declarations are safe to cache, but the SymbolRef values of these variables probably not.
I think it should be safe. But place there an assert and run the test suite. If it won't trigger, that means that we have high confidentiality that this is safe. I know it's not 100%, but pretty close.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644