[PATCH] D83120: [Analyzer][StreamChecker] Using BugType::SuppressOnSink at resource leak report.

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

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang/test/Analysis/stream.c:274-284
 // Check that "location uniqueing" works.
 // This results in reporting only one occurence of resource leak for a stream.
 void check_leak_noreturn_2() {
   FILE *F1 = tmpfile();
   if (!F1)
     return;
   if (Test == 1) {
----------------
NoQ wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Szelethus wrote:
> > > > balazske wrote:
> > > > > NoQ wrote:
> > > > > > balazske wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > Why did this change? Is there a sink in the return branch?
> > > > > > > The change is probably because D83115. Because the "uniqueing" one resource leak is reported from the two possible, and the order changes somehow (probably not the shortest is found first).
> > > > > > The shortest should still be found first. I strongly suggest debugging this. Looks like a bug in suppress-on-sink.
> > > > > There is no code that ensures that the shortest path is reported. In this case one equivalence class is created with both bug reports. If `SuppressOnSink` is false the last one is returned from the list, otherwise the first one (`PathSensitiveBugReporter::findReportInEquivalenceClass`), this causes the difference (seems to be unrelated to D83115).
> > > > > There is no code that ensures that the shortest path is reported.
> > > > 
> > > > There absolutely should be -- See the summary of D65379 for more info, CTRL+F "shortest" helps quite a bit as well. For each bug report, we create a bug path (a path in the exploded graph from the root to the sepcific bug reports error node), and sort them by length.
> > > > 
> > > > It all feels super awkward -- `PathSensitiveBugReporter::findReportInEquivalenceClass` picks out a bug report from an equivalence class as you described, but that will only be reported if it is a `BasicBugReport` (as implemented by `PathSensitiveBugReporter::generateDiagnosticForConsumerMap`), otherwise it should go through the graph cutting process etc.
> > > > 
> > > > So at the end of the day, the shortest path should appear still? 
> > > > 
> > > @balazske I spent a lot of my GSoC rewriting some especially miserable code in `BugReporter.cpp`, please hunt me down if you need any help there.
> > Can we say that the one path in this case is shorter than the other? The difference is only at the "taking true/false branch" at the `if` in line 280. Maybe both have equal length. The notes are taken always from the single picked report that is returned from `findReportInEquivalenceClass` and these notes can contain different source locations (reports in a single equivalence class can have different locations, really this makes the difference between them?).  
> > There is no code that ensures that the shortest path is reported.
> 
> We would have been soooooooooooooo screwed if this was so. In fact, grepping for "shortest" in the entire clang sources immediately points you to the right line of code.
> 
> > the last one is returned from the list, otherwise the first one
> 
> The example report is not actually used later for purposes other than extracting information common to all reports in the path. The array of valid reports is used instead, and it's supposed to be sorted.
> 
> > Can we say that the one path in this case is shorter than the other?
> 
> Dump the graph and see for yourself. I expect a call with an argument and an implicit lvalue-to-rvalue conversion of that argument to take a lot more nodes than an empty return statement.
I found the sorting code, it revealed that the problem has other reason: It happens only if //-analyzer-output text// is not passed to clang. It looks like that in this case the path in `PathDiagnostic` is not collected, so `BugReporter::FlushReport` will use the one report instance from the bug report class (that is different if `SuppressOnSink` is set or not).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83120