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

[Haowei Wu via Phabricator via cfe-commits]

haowei updated this revision to Diff 107201.
haowei added a comment.

Thanks for reviewing this patch. I have modified the checker according NoQ's suggestions and refactored some long functions.

> We're trying to use this CallDescription thing for this purpose recently.

I took a look at CallDescption, but it can only be used when a CallEvent object is present, which is not available in evalCall callbacks. So we cannot use it in our checker. BTW, we have been working on a function matching based on annotation attributes to avoid string comparison as much as possible. But the patch is relatively huge so we would like to land this patch first before publishing the annotation attribute stuff.

> 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.

I agree. For example following code is not covered by the workaround I used in the checker:
struct A {

  mx_handle_t data,
  mx_status_t status,

};
escapeFunction(A arg1);

And it would be hard to implement a workaround in the checker to cover this case. I would like to help but I quite new to the Clang and Clang Static Analyzer. Currently I don't know where to start to fix this issue in the static analyzer. If you have any suggestions on how to fix it in the Clang, let me know.

> 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.

When I first worked on this checker, I actually use ProgramState to track return values of syscalls, which is very similar to the approach used by https://reviews.llvm.org/D32449. But after I read the implementation of StreamChecker, I noticed that it uses program state bifurcation which is more straightforward. So I changed my checker to use this feature as well.

After receiving your suggestion, I conducted a benchmark to compare the performance of my previous return value approach with current state bifurcation approach. It took 11m46.383s to analyze magenta kernel code base by tracking the return value of syscalls compare to 11m39.034s by using state bifurcation. So in my case, the state bifurcation is actually faster, though I am not quite sure about the reason.

Another reason that we would like to keep the state bifurcation approach is that for magenta handle acquire and release syscalls. Both handle acquire syscall and handle release syscall may fail, so the return value of both types of syscalls should be tracked and should be tracked separately. The PthreadLockChecker only tracked one. It would be a lot of harder to get things right if we use return value tracking approach when adding more syscalls support.

Let me know if you have further suggestions or concerns.

Thanks,
Haowei


https://reviews.llvm.org/D34724

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/MagentaHandleChecker.cpp
  test/Analysis/mxhandle.c

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D34724.107201.patch
Type: text/x-patch
Size: 44711 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170719/0428210c/attachment-0001.bin>