[PATCH] D106262: [clang][analyzer] Use generic note tag in alpha.unix.Stream .

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:236-242
+  const BugMessageMap BugMessages = {
+      {&BT_FileNull, "Assuming opening the stream fails here"},
+      {&BT_UseAfterClose, "Stream closed here"},
+      {&BT_UseAfterOpenFailed, "Assuming opening the stream fails here"},
+      {&BT_IndeterminatePosition, "Assuming this stream operation fails"},
+      {&BT_StreamEof, "Assuming stream reaches end-of-file here"},
+      {&BT_ResourceLeak, "Stream opened here"}};
----------------
Szelethus wrote:
> Well this looks odd: for both `BT_StreamEof` AND `BT_IndeterminatePosition` we want to display a `NoteTag` for a failed `fseek`, for one you print "Assuming stream reaches end-of-file here", for the other, "Assuming this stream operation fails". This seems to contradict your comment in the fseek evaluating function:
> 
> ```
> If fseek failed, assume that the file position becomes indeterminate in any case.
> ```
> 
> Also, these `BugTypes` should be responsible for the *error message*, not the `NoteTag` message. I'd prefer if we mapped an enum to these strings (`NoteTagMsgKind`?), pass that as well to `constructNoteTag`, and allow the caller to decide that for which `BugTypes` the `NoteTag` is worth displaying for.
> 
> I think such a 4-argument `constructNoteTag` would capture best what we want here.
It is still unclear how to model the `fseek` (and other functions). (But this is not a problem for this patch.) We can do it according the POSIX or C standard, or just by the experience that a `fseek` may fail with EOF or `ferror` or none of them. The standards are not exact but do not mention that `fseek` should cause the indeterminate flag to be set at all, or that `fseek` can cause `feof` state.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:236-242
+  const BugMessageMap BugMessages = {
+      {&BT_FileNull, "Assuming opening the stream fails here"},
+      {&BT_UseAfterClose, "Stream closed here"},
+      {&BT_UseAfterOpenFailed, "Assuming opening the stream fails here"},
+      {&BT_IndeterminatePosition, "Assuming this stream operation fails"},
+      {&BT_StreamEof, "Assuming stream reaches end-of-file here"},
+      {&BT_ResourceLeak, "Stream opened here"}};
----------------
balazske wrote:
> Szelethus wrote:
> > Well this looks odd: for both `BT_StreamEof` AND `BT_IndeterminatePosition` we want to display a `NoteTag` for a failed `fseek`, for one you print "Assuming stream reaches end-of-file here", for the other, "Assuming this stream operation fails". This seems to contradict your comment in the fseek evaluating function:
> > 
> > ```
> > If fseek failed, assume that the file position becomes indeterminate in any case.
> > ```
> > 
> > Also, these `BugTypes` should be responsible for the *error message*, not the `NoteTag` message. I'd prefer if we mapped an enum to these strings (`NoteTagMsgKind`?), pass that as well to `constructNoteTag`, and allow the caller to decide that for which `BugTypes` the `NoteTag` is worth displaying for.
> > 
> > I think such a 4-argument `constructNoteTag` would capture best what we want here.
> It is still unclear how to model the `fseek` (and other functions). (But this is not a problem for this patch.) We can do it according the POSIX or C standard, or just by the experience that a `fseek` may fail with EOF or `ferror` or none of them. The standards are not exact but do not mention that `fseek` should cause the indeterminate flag to be set at all, or that `fseek` can cause `feof` state.
The message for a NoteTag depends on the bug type and at this state the bug type is sufficient to determine the note message. Because it is possible to add multiple bug types to a NoteTag, passing a custom message to it can be done only by passing a BugType->Message map to the note tag. This may be unnecessary complexity for the current use case.


================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106262/new/

https://reviews.llvm.org/D106262