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

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Thanks for upstreaming this! (And it was great to meet you at the developer conference.)



================
Comment at: lib/StaticAnalyzer/Checkers/SpinLockChecker.cpp:61
+
+const ErrorCategoryStr LockInfo::LockErrCategory("Lock Error");
+const FunctionNameStr LockInfo::SpinLockFuncName("spin_lock");
----------------
In the LLVM/clang codebase we try very hard to avoid running constructors for globals, as this can have adverse effects on startup time. Typically we try to lazily initialize things like these or have them refer to constant data that the linker will handle automatically (such as a c string constant).

Can these be a C string constant instead? Or can they be instance members of the  SpinLockChecker class? (See the note about `CallDescription` below, which may be helpful.)


================
Comment at: lib/StaticAnalyzer/Checkers/SpinLockChecker.cpp:118
+
+  FunctionNameStr CalleeName = Call.getCalleeIdentifier()->getName();
+  if (CalleeName != LockInfo::getSpinLockFuncName() &&
----------------
In the analyzer codebase we try to use IdentifierInfo pointers to compare identifiers. Since these are interned it lets us perform pointer comparisons rather than expensive string comparisons.

There is a `CallDescription` class in `CallEvent.h` that encapsulates this logic. You can see an example of how it is used in SimpleStreamChecker.cpp.



================
Comment at: lib/StaticAnalyzer/Checkers/SpinLockChecker.cpp:182
+  Report->markInteresting(SpinLockLocation);
+  C.emitReport(std::move(Report));
+}
----------------
For these kinds of diagnostics it is often very helpful to emit a path note indicating where the first unlock was.

To do this, you can add a custom bug report visitor that will get called on each node in the path to a diagnostic and emit a note, if appropriate. In this case you would look for nodes with calls to unlock taking the same region as the one you are now reporting for.

There is an example of how to implement a bug report visitor in 'SuperDeallocBRVisitor'.


================
Comment at: lib/StaticAnalyzer/Checkers/SpinLockChecker.cpp:192
+
+  auto Report = llvm::make_unique<BugReport>(
+      *DoubleLockBugType,
----------------
It would be good to emit a path note for the first lock, too.


https://reviews.llvm.org/D26340