[Analyzer][StreamChecker] Use BugType::SuppressOnSink at resource leak report.

[balazske]

Summary

Use the built-in functionality BugType::SuppressOnSink
instead of a manual solution in StreamChecker.

[Szelethus]

LGTM! Though, the test file change is interesting. You could add a test that behaves differently from the previous implementation:

{
  FILE *f = fopen(...);
} // leak here

10 / 0; // sink

Unless this SuppressOnSink behaves differently on sink error nodes -- I honestly don't know :^)

clang/test/Analysis/stream.c

257–269

Why did this change? Is there a sink in the return branch?

[balazske]

clang/test/Analysis/stream.c

257–269

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

[NoQ]

Before i forget again: commit messages (and, therefore, review titles) are traditionally written in imperative mood, i.e. "Using" -> "Use" as if you ask git to change something in the project.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

207–208

Pls add a comment about what "true" means so that was easier to read, i.e. /*SuppressOnSink =*/ true.

clang/test/Analysis/stream.c

257–269

The shortest should still be found first. I strongly suggest debugging this. Looks like a bug in suppress-on-sink.

[balazske]

clang/test/Analysis/stream.c

257–269

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

[Szelethus]

clang/test/Analysis/stream.c

257–269

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?

[Szelethus]

clang/test/Analysis/stream.c

257–269

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

[balazske]

clang/test/Analysis/stream.c

257–269

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

[NoQ]

clang/test/Analysis/stream.c

257–269

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.

[balazske]

clang/test/Analysis/stream.c

257–269

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

[NoQ]

clang/test/Analysis/stream.c

257–269

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

[balazske]

The problems with bug reporting should be fixed in another patch.

clang/test/Analysis/stream.c

257–269

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?

[balazske]

Fixed commit message and added FIXME about bug report location problem.

[Szelethus]

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.

clang/test/Analysis/stream.c

257–269

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.

[Szelethus]

Unless @NoQ has anything else to add :)

[balazske]

clang/test/Analysis/stream.c

257–269

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.

[Szelethus]

clang/test/Analysis/stream.c

257–269

Oh, yup, I misspoke. I meant -analyzer-output=text-minimal. The actual default has always been a mess, as discussed in D76510.

Now that this came up, btw, I do remember difference in output when I set it to plist/left it on default.

[NoQ]

In D83120#2152919, @Szelethus wrote:

Unless @NoQ has anything else to add :)

@balazske seems innocent indeed! :) :) :)

[balazske]

clang/test/Analysis/stream.c

257–269

One attempt to solve the problem:
https://reviews.llvm.org/D83961

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

class StreamChecker : public Checker<check::PreCall, eval::Call,
   BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
   BugType BT_UseAfterOpenFailed{this, "Invalid stream",
                                 "Stream handling error"};
   BugType BT_IndeterminatePosition{this, "Invalid stream state",
                                    "Stream handling error"};
   BugType BT_IllegalWhence{this, "Illegal whence argument",
                            "Stream handling error"};
   BugType BT_StreamEof{this, "Stream already in EOF", "Stream handling error"};
-  BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error"};
+  BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
+                          /*SuppressOnSink =*/true};
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                      const InvalidatedSymbols &Escaped,
                                      const CallEvent *Call,
if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
     return;
   }
   C.addTransition(State);
 }
 
 ExplodedNode *
 StreamChecker::reportLeaks(const SmallVector<SymbolRef, 2> &LeakedSyms,
                            CheckerContext &C, ExplodedNode *Pred) const {
-  // Do not warn for non-closed stream at program exit.
-  // FIXME: Use BugType::SuppressOnSink instead.
-  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
-    return Pred;
-
   ExplodedNode *Err = C.generateNonFatalErrorNode(C.getState(), Pred);
   if (!Err)
     return Pred;
 
   for (SymbolRef LeakSym : LeakedSyms) {
     // Resource leaks can result in multiple warning that describe the same kind
     // of programming error:
     //  void f() {

clang/test/Analysis/stream.c

void check_leak_noreturn_1() {
   if (!F1)
     return;
   if (Test == 1) {
     handle_error(); // no warning
   }
   rewind(F1);
 } // expected-warning {{Opened stream never closed. Potential resource leak}}
 
 // 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) {
-    return; // expected-warning {{Opened stream never closed. Potential resource leak}}
+    return; // no warning
   }
   rewind(F1);
-} // no warning
+} // expected-warning {{Opened stream never closed. Potential resource leak}}
+// FIXME: This warning should be placed at the `return` above.
+// See https://reviews.llvm.org/D83120 about details.