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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
