balazske marked an inline comment as done.
balazske added a comment.
The problems with bug reporting should be fixed in another patch.
================
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:
> > 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?
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
