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
