[PATCH] D34724: [analyzer] Add MagentaHandleChecker for the Magenta kernel

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks for publishing this. It seems that your code is already huge, and you have already added a lot of workarounds for various problems (known and new) in our APIs, while it'd sound great to actually solve these problems and simplify the API to make writing checkers easier (as in http://lists.llvm.org/pipermail/cfe-dev/2017-June/054457.html ). This technical debt seems to slow down any progress on the checkers dramatically - and i'm totally sorry about that.



================
Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:22-49
+//                          mx_channel_write failed
+//                                +---------+
+//                                |         |
+//                                |         |
+//                                |         |      As argument
+//  mx_channel_create succeeded +-+---------v-+    in uninlined   +---------+
+//  mx_channel_read succeeded   |             |    calls          |         |
----------------
I wish all checkers had these :3


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:219
+CallKind
+MagentaHandleChecker::CheckCallSignature(const FunctionDecl *FuncDecl) const {
+  if (!FuncDecl)
----------------
We're trying to use this `CallDescription` thing for this purpose recently.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:297-303
+    const SymbolDerived *ElementSym = dyn_cast<SymbolDerived>(Sym);
+    if (!ElementSym)
+      continue;
+
+    const SymbolRef ParentSymbol = ElementSym->getParentSymbol();
+    if (Escaped.count(ParentSymbol) == 1)
+      EscapedSymbolRef.push_back(Sym);
----------------
Uhm, yet another unobvious boilerplate that shows us that checker API needs to be made a lot easier. Not many checkers need this, but i suspect that `PthreadLockChecker` may suffer from the same problem.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:326-339
+  int64_t ExtVal;
+  if (ExprSVal.getBaseKind() == SVal::NonLocKind &&
+      ExprSVal.getSubKind() == nonloc::ConcreteIntKind) {
+    ExtVal = ExprSVal.castAs<nonloc::ConcreteInt>().getValue().getExtValue();
+  } else if (ExprSVal.getBaseKind() == SVal::LocKind &&
+             ExprSVal.getSubKind() == loc::ConcreteIntKind) {
+    ExtVal = ExprSVal.castAs<loc::ConcreteInt>().getValue().getExtValue();
----------------
This code can be simplified to `return !State->assume(ExprSVal, true);`, which also works for symbolic `SVal`s (eg. as in `CheckSymbolConstraintToNotZero`).


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:373-376
+  for (auto &CurItem : TrackedHandle) {
+    SymbolRef Sym = CurItem.first;
+    // Workaround for zombie symbol issue.
+    bool IsSymDead = SymReaper.maybeDead(Sym);
----------------
This may work as well, yeah.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:456-457
+  // arguments is an acquired handle, treat it as an escaped handle.
+  // This is just a naive approach to reduce the false positive rate, should
+  // be refined later, e.g. through annotation
+  // TODO: Use annotation to make it more accurate.
----------------
This should be fixed by allowing `checkPointerEscape` report non-pointer escapes, or making a separate callback for non-pointer escapes. This huge boilerplate shouldn't be repeated in every checker that needs it.

Annotations are still great though.


================
Comment at: lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp:606-607
+
+  Ctx.addTransition(State);
+  Ctx.addTransition(FailedState);
+
----------------
While this is the easiest thing to do, it halves the analyzer performance every time it happens, exploding the complexity exponentially - because the remaining path would be split up into two paths, which are analyzed independently.

So it is really really rarely a good idea to split the state in the checker.

The alternative approach would be to delay splitting the state now, and only do that when it starts to matter. This is annoying to implement, but this may work. An example of this would be how we handled failed `pthread_mutex_destroy` in http://lists.llvm.org/pipermail/cfe-dev/2017-April/053567.html / https://reviews.llvm.org/D32449 .


https://reviews.llvm.org/D34724