[PATCH] D21506: [analyzer] Block in critical section

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Thanks for the patch!

Have you run this on a large codebase?


================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:406
@@ +405,3 @@
+def BlockInCriticalSectionChecker : Checker<"BlockInCriticalSection">,
+  HelpText<"Check for blocks in critical section">,
+  DescFile<"BlockInCriticalSectionChecker.cpp">;
----------------
"blocks" sounds a lot like blocks ObjC feature. I'd just be more explicit and say "Check for calls to blocking functions inside a critical section".

Please, change the name of the class as well.

================
Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:54
@@ +53,3 @@
+
+REGISTER_TRAIT_WITH_PROGRAMSTATE(Counter, unsigned)
+
----------------
Counter -> MutexCounter or MutexState

================
Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:61
@@ +60,3 @@
+  BlockInCritSectionBugType.reset(
+      new BugType(this, "Block in critical section", "Unix Stream API Error"));
+}
----------------
Same here: "Block" sounds like an ObjC language feature. Also, the category name ("Unix Stream API Error") should be different.

================
Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:85
@@ +84,3 @@
+
+void BlockInCriticalSectionChecker::checkPreCall(const CallEvent &Call,
+                                                 CheckerContext &C) const {
----------------
Why is handling of unlock in preCall?

================
Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:96
@@ +95,3 @@
+
+void BlockInCriticalSectionChecker::reportBlockInCritSection(
+    SymbolRef BlockDescSym, const CallEvent &Call, CheckerContext &C) const {
----------------
It would be great if you could implement a BugReporterVisitor that walks the path and explains where the lock has occurred. (This is not blocking for this commit.)

================
Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:103
@@ +102,3 @@
+  auto R = llvm::make_unique<BugReport>(*BlockInCritSectionBugType,
+      "Block in a critical section", ErrNode);
+  R->addRange(Call.getSourceRange());
----------------
 "Block in a critical section" -> "A blocking function %s is called inside a critical section."

================
Comment at: test/Analysis/block-in-critical-section.cpp:12
@@ +11,3 @@
+
+void testBlockInCriticalSection() {
+  std::mutex m;
----------------
Please, add inter-procedural examples like in the description of the checker.


Repository:
  rL LLVM

http://reviews.llvm.org/D21506