Szelethus added a comment. I gave a lot of thought to the high level logic, but I need some time to really internalize whats happening here.
In D78374#1993858 <https://reviews.llvm.org/D78374#1993858>, @balazske wrote: > Finally I had to make the decision to remove the `ErrorKindTy` enum and use > boolean flags instead for every possible error (including no error). This is > needed because we do not know always what error is possible if it is > "unknown". It could be determined from the last operation but now not any > more. The documentation for `fread` says that if 0 is passed as the size or > count argument the function returns zero and does not change the state. So > the error state before `fread` remains active. The new design is to have bool > values for every error kind (**feof** and **ferror** and a no-error state) so > multiple can be active in a state to indicate that more than one error state > is possible. If no-error or **feof** is possible these flags are turned on. > If we need to know in such a state if the stream is in **EOF** a state split > is needed to handle both cases (one with **EOF** and one with no-error). This > split must be done in the pre-call handler, for example if we want a warning > that the operation is not safe to use in **EOF** state. (But in this case > really no split is needed, only clear the EOF flag and make a warning.) I guess one solution would be to have a history of last operations on the stream, but overall, it makes sense to have a make a shift to calculate the possible errors as we're evaluating the functions. Great idea! > We can have another approach if we do not set return values of the functions > at all, instead save the symbol of the function and determine the returned > value later from the constraints actually applied on it. This may save state > splits, but only until a condition is encountered that checks for the > function's return value. Or any stream modifying operation. If we don't have branches for the return value, that is almost a bug in itself. Also, digging the return value out of a branch condition may be difficult (see `ReturnValueChecker`). Lets stick with the state splits at the function call for now. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222 {StreamState::FEof, StreamState::FError, StreamState::NoError}}}, - {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}}, + // Note: ftell sets errno only. + {{"ftell", 1}, ---------------- balazske wrote: > Szelethus wrote: > > Szelethus wrote: > > > C'99 standard [[ > > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | > > > §7.19.9.4.3]], about the `ftell function`: > > > > > > > If successful, the `ftell` function returns the current value of the > > > > file position indicator for the stream. On failure, the `ftell` > > > > function returns `-1L` and stores an implementation-defined positive > > > > value in `errno`. > > > > > > So we need an evalCall for this. > > I mean, this function can only fail if the stream itself is an erroneous or > > closed state, right? So we can associate the -1 return value with the > > stream being in that, and the other with the stream being opened. > The `ftell` is part of a later patch that is adding `ftell` (and probably > other functions). Only the "PossibleErrors" was updated in this patch because > its meaning was changed (it contains value even if only one error kind is > possible, before it was used only if at least two errors are possible). > It is not sure how the file operations work, maybe the `ftell` needs access > to some data that is not available at the moment (and can fail even if the > stream was not OK). If the stream is already in failed state the question is: > Do ftell fail (then the we can make it return -1) or it does not fail but > gives an unusable value (then generate a checker warning and stop the > analysis). In any case, this should be removed from the patch, because adding it seems to be an orthogonal change to this one. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37 +/// Note: A stream has either FEOF or FERROR set but not both at the same time. +struct StreamErrorState { + /// There is an execution path with none of the error flags set. ---------------- We're not describing the error state of a stream here, but rather //possible// error states, so the name should reflect that. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:207-210 + {{"fread", 4}, + {&StreamChecker::preFread, + std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, + ErrorFEof | ErrorFError), ---------------- What does this mean? I guess not the possible states after an fread call, but rather the possible states after a **failed** fread call, but then... ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:225-228 {{"feof", 1}, {&StreamChecker::preDefault, - &StreamChecker::evalFeofFerror<StreamState::FEof>, - 0, - {StreamState::FError, StreamState::NoError}}}, - // Note: ferror can result in Unknown if at the call there is a - // PossibleErrors with all 3 error states (including NoError). - // Then if ferror is false the remaining error could be FEof or NoError. + std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof), + 0}}, ---------------- ...this doesn't make much sense, `feof` doesn't **cause** and error, it merely detects it. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + ---------------- Will that be the next patch? :D Eager to see it! ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:512 + + Optional<NonLoc> SizeVal = Call.getArgSVal(1).castAs<NonLoc>(); + if (!SizeVal) ---------------- `castAs` doesn't return an `Optional`. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:533-539 + ProgramStateRef StateNotFailed = + State->BindExpr(CE, C.getLocationContext(), *CountVal); + if (StateNotFailed) { + StateNotFailed = + StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc)); + C.addTransition(StateNotFailed); + } ---------------- This is where I'd really like to see a precise quote from the standard. [[http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | C'99 standard]], §7.19.8.1.3, the return value of fread: The fread function returns the number of elements successfully read, which may be less than nmemb if a read error or end-of-file is encountered. If size or nmemb is zero, fread returns zero and the contents of the array and the state of the stream remain unchanged. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78374/new/ https://reviews.llvm.org/D78374 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
