[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: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:
> > 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?`
> The return value case is not as simple because the `DefinedSVal` has no default constructor, but it is not as bad to return only the `RetVal` and have a `CE` argument.
I like the current solution very much!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:31-33
+  // State of a stream symbol.
+  // In any non-opened state the stream really does not exist.
+  // The OpenFailed means a previous open has failed, the stream is not open.
----------------
Could you please move these to the individual enums please? :) I have an indexer that can query those as documentation.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37-40
+  // NoError: No error flag is set or stream is not open.
+  // EofError: EOF condition (feof returns true)
+  // OtherError: other (non-EOF) error (ferror returns true)
+  // AnyError: EofError or OtherError
----------------
These too. Also, I'm not yet sure whether we need `OtherError` and `AnyError`, as stated in a later inline.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:333-335
+  // Ignore the call if the stream is is not tracked.
+  if (!State->get<StreamMap>(StreamSym))
+    return;
----------------
If we check in `preCall` whether the stream is opened why don't we conservatively assume it to be open?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:351-354
+  // Set state to any error.
+  // Assume that EOF and other error is possible after the call.
+  StateFailed =
+      StateFailed->set<StreamMap>(StreamSym, StreamState::getOpenedWithError());
----------------
But why? The standard suggests that the error state isn't changed upon failure. I think we should leave it as-is.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:438-439
   const StreamState *SS = State->get<StreamMap>(Sym);
   if (!SS)
     return State;
 
----------------
Right here, should we just assume a stream to be opened when we can't prove otherwise? `ensureStreamOpened` is only called when we are about to evaluate a function that assumes the stream to be opened anyways, so I don't expect many false positive lack of `fclose` reports.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75356