[PATCH] D135247: [clang][analyzer] Add stream functions to StdLibraryFunctionsChecker.

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D135247#3867445 <https://reviews.llvm.org/D135247#3867445>, @balazske wrote:

> If `StdCLibraryFunctionsChecker` is added as dependency to `StreamChecker` and no other thing is changed these tests fail, and I could not find out the reason:
> Errors in test **stream.c**:
>
>   error: 'warning' diagnostics expected but not seen: 
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 7: Stream pointer might be NULL
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 13: Stream pointer might be NULL
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream.c Line 76: Stream pointer might be NULL
>
> Errors in test **stream-error.c**:
>
>   error: 'warning' diagnostics expected but not seen: 
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 97: might be 'indeterminate'
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 100: Stream might be already closed
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 113: Read function called when stream is in EOF state
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 114: Read function called when stream is in EOF state
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 123: FALSE
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 124: FALSE
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 128: FALSE
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 129: TRUE
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 133: TRUE
>     File /local/clang/llvm2/llvm-project/clang/test/Analysis/stream-error.c Line 134: FALSE
>
> Probably the `StdLibraryFunctionsChecker` runs before the `StreamChecker`, and runs always after it if no dependency is there? But this does not explain all test errors and should cause no errors at all. Adding the dependency itself is not enough, `ModelPOSIX` option should be true too. If the option is set to true in the test file even more test errors appear, and it does not work even when the (StdLibraryFunctionsChecker) checker is added (to the RUN command). Without the dependency the tests do not fail if the order of checkers in the enabled checkers list is changed.

Okay, this is bad. We must understand the reasons behind these failures. It is very strange that the `ModelPOSIX` option triggers these failures. I think we have to hunt down and examine each failing check, could you please continue the debugging of these?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1060-1064
+    } else if (NewState == State) {
+      if (const auto *D = dyn_cast_or_null<FunctionDecl>(Call.getDecl()))
+        if (const NoteTag *NT =
+                Case.getErrnoConstraint().describe(C, D->getNameAsString()))
+          C.addTransition(NewState, NT);
----------------
balazske wrote:
> martong wrote:
> > Why do we need this change?
> It is possible that only the errno related state is changed, no new constraints are added (if the constraint is already here from `evalCall` but the errno was not set there, for example at `fclose` or other stream functions maybe no new state is created here). In such case the note tag is still needed.
Okay, please add that as a comment to this new hunk.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:604-613
+  // Return 0 on success, EOF on failure.
+  SValBuilder &SVB = C.getSValBuilder();
+  ProgramStateRef StateSuccess = State->BindExpr(
+      CE, C.getLocationContext(), SVB.makeIntVal(0, C.getASTContext().IntTy));
+  ProgramStateRef StateFailure =
+      State->BindExpr(CE, C.getLocationContext(),
+                      SVB.makeIntVal(*EofVal, C.getASTContext().IntTy));
----------------
balazske wrote:
> martong wrote:
> > This is redundant with the summary in the `StdLibraryFunctionsChecker`. Why do we need this as well?
> It is probably needed to have a (any) bound value to the `fclose` function call, otherwise setting constraints in the other checker do not work. It may work to bind only a conjured value but it looks better if the correct return value is used. This makes less inter-dependence between the two checkers (`StdLibraryFunctionChecker` sets only the errno state as far as possible).
Ok, makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135247