[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG59878ec8092b: [analyzer] Add path notes to 
FuchsiaHandleCheck. (authored by xazax.hun).

Changed prior to commit:
  https://reviews.llvm.org/D70725?vs=231996&id=234952#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp

Index: clang/test/Analysis/fuchsia_handle.cpp
===
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -analyzer-output=text \
+// RUN: -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 typedef int zx_status_t;
@@ -116,33 +117,76 @@
 
 void checkLeak01(int tag) {
   zx_handle_t sa, sb;
-  if (zx_channel_create(0, &sa, &sb))
-return;
+  if (zx_channel_create(0, &sa, &sb)) // expected-note{{Handle allocated here}}
+return;   // expected-note@-1 {{Assuming the condition is false}}
+  // expected-note@-2 {{Taking false branch}}
   use1(&sa);
-  if (tag)
+  if (tag) // expected-note {{Assuming 'tag' is 0}}
 zx_handle_close(sa);
+  // expected-note@-2 {{Taking false branch}}
   use2(sb); // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
+  zx_handle_close(sb);
+}
+
+void checkReportLeakOnOnePath(int tag) {
+  zx_handle_t sa, sb;
+  if (zx_channel_create(0, &sa, &sb)) // expected-note {{Handle allocated here}}
+return;   // expected-note@-1 {{Assuming the condition is false}}
+  // expected-note@-2 {{Taking false branch}}
   zx_handle_close(sb);
+  switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} 
+case 0:
+  use2(sa);
+  return;
+case 1:
+  use2(sa);
+  return;
+case 2:
+  use2(sa);
+  return;
+case 3:
+  use2(sa);
+  return;
+case 4:
+  use2(sa);
+  return;
+default:
+  use2(sa);
+  return; // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
+  }
 }
 
 void checkDoubleRelease01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  if (tag)
-zx_handle_close(sa);
+  // expected-note@-1 {{Handle allocated here}}
+  if (tag) // expected-note {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
+  // expected-note@-2 {{Taking true branch}}
   zx_handle_close(sa); // expected-warning {{Releasing a previously released handle}}
+  // expected-note@-1 {{Releasing a previously released handle}}
   zx_handle_close(sb);
 }
 
 void checkUseAfterFree01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  // expected-note@-2 {{Handle allocated here}}
+  // expected-note@+2 {{Taking true branch}}
+  // expected-note@+1 {{Taking false branch}}
   if (tag) {
-zx_handle_close(sa);
+// expected-note@-1 {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
 use1(&sa); // expected-warning {{Using a previously released handle}}
+// expected-note@-1 {{Using a previously released handle}}
   }
-  zx_handle_close(sb);
+  // expected-note@-6 {{Assuming 'tag' is 0}}
+  zx_handle_close(sb); // expected-note {{Handle released here}}
   use2(sb); // expected-warning {{Using a previously released handle}}
