Szelethus added a subscriber: NoQ.
Szelethus added a comment.
I have more questions than actual objections, but here we go! The patch looks
nice overall, we just need to iron a few things out ahead of time.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:107
+ /// This value applies to all error states in ErrorState except FEOF.
+ /// An EOF+indeterminate state is the same as EOF state.
+ bool FilePositionIndeterminate = false;
----------------
What does this mean? "An EOF+indeterminate state is the same as EOF state." I
don't understand the message you want to convey here -- is it that we cannot
have an indeterminate file position indicator if we hit EOF, hence we regard a
stream that is **known** to be EOF to have its file position indicator
determinate?
A good followup question for the uninitiated would be that "Well why is it ever
legal to construct a `StreamState` object that can both have the
`FilePositionIndeterminate` set to true and the `ErrorState` indicate that the
steam is **known** to be in EOF?" Well, the answer is that we may only realize
later that the error state can only be EOF, like in here:
```lang=c++
void f() {
FILE *F = fopen(...);
if (fseek(F, ...)) {
// Could be either EOF, ERROR, and ofc indeterminate
if (eof(F)) {
// This is where we have a seemingly impossible stream state, but its not
a programming error, its a design decision.
}
}
```
This might warrant a bit on explanation either here, or in
`ensureNoFilePositionIndeterminate`. Probably the latter.
With that said, can `SteamState`'s constructor ensure that we do not create a
known to be EOF stream that is indeterminate?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:124
+ const StreamErrorState &ES = ErrorNone,
+ bool FPI = false) {
+ return StreamState{L, Opened, ES, FPI};
----------------
Let's not cheap out on this parameter name :^)
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:191-193
mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
- BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof;
+ BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof,
+ BT_IndeterminatePosition;
----------------
In time, we could create a new checker for each of these bug types, similar to
how D77866 does it. But this is clearly a low prio issue, I'm just talking
aloud.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:306
+ ProgramStateRef
+ ensureNoFilePositionIndeterminate(SVal StreamVal, CheckerContext &C,
+ ProgramStateRef State) const;
----------------
Ooooor `ensureFilePositionDeterminate`? :D
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:839-840
+
+ // If only FEof is possible, report no warning (indeterminate position in
+ // FEof is ignored).
+ if (SS->ErrorState.isFEof())
----------------
I think we shouldn't say its ignored, but rather its impossible. I mean, if we
have reached the end of the file, how could the file position indicator be
indeterminate?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:854
+ }
+ return State; // FIXME: nullptr?
+ }
----------------
Good question. It would make sense... @NoQ, @xazax.hun?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:857-868
+ // FEof and other states are possible.
+ // The path with FEof is the one that can continue.
+ // For this reason a non-fatal error is generated to continue the analysis
+ // with only FEof state set.
+ ExplodedNode *N = C.generateNonFatalErrorNode(State);
+ if (N) {
+ C.emitReport(std::make_unique<PathSensitiveBugReport>(
----------------
Ugh, tough one. I think this just isn't really *the* error to highlight here,
but rather that its bad practice to not check on the stream after an operation.
But I'm willing to accept I might be wrong here.
================
Comment at: clang/test/Analysis/stream-error.c:182
+ } else {
+ fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+ }
----------------
Here the user checked whether F is in eof or in ferror, and its in neither.
Isn't it reasonable to assume then that the stream is fine?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80018/new/
https://reviews.llvm.org/D80018
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
