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