[PATCH] Add generic ThreadRegistry class for sanitizer runtimes.

[Dmitry Vyukov]

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:45
@@ +44,3 @@
+  const int tid;  // Thread ID. Main thread should have tid = 0.
+  int unique_id;  // Unique thread ID.
+  uptr os_id;     // PID.
----------------
please make it u64, int is not enough for everyone

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:46
@@ +45,3 @@
+  int unique_id;  // Unique thread ID.
+  uptr os_id;     // PID.
+  uptr user_id;   // Some opaque user thread id (e.g. pthread_t).
----------------
add that it is used for reporting

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:35
@@ +34,3 @@
+// If thread is dead, context may optionally be reused for a new thread.
+class ThreadContext {
+ public:
----------------
I don't like that you name the subclass TsanThreadContext. We really should not prefix each and every entity in Tsan with Tsan. Please either rename this class to ThreadContextBase or something, or think of a better name for TsanThreadContext.

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:301
@@ +300,3 @@
+template <class Registry>
+class ThreadRegistryLock {
+ public:
----------------
Implement ThreadRegistry::Lock/Unlock, and then you can typedef GenericScopedLock<ThreadRegistry> ThreadRegistryLock.

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:73
@@ +72,3 @@
+    user_id = 0;
+  }
+  virtual void SetJoined(void *arg) {
----------------
please add new lines between methods

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:114
@@ +113,3 @@
+//   dead threads will be reused for new threads.
+template <class TCTX, u32 MaxThreads, u32 ThreadQuarantineSize>
+class ThreadRegistry {
----------------
template parameters for ThreadQuarantineSize is overkill here
ctor parameter will do

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:134
@@ +133,3 @@
+ public:
+  explicit ThreadRegistry(LinkerInitialized)
+      : mtx_(LINKER_INITIALIZED) {}
----------------
Make it a normal class, LinkerInitialized spreads like plague over codebase.
If/when you factor something out of this class, you will have to make it LinkerInitialized as well. If/when you include any other class into this class, you will have to support LinkerInitialized initialization for that class as well.

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:152
@@ +151,3 @@
+  // Should be guarded by ThreadRegistryLock.
+  TCTX *GetThreadUnlocked(u32 tid) {
+    DCHECK_LT(tid, n_contexts_);
----------------
such method usually called *Locked*

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:223
@@ +222,3 @@
+  // is found.
+  TCTX *FindThreadContextUnlocked(FindThreadCallback cb, void *arg) {
+    CheckLocked();
----------------
Locked

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:198
@@ +197,3 @@
+  // Invokes callback with a specified arg for each thread context.
+  void RunCallbackForEachThread(ThreadCallback cb, void *arg) {
+    BlockingMutexLock l(&mtx_);
----------------
it should not lock the mutex, but instead require that user has locked the mutex

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:129
@@ +128,3 @@
+
+  uptr alive_threads_;  // created or running.
+  uptr max_alive_threads_;
----------------
please group this with total_threads_


http://llvm-reviews.chandlerc.com/D530