+  // expected-note@-1 {{Using a previously released handle}}
 }
 
 void checkMemberOperatorIndices() {
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -199,6 +199,28 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(HStateMap, SymbolRef, HandleState)
 
+static const ExplodedNode *getAcquireSite(const ExplodedNode *N, SymbolRef Sym,
+  CheckerContext &Ctx) {
+  ProgramStateRef State = N->getState();
+  // When bug type is handle leak, exploded node N does not have state info for
+  // leaking handle. Get the predecessor of N instead.
+  if (!State->get(Sym))
+N = N->getFirstPred();
+
+  const ExplodedNode *Pred = N;
+  while (N) {
+State = N->getState();
+if (!State->get(Sym)) {
+  const HandleState *HState = Pred->getState()->get(Sym);
+  if (HState && (HState->isAllocated() || HState->

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thanks for the reviews!


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Artem Dergachev via Phabricator via cfe-commits]

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

Looks great, thanks!~


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:307
 
+  std::vector> notes;
   SymbolRef ResultSymbol = nullptr;

I will capitalize this name.


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 231996.
xazax.hun added a comment.

- Only add note tag if we have actual notes.


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

https://reviews.llvm.org/D70725

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp

Index: clang/test/Analysis/fuchsia_handle.cpp
===
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -analyzer-output=text \
+// RUN: -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 typedef int zx_status_t;
@@ -101,31 +102,73 @@
 void checkLeak01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
   use1(&sa);
-  if (tag)
+  if (tag) // expected-note {{Assuming 'tag' is 0}}
 zx_handle_close(sa);
+  // expected-note@-2 {{Taking false branch}}
   use2(sb); // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
   zx_handle_close(sb);
 }
 
+void checkReportLeakOnOnePath(int tag) {
+  zx_handle_t sa, sb;
+  zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  zx_handle_close(sb);
+  switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} 
+case 0:
+  use2(sa);
+  return;
+case 1:
+  use2(sa);
+  return;
+case 2:
+  use2(sa);
+  return;
+case 3:
+  use2(sa);
+  return;
+case 4:
+  use2(sa);
+  return;
+default:
+  use2(sa);
+  return; // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
+  }
+}
+
 void checkDoubleRelease01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  if (tag)
-zx_handle_close(sa);
+  // expected-note@-1 {{Handle allocated here}}
+  if (tag) // expected-note {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
+  // expected-note@-2 {{Taking true branch}}
   zx_handle_close(sa); // expected-warning {{Releasing a previously released handle}}
+  // expected-note@-1 {{Releasing a previously released handle}}
   zx_handle_close(sb);
 }
 
 void checkUseAfterFree01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  // expected-note@-2 {{Handle allocated here}}
+  // expected-note@+2 {{Taking true branch}}
+  // expected-note@+1 {{Taking false branch}}
   if (tag) {
-zx_handle_close(sa);
+// expected-note@-1 {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
 use1(&sa); // expected-warning {{Using a previously released handle}}
+// expected-note@-1 {{Using a previously released handle}}
   }
-  zx_handle_close(sb);
+  // expected-note@-6 {{Assuming 'tag' is 0}}
+  zx_handle_close(sb); // expected-note {{Handle released here}}
   use2(sb); // expected-warning {{Using a previously released handle}}
+  // expected-note@-1 {{Using a previously released handle}}
 }
 
 void checkMemberOperatorIndices() {
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -170,8 +170,8 @@
   /*SuppressOnSink=*/true};
   BugType DoubleReleaseBugType{this, "Fuchsia handle double release",
"Fuchsia Handle Error"};
-  BugType UseAfterFreeBugType{this, "Fuchsia handle use after release",
-  "Fuchsia Handle Error"};
+  BugType UseAfterRelease{this, "Fuchsia handle use after release",
+  "Fuchsia Handle Error"};
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -204,6 +204,28 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(HStateMap, SymbolRef, HandleState)
 
