Recognize initial value of standard streams (stdin, ...)
and assume that a new opened stream is not equal to these.
Details
- Reviewers
Szelethus NoQ steakhal martong vsavchenko
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
233–236 | Would it be possible to reuse (i.e put in a common place) the StdLibraryFunctionChecker's lookupTy? That also handles the cases when FILEType is null or when we have a typedef struct FILE FILE;. |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
233–236 | Could you please consider the above question? |
I like the idea, though I wonder whether evalAssume would be a better callback for this. That way, you'd only need to add an assumption when you reach a condition where one of the operands is standard.
Though it may be more trouble than its worth. I don't have strong feelings on this.
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
256 | How about getStdStreamSymbol? |
Split some type lookup functions from StdLibraryFunctionsChecker into separate files.
Thank you, this is awesome!
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
490 | We should be careful, to cache the results (either here, or deeper in the call stack). |
clang/lib/StaticAnalyzer/Checkers/ASTLookup.cpp | ||
---|---|---|
27 | Decl *D -> const Decl *D. Same for the other loop. | |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
242 | You calculate this 3 times now. I mean it's not a big deal, but we could save this to do it only once. | |
244–253 | It will early return and uses one fewer continue. | |
490 | I agree. We should do this only for the first top-level function, instead of doing this for every top-level function. | |
573 | It might be a personal preference but I think lambdas should be pure in the sense that it takes something and produces something else. | |
577 | SValBuilder::getConditionType(), oh they are the same under the hood. We should still probably prefer this one instead. | |
578–581 | I think you should be fine with castAs(). I'm not expecting this to fail. | |
clang/test/Analysis/stream.c | ||
278–279 | How about checking this way instead? clang_analyzer_eval(F != stdin); // TRUE clang_analyzer_eval(F != stdout); // TRUE clang_analyzer_eval(F != stderr); // TRUE |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
244–253 | 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. | |
490 | 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. |
Init std declarations and FILE type only once.
Make a lambda not operate on captured data.
Simplify test.
Let's see if we can cache the stream symbols across top-level analyses.
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
244–253 | Hm, it was not immediately clear to me. | |
490 | 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. | |
493 | ||
497 | ||
538 | This should be probably at the definition of StdinSym and getStdStreamSym(). |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
490 | 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? | |
577 | This lambda is possible to improve. |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
490 | Good to know. I don't think it's necessary to add assertions. |
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | ||
---|---|---|
490 | Okay, so we should not cache the SymbolRefs then. But we must cache the VarDecls. I mean, we should avoid calling StdinDecl = findStdStreamDecl("stdin", C); more than once for each TU. I think you could use and Optional for StdinDecl (and the others) to achieve this. |
Decl *D -> const Decl *D. Same for the other loop.