NoQ 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:
> 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^^
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
