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