[PATCH] D81407: [Analyzer][StreamChecker] Add note tags for file opening.

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

Szelethus added a reviewer: xazax.hun.
Szelethus added a comment.
Herald added a subscriber: rnkovacs.

Alright, I'm sold. How about we add a checker option for it? I don't actually insist, just an idea. @xazax.hun, how has this feature played out?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:394
 
+const ExplodedNode *StreamChecker::getAcquireSite(const ExplodedNode *N,
+                                                  SymbolRef Sym,
----------------
How about `getAcquisitionSite`, and a line of comment:
> Searches for the `ExplodedNode` where the file descriptor was acquired for `Sym`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:398-399
+  ProgramStateRef State = N->getState();
+  // When bug type is resource leak, exploded node N may not have state info
+  // for leaked file descriptor, but predecessor should have it.
+  if (!State->get<StreamMap>(Sym))
----------------
I see what you mean, but I'd phrase this differently, and place it...

>Resource leaks can result in multiple warning that describe the same kind of programming error:
>```
>void f() {
>  FILE *F = fopen("a.txt");
>  if (rand()) // state split
>    return; // warning
>} // warning
>```
>While this isn't necessarily true (leaking the same stream could result from a different kinds of errors), the reduction in redundant reports makes this a worthwhile heuristic.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:961
 
-    C.emitReport(std::make_unique<PathSensitiveBugReport>(
-        BT_ResourceLeak, BT_ResourceLeak.getDescription(), N));
+    ;
+    const ExplodedNode *AcquireNode = getAcquireSite(N, Sym, C);
----------------
...here!


================
Comment at: clang/test/Analysis/stream-note.c:1
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -analyzer-output text -verify %s
+
----------------
`core` package! Also, we don't specify `-analyzer-store region` explicitly, we even wondered whether we should just remove the option altogether. Fun fact, `clang_analyze_cc1` actually expands to an invocation that contains it anyways.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81407