[PATCH] D26340: [analyzer] Add SpinLockChecker for the Magenta kernel

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Thanks for adding the path notes and adopting CallDescription. I've added some additional comments inline, which are mostly minor nits.

Two additional important changes -- and I should have noted these in the initial review -- is that it would be good to remove a MemRegion mapping from the program state when a MemRegion escapes and also when it becomes dead. These should both be small changes.

**Escaping**
Removing the mapping when the lock escapes will avoid false positives when the lock is passed to a function that the analyzer doesn't inline (for example, if it is in another translation unit) that unlocks it:

  void foo(lock_t *l) {
    spin_lock(l);
    ...
    do_stuff_and_unlock(l);
    ..
    spin_lock();
  }

If `do_stuff_and_unlock()` is in another translation unit the analyzer won't see the unlock and so will report a false positive if you don't remove the mapping when the lock escapes.

If you remove the mapping when the lock escapes, the analyzer will optimistically assume the best-case scenario the next time it sees a lock or unlock operation. There is an example of how to do this in `ObjCContainersChecker::checkPointerEscape()`

**Removing Dead Symbols**
It is also important for performance to remove mappings that are no longer relevant. There is a cost to keep around mappings in the program state: both memory (because the analyzer keeps some states around for use in the bug reporter visitor) and time (because the analyzer has to iterate over the map when determining whether two states are the same).  There is an example of how to do this in `ObjCLoopChecker::checkDeadSymbols`.



================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:728
+//===----------------------------------------------------------------------===//
+// Magenta checkers.
+//===----------------------------------------------------------------------===//
----------------
Nit: I think it would good to add a little bit more context to disambiguate so that future maintainers will be able to do a web search to learn about what these checkers are for. For example, maybe something like "Checkers for the Magenta kernel"?


================
Comment at: lib/StaticAnalyzer/Checkers/SpinLockChecker.cpp:208
+      *DoubleUnlockBugType,
+      "Execution path found where spinlock is unlocked twice in a row", Node);
+  Report->markInteresting(SpinLockLocation);
----------------
Nit: it is enough to have the message be "Spinlock is unlocked twice in a row". I don't think it is necessary to explicitly mention the execution path (especially now that you have a path note for the first unlock).


================
Comment at: lib/StaticAnalyzer/Checkers/SpinLockChecker.cpp:224
+      *DoubleLockBugType,
+      "Execution path found where spinlock is locked twice in a row", Node);
+  Report->addRange(Call.getSourceRange());
----------------
Nit: Same here.


================
Comment at: test/Analysis/spinlock_correct.c:1
+// RUN: %clang_cc1 -analyze -analyzer-checker=magenta.SpinLock -verify %s
+// expected-no-diagnostics
----------------
Nit: Typically in the analyzer codebase we try to keep the number of test files to a minimum. Can these three test files all be combined into one?


================
Comment at: test/Analysis/spinlock_correct.c:1
+// RUN: %clang_cc1 -analyze -analyzer-checker=magenta.SpinLock -verify %s
+// expected-no-diagnostics
----------------
dcoughlin wrote:
> Nit: Typically in the analyzer codebase we try to keep the number of test files to a minimum. Can these three test files all be combined into one?
Another nit: The analyzer relies on always having core checkers enabled because they model important aspects of the language. You should change this to `... -analyzer-checker=core,magenta.SpinLock ...`


https://reviews.llvm.org/D26340