Szelethus accepted this revision. Szelethus added a comment. Now that we found the answer to the only lingering question this revision raised, I think you can safely land it while we start looking into fixing this newfound bug. LGTM.
================ 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) { ---------------- balazske wrote: > NoQ wrote: > > balazske wrote: > > > 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). > > Ok, this sounds pretty bad, as if a lot of our lit tests actually have > > warnings misplaced. I.e., we report different bug instances depending on > > the consumer, even within the same analysis! Looks like this entire big > > for-loop in `BugReporter::FlushReport` is potentially dealing with the > > wrong report(?) > > > > Would you have the honor of fixing this mess that you've uncovered? Or i > > can take it up if you're not into it^^ > I still have to look at this bug reporting code to get the details about how > it works. Probably that loop is not bad, only the use of `report` causes the > problem. I discovered that removing lines 2000-2001 in //BugReporter.cpp// > ``` > if (!PDC->shouldGenerateDiagnostics()) > return generateEmptyDiagnosticForReport(R, getSourceManager()); > ``` > fixes the problem at least in this case, maybe this is a good solution? > Wow, great job discovering all this! >I discovered that removing lines 2000-2001 in BugReporter.cpp > > if (!PDC->shouldGenerateDiagnostics()) > return generateEmptyDiagnosticForReport(R, getSourceManager()); >fixes the problem at least in this case, maybe this is a good solution? It shouldn't be, this would create path notes for `-analyzer-output=none`, which is also our default. Also, this shouldn't really have an effect on the bug we uncovered. > 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). This is the issue -- none of this should depend on whether we construct a more detailed diagnostic. >> 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. I really dislike these sorts of (undocumented!) hacks in BugReporter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83120/new/ https://reviews.llvm.org/D83120 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits