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

Fri Aug 6 08:45:27 PDT 2021

steakhal added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:504
+
+  OrigStdin = findStdStream("stdin", C);
+  OrigStdout = findStdStream("stdout", C);
----------------
balazske wrote:
> steakhal wrote:
> > 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.
> I tried to check if these `SymbolRef`'s are the same at `checkBeginFunction` after initialized once. The assertion for equality failed sometimes, or other crash happened when `dump` was called on the value. So it looks not safe to cache these. And should we add assumptions about that these `StdinSym`, `StdoutSym`, `StderrSym` are not equal to each other?
Good to know. I don't think it's necessary to add assertions.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106644