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

This revision was automatically updated to reflect the committed changes. Closed by commit rG11bd3e5c6549: [Analyzer][StreamChecker] Introduction of stream error handling. (authored by balazske). Changed prior to commit: https://reviews.llvm.org/D75682?vs=255906=255935#toc Repository: rG

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

balazske updated this revision to Diff 255906. balazske added a comment. Adding diff to master. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 Files:

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

Szelethus accepted this revision. Szelethus added a comment. Whoo! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 ___ cfe-commits mailing list

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

balazske updated this revision to Diff 255602. balazske marked an inline comment as done. balazske added a comment. Moved test checker to debug package, changed macro to function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/

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

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:26-28 +#define ASSERT_STREAMSTATE_OPENED \ + assert(SS->isOpened() && \ +

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:26-28 +#define ASSERT_STREAMSTATE_OPENED \ + assert(SS->isOpened() && \ +

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

Szelethus added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:455 +def StreamTesterChecker : Checker<"StreamTester">, + HelpText<"Add test functions to StreamChecker for test and debugging purposes.">, NoQ wrote: > This

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

NoQ added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:455 +def StreamTesterChecker : Checker<"StreamTester">, + HelpText<"Add test functions to StreamChecker for test and debugging purposes.">, This should go into the

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

balazske updated this revision to Diff 255360. balazske added a comment. - Adding comment about ferror, feof. - Enabled core checker in test file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 Files:

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

balazske marked an inline comment as done. balazske added inline comments. Comment at: clang/test/Analysis/stream-error.c:1 -// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-checker=debug.ExprInspection -analyzer-store region -verify %s +// RUN:

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

Szelethus accepted this revision. Szelethus added a comment. This revision is now accepted and ready to land. LGTM, let's go! Thank you so much for sticking this out! Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:42-51 /// The error state of a stream. +

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

balazske added a comment. Ping. It is now testable, a sub-checker was added for testing (that can be useful at later improvements too). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682

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

balazske updated this revision to Diff 253587. balazske added a comment. Added test checker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 Files: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

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

Szelethus added a comment. In D75682#1940901 , @balazske wrote: > So what is missing or wrong in this change to be accepted? This change is functional but is not testable. But the high level idea is great! I think we should maybe merge this with the

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

balazske added a comment. So what is missing or wrong in this change to be accepted? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 ___ cfe-commits mailing

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

Szelethus added a comment. In D75682#1931108 , @balazske wrote: > Adding special test functions is not as easy, then `StreamState` should be > accessible from another checker. It could be added to the same file, or new > file but then moving the data

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

balazske added a comment. Adding special test functions is not as easy, then `StreamState` should be accessible from another checker. It could be added to the same file, or new file but then moving the data structures into header is needed. At least for short-term it is more simple to add a

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

baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:29 + assert(SS->isOpened() && \ + "Create of error node failed (only way to get this state)?"); +

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

Szelethus added a subscriber: steakhal. Szelethus added a comment. In D75682#1929093 , @balazske wrote: > In D75682#1928716 , @Szelethus wrote: > > > - For streams where the precise state is unknown (they are not

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

balazske added a comment. In D75682#1928716 , @Szelethus wrote: > - For streams where the precise state is unknown (they are not tracked), > start tracking. If we explicitly check whether a state is `foef()`, we can > rightfully assume both of those

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

Szelethus added a comment. Herald added a subscriber: ASDenysPetrov. In D75682#1926889 , @balazske wrote: > In D75682#1926732 , @Szelethus wrote: > > > Rght I think I finally get it. You don't want to state

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

balazske added a comment. Herald added a subscriber: DenisDvlp. In D75682#1926732 , @Szelethus wrote: > How about untracked streams? What if we call `feof()` on a stream we got from > a parameter, wouldn't that suggest that the stream is probably

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

balazske added a comment. In D75682#1926732 , @Szelethus wrote: > Rght I think I finally get it. You don't want to state split on `feof()` > and `ferror()`, but rather on the stream operations! Yes the split is at the operations. I did not think

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

Szelethus added a comment. Rght I think I finally get it. You don't want to state split on `feof()` and `ferror()`, but rather on the stream operations! This is why you only want to tell the analyzer what the return value of these functions are going to be, because the state of the stream

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

balazske marked 3 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:88 struct FnDescription; using FnCheck = std::function; NoQ wrote: > balazske wrote: > > NoQ wrote: > > >

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

balazske updated this revision to Diff 250588. balazske marked 2 inline comments as done. balazske added a comment. Addressed some comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 Files:

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

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:88 struct FnDescription; using FnCheck = std::function; balazske wrote: > NoQ wrote: > > `llvm::function_ref`? > `function_ref`'s documentation says: > > This class does

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

balazske marked 6 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:88 struct FnDescription; using FnCheck = std::function; NoQ wrote: > `llvm::function_ref`? `function_ref`'s documentation

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

NoQ added a comment. It looks like for now there's no way to put the stream into an error state, right? Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45 + enum KindTy { +Opened, /// Stream is opened. +Closed, /// Closed stream (an invalid stream

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

balazske marked an inline comment as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:393-394 + + // Make the return value accordingly to the error. + State = State->assume(RetVal, (SS->*IsOfError)()); + assert(State &&

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

Szelethus added a comment. Thanks for sticking this out! It just takes me a while to get to the mental space regarding this patch you are already in, sometimes through saying incorrect statements or stupid suggestions! :) If others could chip in in this discussion, it would be most

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

