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) {
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > 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.
> > At me there are no notes shown if clang is run without //-analyzer-output//
> > option (and the mentioned fix is made), only the one warning at the correct
> > location (same as without the fix but at correct place). Passing //none//
> > for this generates an invalid option value error.
> Oh, yup, I misspoke. I meant `-analyzer-output=text-minimal`. The actual
> default has always been a mess, as discussed in D76510.
>
> Now that this came up, btw, I do remember difference in output when I set it
> to plist/left it on default.
One attempt to solve the problem:
https://reviews.llvm.org/D83961
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