+static const ExplodedNode *getAcquireSite(const ExplodedNode *N, SymbolRef Sym,
+  CheckerContext &Ctx) {
+  ProgramStateRef State = N->getState();
+  // When bug type is handle leak, exploded node N does not have state info for
+  // leaking handle. Get the predecessor of N instead.
+  if (!State->get(Sym))
+N = N->getFirstPred();
+
+  const ExplodedNode *Pred = N;
+  while (N) {
+State = N->getState();
+if (!State->get(Sym)) {
+  const HandleState *HState = Pred->getState()->get(Sym);
+  if (HState && (HState->isAllocated() || HState->maybeAllocated()))
+return N;
+}
+Pred = N;
+   

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D70725#1767942 , @NoQ wrote:

> Mmm, right, i guess you should also skip adding the tag if the notes array is 
> empty.
>
> There might be more complicated use-cases (especially in `ExprEngine` itself) 
> where you may end up adding a tag even if the state doesn't change. For 
> instance, when changing an Environment value from absent to `UnknownVal`, the 
> state doesn't get actually updated. Or making range assumptions over 
> `UnknownVal`. But i'd argue that we do indeed want the note tag in such cases 
> because it is still worth annotating the `ExplodedGraph` with an explanation 
> of what's happening. Eg., a note "Assuming a condition is false" makes sense 
> even if the condition is an `UnknownVal` and no actual constraints are added.
>
> Another example: in a recent `StreamChecker` review we've been talking about 
> a peculiar stream function that by design closes a file descriptor if it's 
> open but does nothing if the descriptor is already closed. I believe this 
> event deserves a note "The descriptor remains closed" (possibly prunable) 
> even though there is no change in the state. This is something we couldn't do 
> with our usual visitor-based approach which involves observing changes in the 
> state (we technically could, via pattern-matching over the program point, but 
> that'd directly duplicate a lot more of checker logic than usual).


I see, that makes sense. I updated the code. I was hoping for a low hanging 
fruit to make this more user friendly :)


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Mmm, right, i guess you should also skip adding the tag if the notes array is 
empty.

There might be more complicated use-cases (especially in `ExprEngine` itself) 
where you may end up adding a tag even if the state doesn't change. For 
instance, when changing an Environment value from absent to `UnknownVal`, the 
state doesn't get actually updated. Or making range assumptions over 
`UnknownVal`. But i'd argue that we do indeed want the note tag in such cases 
because it is still worth annotating the `ExplodedGraph` with an explanation of 
what's happening. Eg., a note "Assuming a condition is false" makes sense even 
if the condition is an `UnknownVal` and no actual constraints are added.

Another example: in a recent `StreamChecker` review we've been talking about a 
peculiar stream function that by design closes a file descriptor if it's open 
but does nothing if the descriptor is already closed. I believe this event 
deserves a note "The descriptor remains closed" (possibly prunable) even though 
there is no change in the state. This is something we couldn't do with our 
usual visitor-based approach which involves observing changes in the state (we 
technically could, via pattern-matching over the program point, but that'd 
directly duplicate a lot more of checker logic than usual).


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In D70725#1767673 , @NoQ wrote:

> In D70725#1767643 , @Charusso wrote:
>
> > In D70725#1767579 , @xazax.hun 
> > wrote:
> >
> > > Just a small side note. If the state was same as the previous we do not 
> > > end up creating a new ExplodedNode right? Is this also the case when we 
> > > add a NoteTag?
> >
> >
> > It only happens for evaluation when you cannot evaluate something. Other 
> > than that `Pre/PostCall` working fine to add a node with the `NoteTag`.
>
>
> Tag pointers are part of the `ProgramPoint`'s identity, which in turn are 
> part of the `ExplodedNode`'s identity. If you make a new note tag and 
> transition into it for the first time, the new node is guaranteed to be fresh.


This is what I suspected. I wonder if all the checks end up using NoteTags will 
the increased number of nodes will ever be a concern? Maybe if is the same as 
the previous we should just omit the note tag?

For example, my checker before the note tag would only generate new nodes if 
there was an actual change to the state. After this change it probably ends up 
adding new nodes with empty note tags all the time.  This could be bad both for 
performance and debugging. What do you think?


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 231989.
xazax.hun marked 5 inline comments as done.
xazax.hun added a comment.

- Address review comments.


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

https://reviews.llvm.org/D70725

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp

Index: clang/test/Analysis/fuchsia_handle.cpp
===
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -analyzer-output=text \
+// RUN: -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 typedef int zx_status_t;
@@ -101,31 +102,73 @@
 void checkLeak01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
   use1(&sa);
-  if (tag)
+  if (tag) // expected-note {{Assuming 'tag' is 0}}
 zx_handle_close(sa);
+  // expected-note@-2 {{Taking false branch}}
   use2(sb); // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
   zx_handle_close(sb);
 }
 
+void checkReportLeakOnOnePath(int tag) {
+  zx_handle_t sa, sb;
+  zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  zx_handle_close(sb);
+  switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} 
+case 0:
+  use2(sa);
+  return;
+case 1:
+  use2(sa);
+  return;
+case 2:
+  use2(sa);
+  return;
+case 3:
+  use2(sa);
+  return;
+case 4:
+  use2(sa);
+  return;
+default:
+  use2(sa);
+  return; // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
+  }
+}
+
 void checkDoubleRelease01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  if (tag)