balazske marked 9 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45 + enum KindTy { +Opened, /// Stream is opened. +Closed, /// Closed stream (an invalid stream pointer after it was closed). +

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

balazske added a comment. In D75682#1917257 , @Szelethus wrote: > Could you please add the warning we discussed? That would be a great > demonstration of this patch's capabilities, and wouldn't deserve its own > patch. Was it this warning? if

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

Szelethus added a comment. Could you please add the warning we discussed? That would be a great demonstration of this patch's capabilities, and wouldn't deserve its own patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/

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

balazske updated this revision to Diff 249643. balazske added a comment. Adding correct diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 Files: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

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

balazske updated this revision to Diff 249642. balazske added a comment. - Removed AnyError. - Using common code for `evalFeof` and ferror. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 Files:

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

baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45 + enum KindTy { +Opened, /// Stream is opened. +Closed, /// Closed stream (an invalid stream pointer after it was closed). +OpenFailed /// The last open

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

Szelethus added a comment. I see that we're a bit stuck on naming, and that is largely my fault. Apologies, let us focus on high level stuff for a bit. In D75682#1916435 , @balazske wrote: > I do not understand totally this remove-of-AnyError thing. If

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

Szelethus added a comment. Also, thank you guys for chipping in! Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394 +StreamSym, StreamState::getOpenedWithOtherError()); +C.addTransition(StateEof); +C.addTransition(StateOther);

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

balazske added a comment. And why not add first the "unknown error" state and later the feof and ferror error states? Or add every error state but not the `feof` and `ferror` functions? Every way results in an intermediate state of the checker. Repository: rG LLVM Github Monorepo CHANGES

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

baloghadamsoftware added a comment. In D75682#1916809 , @balazske wrote: > What is better? Have this (or similar) change that adds a feature that is > used in a later change and is only testable in that later change, or have a > bigger change that

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

balazske marked 4 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45 + enum KindTy { +Opened, /// Stream is opened. +Closed, /// Closed stream (an invalid stream pointer after it was closed). +

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

balazske added a comment. What is better? Have this (or similar) change that adds a feature that is used in a later change and is only testable in that later change, or have a bigger change that contains real use of the added features? (This means: Add `fseek` to this change or not.) The error

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

baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:43 +EofError, /// EOF condition (`feof` is true). +OtherError, /// Other (non-EOF) error (`ferror` is true). +AnyError/// EofError or OtherError, used if

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

balazske marked 2 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394 +StreamSym, StreamState::getOpenedWithOtherError()); +C.addTransition(StateEof); +C.addTransition(StateOther);

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

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45 + enum KindTy { +Opened, /// Stream is opened. +Closed, /// Closed stream (an invalid stream pointer after it was closed). +OpenFailed /// The last open operation

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

baloghadamsoftware added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45 + enum KindTy { +Opened, /// Stream is opened. +Closed, /// Closed stream (an invalid stream pointer after it was closed). +OpenFailed /// The last open

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

xazax.hun added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45 + enum KindTy { +Opened, /// Stream is opened. +Closed, /// Closed stream (an invalid stream pointer after it was closed). +OpenFailed /// The last open operation

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

balazske marked 2 inline comments as done. balazske added a comment. I do not understand totally this remove-of-AnyError thing. If handling the `AnyError` will be removed the code remains still not testable. Because none of the possible failing file operations are modeled in this revision. A

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

Szelethus added a comment. You've sold me on `AnyError`, we should have something like that with the addition of `NoteTags`. With that said, I feel uneasy about adding it in this patch and not testing it properly. I can also sympathize with your anxiety against bloating the `fseek` patch

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

balazske marked 6 inline comments as done. balazske added a comment. The problem here with tests is that there is no function to call that sets the error flags `FEof` or `FError`. So it is only possible to check if these are not set after opening the file. Comment at:

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

Szelethus added a comment. Can we see more test cases for when after a call to `feof` we indeed can tell the stream is in an `EOF` state? Same for `ferror`. I feel like there is a lot of code we don't yet cover. Also, I see now that I didn't get in earlier revisions that `OtherError` actually

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

balazske updated this revision to Diff 249107. balazske added a comment. Fixed lint warnings. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75682/new/ https://reviews.llvm.org/D75682 Files: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

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

balazske updated this revision to Diff 249085. balazske added a comment. Removed support for `fseek`, added `clearerr`. This is now error handling only without fseek. No functions set the error flags so test for clearerr is not possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. Let's halt this for now -- we have so many revisions going on seemingly doing the same thing, its becoming really confusing. Please finish D75356

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

balazske added a comment. We can not make a warning if a stream operation is called in failed state: The error was probably handled based on the return value of the previous failed operation. This thing is checked by the "ErrorReturnChecker" (https://reviews.llvm.org/D72705). Only some special

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

balazske added a comment. Some explanation of the error states: - `EofError` indicates an EOF condition in the stream. - `OtherError` indicates a generic (I/O or other but not EOF) error. - `AnyError` is a "placeholder" if the exact error kind is not important. This is a "Schrödinger's cat"

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

balazske added a comment. For older discussion see this: https://reviews.llvm.org/D75356 Functions `feof` and `ferror` are added to show how the stream error flags are used and to make tests possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

balazske created this revision. Herald added subscribers: cfe-commits, martong, Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. Herald added a reviewer: Szelethus. Herald added a project: clang. balazske added a comment. For