[PATCH] D106262: [clang][analyzer] Use generic note tag in alpha.unix.Stream .
Kristóf Umann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 9 08:11:39 PDT 2021
Szelethus added a comment.
Alright, I think we're almost ready to go! I left a few comments, please make sure to mark those done that you feel are properly addressed.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:393-394
+ /// in this sense if a resource leak is detected later.
+ /// At a bug report the "failed operation" is always the last in the path
+ /// (where this note is displayed) and previous such notes are not displayed.
+ const NoteTag *constructFailureNoteTag(CheckerContext &C, SymbolRef StreamSym,
----------------
The main thing to highlight here is that its not only the last failing operation, but more importantly that this operation caused the bug to occur.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:412-413
+ /// the path (if no other failing operation follows).
+ /// This note is inserted into places where something important about
+ /// the last failing operation is discovered.
+ const NoteTag *constructNonFailureNoteTag(CheckerContext &C,
----------------
Same here, mention that its the failed stream operation that **caused** the bug is what we're specifying further.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:518
+ C.addTransition(StateNull,
+ constructFailureNoteTag(C, RetSym, "Stream open fails here"));
}
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:576
+ C.addTransition(StateRetNull, constructFailureNoteTag(
+ C, StreamSym, "Stream reopen fails here"));
}
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:696
// Add transition for the failed state.
Optional<NonLoc> RetVal = makeRetVal(C, CE).castAs<NonLoc>();
assert(RetVal && "Value should be NonLoc.");
----------------
Lets leave a TODO here, before we forget it:
[[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | C'99, pdf page 313, §7.19.8.1.2]], Description of `fread`:
> If a partial element is read, its value is indeterminate.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:726-729
+ "Stream reaches end-of-file or operation fails here"));
else
- C.addTransition(StateFailed);
+ C.addTransition(StateFailed, constructFailureNoteTag(
+ C, StreamSym, "Operation fails here"));
----------------
We can be more specific here. While the standard doesn't explicitly specify that a read failure could result in ferror being set, it does state that the file position indicator will be indeterminate:
[[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | C'99, pdf page 313, §7.19.8.1.2]], Description of `fread`:
> If an error occurs, the resulting value of the file position indicator for the stream is indeterminate.
[[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | C'99, pdf page 313, §7.19.8.2.2]], Description of `fwrite`:
> If an error occurs, the resulting value of the file position indicator for the stream is indeterminate.
Since this is the event to highlight, I'd like to see it mentioned. How about:
> Stream either reaches end-of-file, or fails and has its file position indicator left indeterminate, or the error flag set.
> After this operation fails, the stream either has its file position indicator left indeterminate, or the error flag set.
Same for any other case where indeterminate file positions could occur.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:791
+ constructFailureNoteTag(
+ C, StreamSym, "Operation fails or stream reaches end-of-file here"));
}
----------------
Like here.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:933
BT_UseAfterClose,
- "Stream might be already closed. Causes undefined behaviour.", N));
+ "Stream might be already closed. Causes undefined behaviour.", N);
+ R->markInteresting(Sym);
----------------
Please leave a TODO here, don't fix now.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:969
"File position of the stream might be 'indeterminate' "
"after a failed operation. "
"Can cause undefined behavior.";
----------------
Stating that it happened as a result of a failed operation seems kind of redundant, especially if the `NoteTag` states that as well. Lets leave a TODO here to address this warning message, but leave as-is for now.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:1158
}
\ No newline at end of file
----------------
Lets put one there!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:387-391
+ /// Create a NoteTag to display a note if a later bug report is generated.
+ /// The `BT` should contain all bug types that could be caused by the
+ /// operation at this location.
+ /// If later on the path a problematic event (reported bug) happens with the
+ /// same type, the last place is found with a note tag with that bug type.
----------------
balazske wrote:
> balazske wrote:
> > Szelethus wrote:
> > > How about:
> > >
> > > Create a `NoteTag` describing an stream operation (whether stream opening succeeds or fails, stream reaches EOF, etc).
> > > As not all operations are interesting for all types of stream bugs (the stream being at an indeterminate file position is irrelevant to whether it leaks or not), callers can specify in `BT` for which `BugType`s should this note be displayed for.
> > > Only the `NoteTag` closest to the error location will be added to the bug report.
> > The `NoteTag` is added at a place where a possible future bug is introduced. The bug type indicates which bug is the one that can happen after this event. If this bug is really detected the last NoteTag for this type (ignore other NoteTags with non-matching bug type) contains the relevant information.
> This is the planned comment at `constructNoteTag`:
> /// Create a NoteTag to display a note if a later bug report is generated.
> /// This should be added added at a place where a possible future bug is
> /// introduced. The bug type indicates which bug is the one that can happen
> /// after this event. If this bug is really detected the last NoteTag for
> /// its type (ignore other NoteTags with non-matching bug type) contains the
> /// relevant information (location and text message).
>
Aha, okay, so you need a `NoteTag` that **removes** interesstingness in the case where we found the stream operation that caused the bug report, and one that **does not** remove interesstingness in the case where a stream operation is worth explaining, but is not the cause. Fair enough!
In the function name, you use the word "failure", but state that its not always a failure that the `NoteTag` describes. How about `constructLatestNoteTag`, and `constructNoteTag`? I think that explains what happens in the function (and its comments) better.
================
Comment at: clang/test/Analysis/stream-note.c:51
+void check_note_fopen_fail() {
+ FILE *F = fopen("file", "r"); // expected-note {{Assuming opening the stream fails here}} expected-note {{Assuming pointer value is null}} expected-note {{'F' initialized here}}
+ fclose(F); // expected-warning {{Stream pointer might be NULL}}
----------------
Szelethus wrote:
> I'd prefer an individual line for these `expected-.*` directives. Its down to personal preference, but I find that far easier to read.
I'd like to see this addressed. Lets have a new line for each directive, at least where the 80 column limit is reached.
================
Comment at: clang/test/Analysis/stream-note.c:36-41
- FILE *F = fopen("file", "r"); // expected-note {{Stream opened here}}
+ FILE *F = fopen("file", "r");
if (!F)
// expected-note at -1 {{'F' is non-null}}
// expected-note at -2 {{Taking false branch}}
return;
- F = freopen(0, "w", F); // expected-note {{Stream reopened here}}
----------------
Szelethus wrote:
> Szelethus wrote:
> > I think I preferred this, honestly.
> Hmmm... I've given this some thought, and yes, I the stream misuse can indeed be captured starting from the last `freopen` call. The specialized message for reopen was nice, but I guess no actual information was lost by this patch.
You can mark these done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D106262/new/
https://reviews.llvm.org/D106262
More information about the cfe-commits
mailing list