[PATCH] D75356: [Analyzer][StreamChecker] Introduction of stream error state handling.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:50-68
+struct StreamErrorState {
+  // The error state of an opened stream.
+  // EofError: EOF condition (feof returns true)
+  // OtherError: other (non-EOF) error (ferror returns true)
+  // AnyError: EofError or OtherError
+  enum Kind { EofError, OtherError, AnyError } K;
+
----------------
balazske wrote:
> Szelethus wrote:
> > Shouldn't we merge this with `StreamState`?
> The intention was that the error state is only stored when the stream is opened (in opened state). Additionally it exists in the map only if there is error, so no "NoError" kind is needed. This is only to save memory, if it is not relevant I can move the error information into `StreamState` (that will contain two enums then).
I personally wouldn't worry about memory consumption in this case too much, considering how much information needs to be store for simple expressions, stream objects are relatively few and far in between, even on projects that use them a lot.

Having one more enum in `StreamState` would be better in this case then! :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:92-125
+class MakeRetVal {
+  const CallExpr *CE = nullptr;
+  std::unique_ptr<DefinedSVal> RetVal;
+  SymbolRef RetSym;
+
+public:
+  MakeRetVal(const CallEvent &Call, CheckerContext &C)
----------------
balazske wrote:
> Szelethus wrote:
> > Do you have other patches that really crave the need for this class? Why isn't `CallEvent::getReturnValue` sufficient? This is a legitimate question, I really don't know. :)
> This is an "interesting" solution for the problem that there is need for a function with 3 return values. The constructor performs the task of the function: Create a conjured value (and get the various objects for it). The output values are RetVal and RetSym, and the success state, and the call expr that is computed here anyway. It could be computed independently but if the value was retrieved once it is better to store it for later use. (I did not check how costly that operation is.)
> 
> I had some experience that using only `getReturnValue` and make constraints on that does not work as intended, and the reason can be that we need to bind a value for the call expr otherwise it is an unknown (undefined?) value (and not the conjured symbol)?
I suspect that `getReturnValue` might only work in `postCall`, but I'm not sure.

I think instead of this class, a function returning a `std::tuple` would be nicer, with `std::tie` on the call site. You seem to use all 3 returns values in the functions that instantiate `MakeRetVal` anyways :).

In `StdLibraryFunctionsChecker`'s `evalCall`, the return value is [[https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp#L403|explicitly constructed]], and further constraints on it are only imposed in `postCall`. I wonder why that is. @martong, any idea why we don't `apply` the constraints for pure functions in `evalCall?`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:383
+  // Record the failed status, only if failed.
+  // fseek clears the EOF flag, sets only error flag.
+  StateFailed = StateFailed->set<StreamErrorMap>(RV.getRetSym(),
----------------
According to the C'98 standard [[http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf|§7.19.9.2.5]]:
> After determining the new position, a successful call to the fseek function undoes any effects of the `ungetc` function on the stream, clears the end-of-file indicator for the stream, and then establishes the new position. After a successful fseek call, the next operation on an update stream may be either input or output.

So it definitely doesn't clear the `EOF` flag on failure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75356