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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits