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