[PATCH] Add generic ThreadRegistry class for sanitizer runtimes.

[Alexey Samsonov]

  Updated this change together with http://llvm-reviews.chandlerc.com/D534


================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:44
@@ +43,3 @@
+
+  const int tid;  // Thread ID. Main thread should have tid = 0.
+  int unique_id;  // Unique thread ID.
----------------
Kostya Serebryany wrote:
> shouldn't this be u32? 
Done

================
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.
----------------
Dmitry Vyukov wrote:
> please make it u64, int is not enough for everyone
Done

================
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).
----------------
Dmitry Vyukov wrote:
> add that it is used for reporting
Done

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

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

================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.h:134
@@ +133,3 @@
+ public:
+  explicit ThreadRegistry(LinkerInitialized)
+      : mtx_(LINKER_INITIALIZED) {}
----------------
Dmitry Vyukov wrote:
> 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.
Done

================
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_);
----------------
Dmitry Vyukov wrote:
> such method usually called *Locked*
Done

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

================
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:
----------------
Dmitry Vyukov wrote:
> 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.
Done

================
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 {
----------------
Dmitry Vyukov wrote:
> template parameters for ThreadQuarantineSize is overkill here
> ctor parameter will do
Done

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

================
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_);
----------------
Dmitry Vyukov wrote:
> it should not lock the mutex, but instead require that user has locked the mutex
Could you point a place where you want to use a modified version?


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