[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9b7c43d341da: [Analyzer][StreamChecker] Report every leak, 
clean up state. (authored by balazske).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -46,3 +46,34 @@
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
 // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+
+void check_note_leak_2(int c) {
+  FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  if (!F1)
+// expected-note@-1 {{'F1' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F1' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+return;
+  FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  if (!F2) {
+// expected-note@-1 {{'F2' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F2' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+fclose(F1);
+return;
+  }
+  if (c)
+// expected-note@-1 {{Assuming 'c' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'c' is not equal to 0}}
+// expected-note@-4 {{Taking true branch}}
+return;
+  // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+  // expected-warning@-3 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-4 {{Opened stream never closed. Potential resource leak}}
+  fclose(F1);
+  fclose(F2);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -337,6 +337,12 @@
   /// to ensure uniform handling.
   void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
 
+  /// Emit resource leak warnings for the given symbols.
+  /// Createn a non-fatal error node for these, and returns it (if any warnings
+  /// were generated). Return value is non-null.
+  ExplodedNode *reportLeaks(const SmallVector &LeakedSyms,
+CheckerContext &C, ExplodedNode *Pred) const;
+
   /// Find the description data of the function called by a call event.
   /// Returns nullptr if no function is recognized.
   const FnDescription *lookupFn(const CallEvent &Call) const {
@@ -956,28 +962,19 @@
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-
-  // TODO: Clean up the state.
-  const StreamMapTy &Map = State->get();
-  for (const auto &I : Map) {
-SymbolRef Sym = I.first;
-const StreamState &SS = I.second;
-if (!SymReaper.isDead(Sym) || !SS.isOpened())
-  continue;
-
-ExplodedNode *N = C.generateErrorNode();
-if (!N)
-  continue;
+ExplodedNode *
+StreamChecker::reportLeaks(const SmallVector &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;
 
-// Do not warn for non-closed stream at program exit.
-ExplodedNode *Pred = C.getPredecessor();
-if (Pred && Pred->getCFGBlock() &&
-Pred->getCFGBlock()->hasNoReturnElement())
-  continue;
+  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() {
@@ -989,8 +986,7 @@
 // from a different kinds of errors), the reduction in redundant reports
 // makes this a worthwhile heuristic.
 // FIXME: Add a checker option to turn this uniqueing feature off.
-
-const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C);
+const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
 assert(StreamOpenNode && "Could not find place of stream opening.");
 PathDiagnosticLocation LocUsedForUniqueing =
 PathDiagnosticLocation::createBegin(
@@ -1000,12 +996,38 @@
 std::unique_ptr R =
 std::make_unique(
 BT_ResourceLeak,
-"Opened stream never closed. Po

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 279133.
balazske added a comment.

Rebase to master.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -46,3 +46,34 @@
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
 // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+
+void check_note_leak_2(int c) {
+  FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  if (!F1)
+// expected-note@-1 {{'F1' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F1' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+return;
+  FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  if (!F2) {
+// expected-note@-1 {{'F2' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F2' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+fclose(F1);
+return;
+  }
+  if (c)
+// expected-note@-1 {{Assuming 'c' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'c' is not equal to 0}}
+// expected-note@-4 {{Taking true branch}}
+return;
+  // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+  // expected-warning@-3 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-4 {{Opened stream never closed. Potential resource leak}}
+  fclose(F1);
+  fclose(F2);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -337,6 +337,12 @@
   /// to ensure uniform handling.
   void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
 
+  /// Emit resource leak warnings for the given symbols.
+  /// Createn a non-fatal error node for these, and returns it (if any warnings
+  /// were generated). Return value is non-null.
+  ExplodedNode *reportLeaks(const SmallVector &LeakedSyms,
+CheckerContext &C, ExplodedNode *Pred) const;
+
   /// Find the description data of the function called by a call event.
   /// Returns nullptr if no function is recognized.
   const FnDescription *lookupFn(const CallEvent &Call) const {
@@ -956,28 +962,19 @@
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-
-  // TODO: Clean up the state.
-  const StreamMapTy &Map = State->get();
-  for (const auto &I : Map) {
-SymbolRef Sym = I.first;
-const StreamState &SS = I.second;
-if (!SymReaper.isDead(Sym) || !SS.isOpened())
-  continue;
-
-ExplodedNode *N = C.generateErrorNode();
-if (!N)
-  continue;
+ExplodedNode *
+StreamChecker::reportLeaks(const SmallVector &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;
 
-// Do not warn for non-closed stream at program exit.
-ExplodedNode *Pred = C.getPredecessor();
-if (Pred && Pred->getCFGBlock() &&
-Pred->getCFGBlock()->hasNoReturnElement())
-  continue;
+  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() {
@@ -989,8 +986,7 @@
 // from a different kinds of errors), the reduction in redundant reports
 // makes this a worthwhile heuristic.
 // FIXME: Add a checker option to turn this uniqueing feature off.
-
-const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C);
+const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
 assert(StreamOpenNode && "Could not find place of stream opening.");
 PathDiagnosticLocation LocUsedForUniqueing =
 PathDiagnosticLocation::createBegin(
@@ -1000,12 +996,38 @@
 std::unique_ptr R =
 std::make_unique(
 BT_ResourceLeak,
-"Opened stream never closed. Potential resource leak.", N,
+"Opened stream never closed. Potential resource leak.", Err

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 279131.
balazske added a comment.

Fixed formatting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -46,3 +46,34 @@
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
 // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+
+void check_note_leak_2(int c) {
+  FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  if (!F1)
+// expected-note@-1 {{'F1' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F1' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+return;
+  FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  if (!F2) {
+// expected-note@-1 {{'F2' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F2' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+fclose(F1);
+return;
+  }
+  if (c)
+// expected-note@-1 {{Assuming 'c' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'c' is not equal to 0}}
+// expected-note@-4 {{Taking true branch}}
+return;
+  // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+  // expected-warning@-3 {{Opened stream never closed. Potential resource leak}}
+  // expected-note@-4 {{Opened stream never closed. Potential resource leak}}
+  fclose(F1);
+  fclose(F2);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -337,6 +337,12 @@
   /// to ensure uniform handling.
   void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
 
+  /// Emit resource leak warnings for the given symbols.
+  /// Createn a non-fatal error node for these, and returns it (if any warnings
+  /// were generated). Return value is non-null.
+  ExplodedNode *reportLeaks(const SmallVector &LeakedSyms,
+CheckerContext &C, ExplodedNode *Pred) const;
+
   /// Find the description data of the function called by a call event.
   /// Returns nullptr if no function is recognized.
   const FnDescription *lookupFn(const CallEvent &Call) const {
@@ -956,28 +962,19 @@
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-
-  // TODO: Clean up the state.
-  const StreamMapTy &Map = State->get();
-  for (const auto &I : Map) {
-SymbolRef Sym = I.first;
-const StreamState &SS = I.second;
-if (!SymReaper.isDead(Sym) || !SS.isOpened())
-  continue;
-
-ExplodedNode *N = C.generateErrorNode();
-if (!N)
-  continue;
+ExplodedNode *
+StreamChecker::reportLeaks(const SmallVector &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;
 
-// Do not warn for non-closed stream at program exit.
-ExplodedNode *Pred = C.getPredecessor();
-if (Pred && Pred->getCFGBlock() &&
-Pred->getCFGBlock()->hasNoReturnElement())
-  continue;
+  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() {
@@ -989,8 +986,7 @@
 // from a different kinds of errors), the reduction in redundant reports
 // makes this a worthwhile heuristic.
 // FIXME: Add a checker option to turn this uniqueing feature off.
-
-const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C);
+const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
 assert(StreamOpenNode && "Could not find place of stream opening.");
 PathDiagnosticLocation LocUsedForUniqueing =
 PathDiagnosticLocation::createBegin(
@@ -1000,12 +996,38 @@
 std::unique_ptr R =
 std::make_unique(
 BT_ResourceLeak,
-"Opened stream never closed. Potential resource leak.", N,
+"Opened stream never closed. Potential resource leak.", Err

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 278755.
balazske added a comment.

Checking notes in test `f_leak_2`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream-note.c

Index: clang/test/Analysis/stream-note.c
===
--- clang/test/Analysis/stream-note.c
+++ clang/test/Analysis/stream-note.c
@@ -46,3 +46,34 @@
 }
 // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
 // expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+
+void check_note_leak_2(int c) {
+  FILE *F1 = fopen("foo1.c", "r"); // expected-note {{Stream opened here}}
+  if (!F1)
+// expected-note@-1 {{'F1' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F1' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+return;
+  FILE *F2 = fopen("foo2.c", "r"); // expected-note {{Stream opened here}}
+  if (!F2) {
+// expected-note@-1 {{'F2' is non-null}}
+// expected-note@-2 {{Taking false branch}}
+// expected-note@-3 {{'F2' is non-null}}
+// expected-note@-4 {{Taking false branch}}
+fclose(F1);
+return;
+  }
+  if(c)
+// expected-note@-1 {{Assuming 'c' is not equal to 0}}
+// expected-note@-2 {{Taking true branch}}
+// expected-note@-3 {{Assuming 'c' is not equal to 0}}
+// expected-note@-4 {{Taking true branch}}
+return;
+// expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
+// expected-note@-2 {{Opened stream never closed. Potential resource leak}}
+// expected-warning@-3 {{Opened stream never closed. Potential resource leak}}
+// expected-note@-4 {{Opened stream never closed. Potential resource leak}}
+  fclose(F1);
+  fclose(F2);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -337,6 +337,12 @@
   /// to ensure uniform handling.
   void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
 
+  /// Emit resource leak warnings for the given symbols.
+  /// Createn a non-fatal error node for these, and returns it (if any warnings
+  /// were generated). Return value is non-null.
+  ExplodedNode *reportLeaks(const SmallVector &LeakedSyms,
+CheckerContext &C, ExplodedNode *Pred) const;
+
   /// Find the description data of the function called by a call event.
   /// Returns nullptr if no function is recognized.
   const FnDescription *lookupFn(const CallEvent &Call) const {
@@ -956,28 +962,19 @@
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-
-  // TODO: Clean up the state.
-  const StreamMapTy &Map = State->get();
-  for (const auto &I : Map) {
-SymbolRef Sym = I.first;
-const StreamState &SS = I.second;
-if (!SymReaper.isDead(Sym) || !SS.isOpened())
-  continue;
-
-ExplodedNode *N = C.generateErrorNode();
-if (!N)
-  continue;
+ExplodedNode *
+StreamChecker::reportLeaks(const SmallVector &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;
 
-// Do not warn for non-closed stream at program exit.
-ExplodedNode *Pred = C.getPredecessor();
-if (Pred && Pred->getCFGBlock() &&
-Pred->getCFGBlock()->hasNoReturnElement())
-  continue;
+  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() {
@@ -989,8 +986,7 @@
 // from a different kinds of errors), the reduction in redundant reports
 // makes this a worthwhile heuristic.
 // FIXME: Add a checker option to turn this uniqueing feature off.
-
-const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C);
+const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
 assert(StreamOpenNode && "Could not find place of stream opening.");
 PathDiagnosticLocation LocUsedForUniqueing =
 PathDiagnosticLocation::createBegin(
@@ -1000,12 +996,38 @@
 std::unique_ptr R =
 std::make_unique(
 BT_ResourceLeak,
-"Opened stream never closed. Potential resource leak.", N,
+"Opened stream never closed. Potent

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

I'd prefer if you moved `f_leak_2` to `stream-notes.c`. Otherwise, LGTM.




Comment at: clang/test/Analysis/stream.c:147-150
+  FILE *p1 = fopen("foo1.c", "r");
+  if (!p1)
+return;
+  FILE *p2 = fopen("foo2.c", "r");

I'd prefer to see notes from D81407 here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske updated this revision to Diff 275311.
balazske added a comment.

Rebase, added a FIXME


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream.c

Index: clang/test/Analysis/stream.c
===
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -134,7 +134,7 @@
   fclose(p);
 }
 
-void f_leak(int c) {
+void f_leak_1(int c) {
   FILE *p = fopen("foo.c", "r");
   if (!p)
 return;
@@ -143,6 +143,23 @@
   fclose(p);
 }
 
+void f_leak_2(int c) {
+  FILE *p1 = fopen("foo1.c", "r");
+  if (!p1)
+return;
+  FILE *p2 = fopen("foo2.c", "r");
+  if (!p2) {
+fclose(p1);
+return;
+  }
+  if (c)
+return;
+  // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
+  // expected-warning@-2 {{Opened stream never closed. Potential resource leak}}
+  fclose(p1);
+  fclose(p2);
+}
+
 FILE *f_null_checked(void) {
   FILE *p = fopen("foo.c", "r");
   if (p)
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -337,6 +337,12 @@
   /// to ensure uniform handling.
   void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
 
+  /// Emit resource leak warnings for the given symbols.
+  /// Createn a non-fatal error node for these, and returns it (if any warnings
+  /// were generated). Return value is non-null.
+  ExplodedNode *reportLeaks(const SmallVector &LeakedSyms,
+CheckerContext &C, ExplodedNode *Pred) const;
+
   /// Find the description data of the function called by a call event.
   /// Returns nullptr if no function is recognized.
   const FnDescription *lookupFn(const CallEvent &Call) const {
@@ -956,28 +962,19 @@
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-
-  // TODO: Clean up the state.
-  const StreamMapTy &Map = State->get();
-  for (const auto &I : Map) {
-SymbolRef Sym = I.first;
-const StreamState &SS = I.second;
-if (!SymReaper.isDead(Sym) || !SS.isOpened())
-  continue;
-
-ExplodedNode *N = C.generateErrorNode();
-if (!N)
-  continue;
+ExplodedNode *
+StreamChecker::reportLeaks(const SmallVector &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;
 
-// Do not warn for non-closed stream at program exit.
-ExplodedNode *Pred = C.getPredecessor();
-if (Pred && Pred->getCFGBlock() &&
-Pred->getCFGBlock()->hasNoReturnElement())
-  continue;
+  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() {
@@ -989,8 +986,7 @@
 // from a different kinds of errors), the reduction in redundant reports
 // makes this a worthwhile heuristic.
 // FIXME: Add a checker option to turn this uniqueing feature off.
-
-const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C);
+const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
 assert(StreamOpenNode && "Could not find place of stream opening.");
 PathDiagnosticLocation LocUsedForUniqueing =
 PathDiagnosticLocation::createBegin(
@@ -1000,12 +996,38 @@
 std::unique_ptr R =
 std::make_unique(
 BT_ResourceLeak,
-"Opened stream never closed. Potential resource leak.", N,
+"Opened stream never closed. Potential resource leak.", Err,
 LocUsedForUniqueing,
 StreamOpenNode->getLocationContext()->getDecl());
-R->markInteresting(Sym);
+R->markInteresting(LeakSym);
 C.emitReport(std::move(R));
   }
+
+  return Err;
+}
+
+void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+
+  llvm::SmallVector LeakedSyms;
+
+  const StreamMapTy &Map = State->get();
+  for (const auto &I : Map) {
+SymbolRef Sym = I.first;
+const StreamState &SS = I.second;
+if (!SymReaper.isDead(Sym))
+  continue;
+if (SS.isOpened())
+  LeakedSyms.push_back(Sym);
+State = State->remove(Sym);
+  }
+
+  ExplodedNode *N = C.getPredecessor();
+  if (!LeakedSyms.em

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked an inline comment as done.
balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:969
+  // Do not warn for non-closed stream at program exit.
+  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
+return Pred;

NoQ wrote:
> balazske wrote:
> > Szelethus wrote:
> > > It is a realistic worry that either the predecessor exploded node or the 
> > > CFGBlock is null? Could we assert this instead?
> > It may be simply more safe to check for it. I do not know if exactly if 
> > these can be null at this place.
> Please use `SuppressOnSink` instead.
That was my plan for the next patch. So I want to leave this as is for now (add 
a FIXME), it should be removed anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:969
+  // Do not warn for non-closed stream at program exit.
+  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
+return Pred;

balazske wrote:
> Szelethus wrote:
> > It is a realistic worry that either the predecessor exploded node or the 
> > CFGBlock is null? Could we assert this instead?
> It may be simply more safe to check for it. I do not know if exactly if these 
> can be null at this place.
Please use `SuppressOnSink` instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

The problem is that `PathDiagnostic::Profile` does not use the uniqueing 
locations. Two `PathDiagnostic` objects with different uniqueing location but 
otherwise same data are counted as "same" and only one is kept 
(`BugReporter::FlushReport`). How to fix this?

- Add uniqueing locations to the profile. May have unexpected effects to other 
checkers.
- Change something in the leak bug reports to make these different. Probably 
the location can be set to the allocation site (but the full path should be 
reported). Or include name of the leaked symbol in the report, but it may not 
exists always.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D82845#2123283 , 
@baloghadamsoftware wrote:

> In D82845#2122984 , @balazske wrote:
>
> > I checked it with simple debug print, the `LeakedSyms` will contain 2 items 
> > with different `StreamOpenNode`. So I think these should be independent 
> > problem reports for the bug reporter.
>
>
> I wonder whether you can report two bugs on the same error node.


I recall that i did something similar in UninitializedObjectChecker when the 
option NotesAsWarnings is true.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D82845#2122984 , @balazske wrote:

> I checked it with simple debug print, the `LeakedSyms` will contain 2 items 
> with different `StreamOpenNode`. So I think these should be independent 
> problem reports for the bug reporter.


I wonder whether you can report two bugs on the same error node.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D82845#2122984 , @balazske wrote:

> I checked it with simple debug print, the `LeakedSyms` will contain 2 items 
> with different `StreamOpenNode`. So I think these should be independent 
> problem reports for the bug reporter.


It would still be beneficial to check the exploded graph to see whether it was 
constructed correctly, find the error nodes, etc. Have you tried modifying the 
warning message? That is considered during uniquing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked an inline comment as done.
balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:969
+  // Do not warn for non-closed stream at program exit.
+  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
+return Pred;

Szelethus wrote:
> It is a realistic worry that either the predecessor exploded node or the 
> CFGBlock is null? Could we assert this instead?
It may be simply more safe to check for it. I do not know if exactly if these 
can be null at this place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

I checked it with simple debug print, the `LeakedSyms` will contain 2 items 
with different `StreamOpenNode`. So I think these should be independent problem 
reports for the bug reporter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D82845#2121940 , @balazske wrote:

> Why does this not work?
>  I get only warning for the first resource leak (in the test `f_leak_2`). How 
> to fix this?


First off, are two errors detected by the checker? You could check this with 
`debug.ViewExplodedGraph` + `clang/tools/analyzer/exploded-graph-rewriter.py`.

I do suspect though that this is a uniqueing issue. Just out of curiosity, what 
would happen is you dump the leaked stream symbol into the warning message? (or 
whatever, just so that two distinct warning messages are emitted)




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:969
+  // Do not warn for non-closed stream at program exit.
+  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
+return Pred;

It is a realistic worry that either the predecessor exploded node or the 
CFGBlock is null? Could we assert this instead?



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:971
-
-ExplodedNode *N = C.generateErrorNode();
-if (!N)

Ah, I see, we made it fatal accidentally.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

Why does this not work?
I get only warning for the first resource leak (in the test `f_leak_2`). How to 
fix this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82845/new/

https://reviews.llvm.org/D82845



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82845: [Analyzer][StreamChecker] Report every leak, clean up state.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, 
gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a project: clang.
balazske added a comment.
balazske added reviewers: baloghadamsoftware, NoQ.
Herald added a subscriber: rnkovacs.

Why does this not work?
I get only warning for the first resource leak (in the test `f_leak_2`). How to 
fix this?


Report resource leaks with non-fatal error.
Report every resource leak.
Stream state is cleaned up at `checkDeadSymbols`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82845

Files:
  clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  clang/test/Analysis/stream.c

Index: clang/test/Analysis/stream.c
===
--- clang/test/Analysis/stream.c
+++ clang/test/Analysis/stream.c
@@ -134,7 +134,7 @@
   fclose(p);
 }
 
-void f_leak(int c) {
+void f_leak_1(int c) {
   FILE *p = fopen("foo.c", "r");
   if (!p)
 return;
@@ -143,6 +143,23 @@
   fclose(p);
 }
 
+void f_leak_2(int c) {
+  FILE *p1 = fopen("foo1.c", "r");
+  if (!p1)
+return;
+  FILE *p2 = fopen("foo2.c", "r");
+  if (!p2) {
+fclose(p1);
+return;
+  }
+  if (c)
+return;
+  // expected-warning@-1 {{Opened stream never closed. Potential resource leak}}
+  // expected-warning@-2 {{Opened stream never closed. Potential resource leak}}
+  fclose(p1);
+  fclose(p2);
+}
+
 FILE *f_null_checked(void) {
   FILE *p = fopen("foo.c", "r");
   if (p)
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -337,6 +337,12 @@
   /// to ensure uniform handling.
   void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const;
 
+  /// Emit resource leak warnings for the given symbols.
+  /// Createn a non-fatal error node for these, and returns it (if any warnings
+  /// were generated). Return value is non-null.
+  ExplodedNode *reportLeaks(const SmallVector &LeakedSyms,
+CheckerContext &C, ExplodedNode *Pred) const;
+
   /// Find the description data of the function called by a call event.
   /// Returns nullptr if no function is recognized.
   const FnDescription *lookupFn(const CallEvent &Call) const {
@@ -956,28 +962,18 @@
   C.addTransition(State);
 }
 
-void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-
-  // TODO: Clean up the state.
-  const StreamMapTy &Map = State->get();
-  for (const auto &I : Map) {
-SymbolRef Sym = I.first;
-const StreamState &SS = I.second;
-if (!SymReaper.isDead(Sym) || !SS.isOpened())
-  continue;
-
-ExplodedNode *N = C.generateErrorNode();
-if (!N)
-  continue;
+ExplodedNode *
+StreamChecker::reportLeaks(const SmallVector &LeakedSyms,
+   CheckerContext &C, ExplodedNode *Pred) const {
+  // Do not warn for non-closed stream at program exit.
+  if (Pred && Pred->getCFGBlock() && Pred->getCFGBlock()->hasNoReturnElement())
+return Pred;
 
-// Do not warn for non-closed stream at program exit.
-ExplodedNode *Pred = C.getPredecessor();
-if (Pred && Pred->getCFGBlock() &&
-Pred->getCFGBlock()->hasNoReturnElement())
-  continue;
+  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() {
@@ -989,8 +985,7 @@
 // from a different kinds of errors), the reduction in redundant reports
 // makes this a worthwhile heuristic.
 // FIXME: Add a checker option to turn this uniqueing feature off.
-
-const ExplodedNode *StreamOpenNode = getAcquisitionSite(N, Sym, C);
+const ExplodedNode *StreamOpenNode = getAcquisitionSite(Err, LeakSym, C);
 assert(StreamOpenNode && "Could not find place of stream opening.");
 PathDiagnosticLocation LocUsedForUniqueing =
 PathDiagnosticLocation::createBegin(
@@ -1000,12 +995,38 @@
 std::unique_ptr R =
 std::make_unique(
 BT_ResourceLeak,
-"Opened stream never closed. Potential resource leak.", N,
+"Opened stream never closed. Potential resource leak.", Err,
 LocUsedForUniqueing,
 StreamOpenNode->getLocationContext()->getDecl());
-R->markInteresting(Sym);
+R->markInteresting(LeakSym);
 C.emitReport(std::move(R));
   }
+
+  return Err;
+}
+
+void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
+  Pr