[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 <https://reviews.llvm.org/D70725#1767643>, @Charusso wrote:

> In D70725#1767579 <https://reviews.llvm.org/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<HStateMap>(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