-zx_handle_close(sa);
+  // expected-note@-1 {{Handle allocated here}}
+  if (tag) // expected-note {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
+  // expected-note@-2 {{Taking true branch}}
   zx_handle_close(sa); // expected-warning {{Releasing a previously released handle}}
+  // expected-note@-1 {{Releasing a previously released handle}}
   zx_handle_close(sb);
 }
 
 void checkUseAfterFree01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  // expected-note@-2 {{Handle allocated here}}
+  // expected-note@+2 {{Taking true branch}}
+  // expected-note@+1 {{Taking false branch}}
   if (tag) {
-zx_handle_close(sa);
+// expected-note@-1 {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
 use1(&sa); // expected-warning {{Using a previously released handle}}
+// expected-note@-1 {{Using a previously released handle}}
   }
-  zx_handle_close(sb);
+  // expected-note@-6 {{Assuming 'tag' is 0}}
+  zx_handle_close(sb); // expected-note {{Handle released here}}
   use2(sb); // expected-warning {{Using a previously released handle}}
+  // expected-note@-1 {{Using a previously released handle}}
 }
 
 void checkMemberOperatorIndices() {
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -170,7 +170,7 @@
   /*SuppressOnSink=*/true};
   BugType DoubleReleaseBugType{this, "Fuchsia handle double release",
"Fuchsia Handle Error"};
-  BugType UseAfterFreeBugType{this, "Fuchsia handle use after release",
+  BugType UseAfterRelease{this, "Fuchsia handle use after release",
   "Fuchsia Handle Error"};
 
 public:
@@ -204,6 +204,28 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(HStateMap, SymbolRef, HandleState)
 
+static const ExplodedNode *getAcquireSite(const ExplodedNode *N, SymbolRef Sym,
+  CheckerContext &Ctx) {
+  ProgramStateRef State = N->getState();
+  // When bug type is handle leak, exploded node N does not have state info for
+  // leaking handle. Get the predecessor of N instead.
+  if (!State->get(Sym))
+N = N->getFirstPred();
+
+  const ExplodedNode *Pred = N;
+  while (N) {
+State = N->getState();
+if (!State->get(Sym)) {
+  const HandleState *HState = Pred->getState()->get(Sym);
+  if (HState && (HState->isAllocated() || HState->maybeAllocated()))
+return N;
+}
+Pred = N;
+N = N->getFirstPred();
+  }
+  return nullptr;
+}
+
 /// Returns the symbols extracted from the

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:363-366
+if (&BR.getBugType() != &UseAfterRelease &&
+&BR.getBugType() != &LeakBugType &&
+&BR.getBugType() != &DoubleReleaseBugType)
+  return "";

NoQ wrote:
> Ugh, it sucks that we have to do this by hand.
> 
> Why would the symbol be interesting if the bugtype is wrong? You imagine 
> something like division by a zero handle? Do we really mind the updates to 
> the handle highlighted in this case? I guess we do.
> 
> Maybe we should make "note tag tags" to avoid users writing this by hand. 
> Like, note tags are tagged with note tag tags and the report is also tagged 
> with a note tag tag and the tag is invoked only if the tag tag on the report 
> matches the tag tag on the tag. Setting a tag tag on the report will be 
> equivalent to calling an "addVisitor" on the report in the visitor API, which 
> was actually a good part of the visitor API.
> 
> Just thinking out loud, please ignore. This code is a nice example to 
> generalize from.
I totally agree :) But if we do something like this please do not call the tag 
tag a tag. It will confuse people. :D



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:369
+  std::string text = note(BR);
+  if (!text.empty())
+return text;

