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