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@-1 {{'F' is non-null}} // expected-note@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
