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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
