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