[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske abandoned this revision. balazske added a comment. Yes, closing it. (Will change `StreamErrorState` probably to bitfield later.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78374/new/ https://reviews.llvm.org/D78374

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added a comment. We can close this now, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78374/new/ https://reviews.llvm.org/D78374 ___ cfe-commits mailing list

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

baloghadamsoftware added a comment. > Is there a significant gain behind doing that instead of doing a state split > immediately? It feels unnatural to delay the split. It doesn't accurately > represent how the code would run. Are we supposed to do this at every > precall? What about other

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. Created new revisions for parts of this change and a bit improved: D80009 and others in "Stack". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78374/new/ https://reviews.llvm.org/D78374

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. We can assume that size of a file does not change by external (to the analyzed program) events so the **EOF** state can not disappear. (Is this true for `stdin`, or it has never an **EOF**?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. Adding a state chart may help something but does not tell the full picture because conditions on the transitions are not trivial to indicate and follow. And the state chart is too complex to draw it in ASCII-art format. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. The indeterminate position is mentioned only at fread and fwrite. I do not know if it is reasonable to make the indeterminate position in other cases. Th indeterminate position is separate from error flags because of "clearerr" that clears the error flag but not the

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. The problem with small changes is that still we (at least I who writes the code) should know the final goal (and this can be hard if I have multiple not related problems to work on and everything goes forward by little pieces spread out in a long time). (Also the

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added a comment. In D78374#2033826 , @balazske wrote: > I think this is a expected way of how it works, failure of a stream operation > does not necessarily depend on result of a previous operation. So any > operation can fail or not,

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske marked 2 inline comments as done. balazske added a comment. The intent is to model the fread-fwrite function failure by returning the error value and set the stream into error state. The error state is a composite of **ferror** and **feof**. The questions are now, at what case do these

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. I think this is a expected way of how it works, failure of a stream operation does not necessarily depend on result of a previous operation. So any operation can fail or not, independent of the previous error state (the "ferror" may happen because a temporary disk

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus requested changes to this revision. Szelethus added a comment. This revision now requires changes to proceed. I'm sorry for the late review -- please know that this isn't the first time me taking a look, this is a complex issue. I find navigating your phabricator comments a bit

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske updated this revision to Diff 263447. balazske added a comment. Added state split before fread to make warning for error state possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78374/new/ https://reviews.llvm.org/D78374 Files:

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. I am not sure if this FEOF thing is such a big problem. The modeling is not exact but practically this does not cause big problems and still better than before this patch. The problem is, if a `fread` is made and the stream is already in **EOF** state the (modeled)

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. The "indeterminate file position" case is now handled separately and is always error. The current implementation looks to be wrong because the indeterminate position is set at `fread` when a **FEOF** error is produced. This can be solved probably only with more state

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + balazske wrote: > balazske wrote: > > Szelethus wrote: > > > balazske wrote: > > > > Szelethus wrote: > > > > >

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske marked 2 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + Szelethus wrote: > balazske wrote: > > Szelethus wrote: > > > Will that

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:152-158 /// Get the value of the stream argument out of the passed call event. /// The call should contain a function that is described by Desc. SVal getStreamArg(const

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478 + + // FIXME: Check for stream in EOF state? + balazske wrote: > Szelethus wrote: > > Will that be the next patch? :D Eager to see it! > It may be too big

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske updated this revision to Diff 262058. balazske marked 2 inline comments as done. balazske added a comment. Small review related fixes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78374/new/ https://reviews.llvm.org/D78374 Files:

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske marked 3 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:207-210 + {{"fread", 4}, + {::preFread, +std::bind(::evalFreadFwrite, _1, _2, _3, _4, + ErrorFEof |

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

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 , @balazske wrote: > Finally I had to make the decision to remove the `ErrorKindTy` enum and

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske updated this revision to Diff 259491. balazske added a comment. - Stream error is stored in separate boolean flags instead of enum. - Removed `PossibleErrors`. - Added "FilePositionIndeterminate". - Updated fread, fwrite, fseek, clearerr. - Added tests. Repository: rG LLVM Github

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske added a comment. 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

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske marked 2 inline comments as done. balazske added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222 {StreamState::FEof, StreamState::FError, StreamState::NoError}}}, - {{"ftell", 1}, {::preDefault, nullptr, 0, {}}}, +

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus requested changes to this revision. Szelethus added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:152-158 /// Get the value of the stream argument out of the passed call event. /// The

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Szelethus added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222 {StreamState::FEof, StreamState::FError, StreamState::NoError}}}, - {{"ftell", 1}, {::preDefault, nullptr, 0, {}}}, + // Note: ftell sets errno only. +

[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

balazske created this revision. Herald added subscribers: cfe-commits, ASDenysPetrov, 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. Functions