NoQ wrote:
> Another option here is to concatenate the notes if there are indeed two 
> interesting handles at once: `Handle is allocated through 2nd parameter; 
> Handle is released through 3rd parameter". In this checker it probably never 
> happens, as every report is about exactly one handle (is it true for leak 
> reports though?).
I think this is true for leaks as well, as we will generate a new report for 
each of the symbols.


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yay, it actually works!

I only have minor comments and ignorable hand-waving now.

In D70725#1767643 , @Charusso wrote:

> In D70725#1767579 , @xazax.hun wrote:
>
> > Just a small side note. If the state was same as the previous we do not end 
> > up creating a new ExplodedNode right? Is this also the case when we add a 
> > NoteTag?
>
>
> It only happens for evaluation when you cannot evaluate something. Other than 
> that `Pre/PostCall` working fine to add a node with the `NoteTag`.


Tag pointers are part of the `ProgramPoint`'s identity, which in turn are part 
of the `ExplodedNode`'s identity. If you make a new note tag and transition 
into it for the first time, the new node is guaranteed to be fresh.




Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:215
+  if (!State->get(Sym)) {
+N = N->pred_empty() ? nullptr : *(N->pred_begin());
+  }

`N = N->getFirstPred()`.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:224
+  if (HState && (HState->isAllocated() || HState->maybeAllocated()))
+AcquireNode = N;
+  break;

`return N;`? (eliminates a variable)



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:228
+Pred = N;
+N = N->pred_empty() ? nullptr : *(N->pred_begin());
+  }

`N = N->getFirstPred()`.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:363-366
+if (&BR.getBugType() != &UseAfterRelease &&
+&BR.getBugType() != &LeakBugType &&
+&BR.getBugType() != &DoubleReleaseBugType)
+  return "";

Ugh, it sucks that we have to do this by hand.

Why would the symbol be interesting if the bugtype is wrong? You imagine 
something like division by a zero handle? Do we really mind the updates to the 
handle highlighted in this case? I guess we do.

Maybe we should make "note tag tags" to avoid users writing this by hand. Like, 
note tags are tagged with note tag tags and the report is also tagged with a 
note tag tag and the tag is invoked only if the tag tag on the report matches 
the tag tag on the tag. Setting a tag tag on the report will be equivalent to 
calling an "addVisitor" on the report in the visitor API, which was actually a 
good part of the visitor API.

Just thinking out loud, please ignore. This code is a nice example to 
generalize from.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:369
+  std::string text = note(BR);
+  if (!text.empty())
+return text;

Another option here is to concatenate the notes if there are indeed two 
interesting handles at once: `Handle is allocated through 2nd parameter; Handle 
is released through 3rd parameter". In this checker it probably never happens, 
as every report is about exactly one handle (is it true for leak reports 
though?).


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D70725#1767579 , @xazax.hun wrote:

> Just a small side note. If the state was same as the previous we do not end 
> up creating a new ExplodedNode right? Is this also the case when we add a 
> NoteTag?


It only happens for evaluation when you cannot evaluate something. Other than 
that `Pre/PostCall` working fine to add a node with the `NoteTag`.


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

You were right, the new API looks cleaner even in this case :)


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:362
   }
-  C.addTransition(State);
+  const NoteTag *T = C.getNoteTag([this, notes](BugReport &BR) -> std::string {
+if (&BR.getBugType() != &UseAfterRelease &&

Hmm, so we have C++14 now and I can do a move capture right? Will update in the 
next iteration.


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Just a small side note. If the state was same as the previous we do not end up 
creating a new ExplodedNode right? Is this also the case when we add a NoteTag?


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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 231955.
xazax.hun added a comment.

- Use the new notes API for path notes.


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

https://reviews.llvm.org/D70725

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp

Index: clang/test/Analysis/fuchsia_handle.cpp
===
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -analyzer-output=text \
+// RUN: -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 typedef int zx_status_t;
@@ -101,31 +102,73 @@
 void checkLeak01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
   use1(&sa);
-  if (tag)
+  if (tag) // expected-note {{Assuming 'tag' is 0}}
 zx_handle_close(sa);
+  // expected-note@-2 {{Taking false branch}}
   use2(sb); // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
   zx_handle_close(sb);
 }
 
+void checkReportLeakOnOnePath(int tag) {
+  zx_handle_t sa, sb;
+  zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  zx_handle_close(sb);
+  switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} 
+case 0:
+  use2(sa);
+  return;
+case 1:
+  use2(sa);
+  return;
+case 2:
+  use2(sa);
+  return;
+case 3:
+  use2(sa);
+  return;
+case 4:
+  use2(sa);
+  return;
+default:
+  use2(sa);
+  return; // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
+  }
+}
+
 void checkDoubleRelease01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  if (tag)
