Szelethus 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) {
----------------
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.
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