-zx_handle_close(sa);
+  // expected-note@-1 {{Handle allocated here}}
+  if (tag) // expected-note {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
+  // expected-note@-2 {{Taking true branch}}
   zx_handle_close(sa); // expected-warning {{Releasing a previously released handle}}
+  // expected-note@-1 {{Releasing a previously released handle}}
   zx_handle_close(sb);
 }
 
 void checkUseAfterFree01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  // expected-note@-2 {{Handle allocated here}}
+  // expected-note@+2 {{Taking true branch}}
+  // expected-note@+1 {{Taking false branch}}
   if (tag) {
-zx_handle_close(sa);
+// expected-note@-1 {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
 use1(&sa); // expected-warning {{Using a previously released handle}}
+// expected-note@-1 {{Using a previously released handle}}
   }
-  zx_handle_close(sb);
+  // expected-note@-6 {{Assuming 'tag' is 0}}
+  zx_handle_close(sb); // expected-note {{Handle released here}}
   use2(sb); // expected-warning {{Using a previously released handle}}
+  // expected-note@-1 {{Using a previously released handle}}
 }
 
 void checkMemberOperatorIndices() {
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -170,7 +170,7 @@
   /*SuppressOnSink=*/true};
   BugType DoubleReleaseBugType{this, "Fuchsia handle double release",
"Fuchsia Handle Error"};
-  BugType UseAfterFreeBugType{this, "Fuchsia handle use after release",
+  BugType UseAfterRelease{this, "Fuchsia handle use after release",
   "Fuchsia Handle Error"};
 
 public:
@@ -204,6 +204,32 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(HStateMap, SymbolRef, HandleState)
 
+static const ExplodedNode *getAcquireSite(const ExplodedNode *N, SymbolRef Sym,
+  CheckerContext &Ctx) {
+  const ExplodedNode *AcquireNode = N;
+
+  ProgramStateRef State = N->getState();
+  // When bug type is handle leak, exploded node N does not have state info for
+  // leaking handle. Get the predecessor of N instead.
+  if (!State->get(Sym)) {
+N = N->pred_empty() ? nullptr : *(N->pred_begin());
+  }
+
+  const ExplodedNode *Pred = N;
+  while (N) {
+State = N->getState();
+if (!State->get(Sym)) {
+  const HandleState *HState = Pred->getState()->get(Sym);
+  if (HState && (HState->isAllocated() || HState->maybeAllocated()))
+AcquireNode = N;
+  break;
+}
+Pred = N;
+N = N->pred_empty() ? nullp

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> `note()`

Of course i mean something like this:

  std::string text = note();
  if (!text.empty())
return text;

(and return something like `""` on the other return path in the inner lambdas).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D70725#1765981 , @xazax.hun wrote:

> the same call might both acquire and release handles (there are such calls in 
> Fuchsia), so we might end up adding more than one note for a call for which 
> we would need to add more than one transitions


Hmm, would this be too functional?:

  std::vector notes;
  for (arg : args) {
if (isAcquired(param)) {
  State = State->set(arg, Acquired);
  notes.push_back([](Report) {
if (Report.isInteresting(arg))
  return "Handle acquired here";
  });
}
if (isReleased(param)) {
  State = State->set(arg, Released);
  notes.push_back([](Report) {
if (Report.isInteresting(arg))
  return "Handle released here";
  });
}
  }
  
  C.addTransition(State, C.getNoteTag(
  // We might as well add a C.getNoteTag() overload
  // to do this automatically.
  [std::move(notes)](Report) {
for (note : notes)
  note();
  }));

Or, well, yeah, chain the nodes together; you anyway have to do this 
occasionally due to clumsiness of the rest of the API.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked 3 inline comments as done.
xazax.hun added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:261
+
+  // TODO: do we want to emit notes for escapes?
+  //   do we want to emit notes for for error checks? I.e. on which branch

NoQ wrote:
> Will we ever emit a report against an escaped symbol? If not, there will 
> never be a report to attach the note to.
I think there might be cases where we go from an escaped symbol to released and 
so on. But you are right, it would not contribute to understanding the actual 
bug report.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:262-263
+  // TODO: do we want to emit notes for escapes?
+  //   do we want to emit notes for for error checks? I.e. on which branch
+  //   do we consider the acquire/release success or fail.
+

NoQ wrote:
> If the function is implemented as an eager state split, having a note is 
> great and easy to implement.
> 
> If it's implemented as a Schrödinger state split, then i think we should 
> still emit a note at the collapse point (especially given that it may happen 
> in a deeper stack frame than the call that produced the symbol). But this 
> makes me recognize the need for note tags in `evalAssume`.
I agree.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:521
+  if (Type.isSuppressOnSink()) {
+const ExplodedNode *AcquireNode = getAcquireSite(ErrorNode, Sym, C);
+if (AcquireNode) {

NoQ wrote:
> I wonder how terrible would it be to allow bug visitors (or note tags, which 
> are just an "API sugar" for bug visitors) to mutate the uniqueing location. 
> Because such information is naturally available in the visitor.
> 
> It most likely won't work because the information is necessary before the 
> visitors are run. But it would have been nice though, so i feel as if we're 
> missing an opportunity here.
On one hand, it would be nice :) On the other hand we could have more than one 
visitor and it would be a hell to debug when the visitors disagree an the 
uniqueing location. If we can come up with an API that is not that easy to 
misuse, I am in favor of a change like that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Sorry, my previous comment might not be clear. The point is, the same call 
might both acquire and release handles (there are such calls in Fuchsia), so we 
might end up adding more than one note for a call for which we would need to 
add more than one transitions.  If you think it is still cleaner that way, I 
could do that as well :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I was thinking about note tags, but since we iterate on the argument/parameter 
pairs and might change the state in each iteration we would need to add a new 
transition after each change. I was wondering if using note tags would worth 
the cost (both in performance due to the additional number of nodes and in code 
complexity due to making transitions more often).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

So... note tags ?




Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:261
+
+  // TODO: do we want to emit notes for escapes?
+  //   do we want to emit notes for for error checks? I.e. on which branch

Will we ever emit a report against an escaped symbol? If not, there will never 
be a report to attach the note to.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:262-263
+  // TODO: do we want to emit notes for escapes?
+  //   do we want to emit notes for for error checks? I.e. on which branch
+  //   do we consider the acquire/release success or fail.
+

If the function is implemented as an eager state split, having a note is great 
and easy to implement.

If it's implemented as a Schrödinger state split, then i think we should still 
emit a note at the collapse point (especially given that it may happen in a 
deeper stack frame than the call that produced the symbol). But this makes me 
recognize the need for note tags in `evalAssume`.



Comment at: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp:521
+  if (Type.isSuppressOnSink()) {
+const ExplodedNode *AcquireNode = getAcquireSite(ErrorNode, Sym, C);
+if (AcquireNode) {

I wonder how terrible would it be to allow bug visitors (or note tags, which 
are just an "API sugar" for bug visitors) to mutate the uniqueing location. 
Because such information is naturally available in the visitor.

It most likely won't work because the information is necessary before the 
visitors are run. But it would have been nice though, so i feel as if we're 
missing an opportunity here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70725



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

[PATCH] D70725: [analyzer] Add path notes to FuchsiaHandleCheck

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: NoQ, haowei.
xazax.hun added a project: clang.
Herald added subscribers: Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.
xazax.hun added a reviewer: Szelethus.
xazax.hun added a parent revision: D70470: [analyzer] Add FuchsiaHandleCheck to 
catch handle leaks, use after frees and double frees.

I also added a missing const to a BugReporter API.

There are some TODOs for potential future updates if we want richer notes 
during reporting.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70725

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/test/Analysis/fuchsia_handle.cpp

Index: clang/test/Analysis/fuchsia_handle.cpp
===
--- clang/test/Analysis/fuchsia_handle.cpp
+++ clang/test/Analysis/fuchsia_handle.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,fuchsia.HandleChecker -analyzer-output=text \
+// RUN: -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 typedef int zx_status_t;
@@ -102,31 +103,73 @@
 void checkLeak01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
   use1(&sa);
-  if (tag)
+  if (tag) // expected-note {{Assuming 'tag' is 0}}
 zx_handle_close(sa);
+  // expected-note@-2 {{Taking false branch}}
   use2(sb); // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
   zx_handle_close(sb);
 }
 
+void checkReportLeakOnOnePath(int tag) {
+  zx_handle_t sa, sb;
+  zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  zx_handle_close(sb);
+  switch(tag) { // expected-note {{Control jumps to the 'default' case at line}} 
+case 0:
+  use2(sa);
+  return;
+case 1:
+  use2(sa);
+  return;
+case 2:
+  use2(sa);
+  return;
+case 3:
+  use2(sa);
+  return;
+case 4:
+  use2(sa);
+  return;
+default:
+  use2(sa);
+  return; // expected-warning {{Potential leak of handle}}
+  // expected-note@-1 {{Potential leak of handle}}
+  }
+}
+
 void checkDoubleRelease01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
-  if (tag)
-zx_handle_close(sa);
+  // expected-note@-1 {{Handle allocated here}}
+  if (tag) // expected-note {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
+  // expected-note@-2 {{Taking true branch}}
   zx_handle_close(sa); // expected-warning {{Releasing a previously released handle}}
+  // expected-note@-1 {{Releasing a previously released handle}}
   zx_handle_close(sb);
 }
 
 void checkUseAfterFree01(int tag) {
   zx_handle_t sa, sb;
   zx_channel_create(0, &sa, &sb);
+  // expected-note@-1 {{Handle allocated here}}
+  // expected-note@-2 {{Handle allocated here}}
+  // expected-note@+2 {{Taking true branch}}
+  // expected-note@+1 {{Taking false branch}}
   if (tag) {
-zx_handle_close(sa);
+// expected-note@-1 {{Assuming 'tag' is not equal to 0}}
+zx_handle_close(sa); // expected-note {{Handle released here}}
 use1(&sa); // expected-warning {{Using a previously released handle}}
+// expected-note@-1 {{Using a previously released handle}}
   }
-  zx_handle_close(sb);
+  // expected-note@-6 {{Assuming 'tag' is 0}}
+  zx_handle_close(sb); // expected-note {{Handle released here}}
   use2(sb); // expected-warning {{Using a previously released handle}}
+  // expected-note@-1 {{Using a previously released handle}}
 }
 
 void checkMemberOperatorIndices() {
Index: clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
@@ -163,6 +163,28 @@
   LLVM_DUMP_METHOD void dump() const { dump(llvm::errs()); }
 };
 
+class HandleBugVisitor : public BugReporterVisitor {
+private:
+  SymbolRef HandleSym;
+
+  static void *getTag() {
+static int Tag = 0;
+return &Tag;
+  }
+
+public:
+  HandleBugVisitor(SymbolRef HandleSym) : HandleSym(HandleSym) {}
+
+  void Profile(llvm::FoldingSetNodeID &ID) const override {
+ID.AddPointer(HandleSym);
+ID.AddPointer(getTag());
+  }
+
+  PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+   BugReporterContext &BRC,
+   PathSensitiveBugReport &BR) override;
+};
+
 class FuchsiaHandleChecker
 : public Checker {
@@ -204,6 +226,71 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(HStateMap, SymbolRef, HandleState)
 
+PathDiagnosticPieceRef HandleBugVisitor::VisitNode(const