[asan] Refactor thread creation bookkeeping

[mcgrathr]

Summary

This is a pure refactoring change. It paves the way for OS-specific
implementations, such as Fuchsia's, that can do most of the
per-thread bookkeeping work in the creator thread before the new
thread actually starts. This model is simpler and cleaner, avoiding
some race issues that the interceptor code for thread creation has
to do for the existing OS-specific implementations.

[vitalybuka]

lib/sanitizer_common/sanitizer_thread_registry.cc

258 ↗

(On Diff #109946)

aborted = (ThreadStatusCreated == tctx->status)
so we can avoid additional argument

[mcgrathr]

Avoided new argument to FinishThread.

[vitalybuka]

lib/sanitizer_common/sanitizer_thread_registry.cc

265 ↗

(On Diff #110101)

can you remove dead and just have

if (tctx->detached) {
   tctx->detached = false;
    tctx->SetDead();
    QuarantinePush(tctx);
}

[mcgrathr]

lib/sanitizer_common/sanitizer_thread_registry.cc

265 ↗

(On Diff #110101)

I'm not sure I understand what you're suggesting.

The ->detached value has no effect on SetDead that I can see.

The call to SetDead must come after the call to SetFinished.

For the ThreaadStatusCreated (Fuchsia) case, ->detached must be false when
SetFinished is called. I don't see a way to do it without duplicating some
of the code in the two cases.

One option is:

if (tctx->status == ThreadStatusRunning) {                                    
  CHECK_GT(running_threads_, 0);                                              
  running_threads_--;                                                         
  tctx->SetFinished();                                                        
} else {                                                                      
  // The thread never really existed.                                         
  CHECK_EQ(tctx->status, ThreadStatusCreated);                                
  tctx->detached = false;                                                     
  tctx->SetFinished();                                                        
  tctx->detached = true;                                                      
}                                                                             
if (tctx->detached) {                                                         
  tctx->SetDead();                                                            
  QuarantinePush(tctx);                                                       
}

where SetFinished is duplicated and ->detached is re-set purely for the
purposes of the common test (i.e. turning it into what the 'dead' local was
doing in my earlier version).

Another option is:

if (tctx->status == ThreadStatusRunning) {                                    
  CHECK_GT(running_threads_, 0);                                              
  running_threads_--;                                                         
  tctx->SetFinished();                                                        
  if (tctx->detached) {                                                       
    tctx->SetDead();                                                          
    QuarantinePush(tctx);                                                     
  }                                                                           
} else {                                                                      
  // The thread never really existed.                                         
  CHECK_EQ(tctx->status, ThreadStatusCreated);                                
  tctx->detached = false;                                                     
  tctx->SetFinished();                                                        
  tctx->SetDead();                                                            
  QuarantinePush(tctx);                                                       
}

where basically everything is duplicated, but is concise in each fork of
the if.

IMHO both are more error-prone for future maintenance than what I wrote
with the 'dead' local, since there are duplicate code paths that have to be
updated in tandem if anything changes.

If there's another possibility you had in mind that gets everything right,
I haven't thought of it so you'll have to show me what your version of the
function actually looks like.

[alekseyshl]

lib/asan/asan_thread.cc

290 ↗

(On Diff #110101)

What was wrong with the wrapping here?

lib/sanitizer_common/sanitizer_thread_registry.cc

255 ↗

(On Diff #110101)

What does 'aborted' refer to?

272 ↗

(On Diff #110101)

The only reason you set detached to false here is to force SetFinished() to set status to ThreadStatusFinished, right? If so, wouldn't it be more appropriate to modify SetFinished to do this:

// Comment why checking status is important.
if (!detached || status == ThreadStatusCreated)
  status = ThreadStatusFinished;

then you won't need dead and detached = false.

[mcgrathr]

lib/asan/asan_thread.cc

290 ↗

(On Diff #110101)

It was over 79 chars, which makes an Emacs in an 80-chars-wide terminal put a \ at the end.
Obviously I don't care about formatting issues, this is just a change I make unconsciously in anything I'm touching.

lib/sanitizer_common/sanitizer_thread_registry.cc

255 ↗

(On Diff #110101)

It's left over from a previous version of the change. I'll update the comment.

[mcgrathr]

Updated stale comment. Changed FinishThread logic as suggested in review.

[vitalybuka]

lib/asan/asan_thread.h

71 ↗

(On Diff #110246)

In common code.

struct Options; // Probably don't need to define at all and pass null pointer.

void Init(const Options* options = nullptr) {
...
}

in fuchsia code:

struct AsanThread::Options {
  uptr stack_bottom;
  uptr stack_size;
};


void AsanThread::SetThreadStackAndTls(const Options* options) {
  ...
  // use  options.stack_bottom and  options.stack_size
}

[alekseyshl]

lib/asan/asan_thread.cc

290 ↗

(On Diff #110101)

It's exactly at 80 chars limit we enforce in sanitizers, please change it back to reduce the unnecessary churn.

lib/sanitizer_common/sanitizer_thread_registry.cc

276 ↗

(On Diff #110246)

To double check, you need those never started threads to be treated the same way as detached threads, right? The reason I'm asking is if you set it here only to call SetDead/QuarantinePush later, I'd rather have a local boolean flag than changing the tctx. If there's more behind it and you need the tctx->detached set, what we have now is fine.

Otherwise, sorry for the hassle, but it will be almost back to your original version, it conveys the semantics better:

bool dead = tctx->detached;
if (tctx->status == ThreadStatusRunning) {
  CHECK_GT(running_threads_, 0);
  running_threads_--;
} else {
  // The thread never really existed.
  CHECK_EQ(tctx->status, ThreadStatusCreated);
  dead = true;
}
tctx->SetFinished();
if (dead) {
  tctx->SetDead();
  QuarantinePush(tctx);
}

[mcgrathr]

Options struct for Init.
Remove excess reformatting.
Don't change tctx->detached.

[mcgrathr]

I think I've done everything requested in review.
If you are both happy with it now, please land it for me!

Thanks,
Roland

[vitalybuka]

dead returned back?

[vitalybuka]

Is this a merge mistake or intentional?

[vitalybuka]

lib/asan/asan_thread.cc

290 ↗

(On Diff #110101)

"git clang-format --style file -f origin/master" can help

[alekseyshl]

In D36385#836123, @vitalybuka wrote:

dead returned back?

Yep, it's back, I think it's better not to change tctx than to save one local var.

[krytarowski]

compiler-rt/trunk/lib/asan/asan_thread.cc

277

Does it mean that there is assumption that process id = thread it?

[mcgrathr]

compiler-rt/trunk/lib/asan/asan_thread.cc

277

Note this code is just moved around by this change (was in asan_rtl.cc), not introduced. The logic using internal_getpid() here (and earlier GetPid()) goes back many years.

On Linux it is the case that the TID of the initial thread is the same as the PID, and this code does seem to assume that.

[krytarowski]

compiler-rt/trunk/lib/asan/asan_thread.cc

277

This is invalid assumption.. however I don't see failures on my OS (NetBSD).

On NetBSD there is global PID for the process and TIDs (LWP IDs) for threads counting from 1, 2, 3...
To retrieve the current thread ID we need to call _lwp_self().

compiler-rt/trunk/lib/asan/asan_internal.h

 void ReplaceSystemMalloc();
 
 // asan_linux.cc / asan_mac.cc / asan_win.cc
 uptr FindDynamicShadowStart();
 void *AsanDoesNotSupportStaticLinkage();
 void AsanCheckDynamicRTPrereqs();
 void AsanCheckIncompatibleRT();
 
+// asan_thread.cc
+AsanThread *CreateMainThread();
+
 // Support function for __asan_(un)register_image_globals. Searches for the
 // loaded image containing `needle' and then enumerates all global metadata
 // structures declared in that image, applying `op' (e.g.,
 // __asan_(un)register_globals) to them.
 typedef void (*globals_op_fptr)(__asan_global *, uptr);
 void AsanApplyToGlobals(globals_op_fptr op, const void *needle);
 
 void AsanOnDeadlySignal(int, void *siginfo, void *context);

compiler-rt/trunk/lib/asan/asan_rtl.cc

static void AsanInitInternal() {
   // necessary, so that it can be re-activated when requested.
   if (flags()->start_deactivated)
     AsanDeactivate();
 
   // interceptors
   InitTlsSize();
 
   // Create main thread.
-  AsanThread *main_thread = AsanThread::Create(
-      /* start_routine */ nullptr, /* arg */ nullptr, /* parent_tid */ 0,
-      /* stack */ nullptr, /* detached */ true);
+  AsanThread *main_thread = CreateMainThread();
   CHECK_EQ(0, main_thread->tid());
-  SetCurrentThread(main_thread);
-  main_thread->ThreadStart(internal_getpid(),
-                           /* signal_thread_is_registered */ nullptr);
   force_interface_symbols();  // no-op.
   SanitizerInitializeUnwinder();
 
   if (CAN_SANITIZE_LEAKS) {
     __lsan::InitCommonLsan();
     if (common_flags()->detect_leaks && common_flags()->leak_check_at_exit) {
       Atexit(__lsan::DoLeakCheck);
     }

compiler-rt/trunk/lib/asan/asan_thread.h

explicit AsanThreadContext(int tid)
         thread(nullptr) {}
   bool announced;
   u8 destructor_iterations;
   u32 stack_id;
   AsanThread *thread;
 
   void OnCreated(void *arg) override;
   void OnFinished() override;
+
+  struct CreateThreadContextArgs {
+    AsanThread *thread;
+    StackTrace *stack;
+  };
 };
 
 // AsanThreadContext objects are never freed, so we need many of them.
 COMPILER_CHECK(sizeof(AsanThreadContext) <= 256);
 
 // AsanThread are stored in TSD and destroyed when the thread dies.
 class AsanThread {
  public:
   static AsanThread *Create(thread_callback_t start_routine, void *arg,
                             u32 parent_tid, StackTrace *stack, bool detached);
   static void TSDDtor(void *tsd);
   void Destroy();
 
-  void Init();  // Should be called from the thread itself.
+  struct InitOptions;
+  void Init(const InitOptions *options = nullptr);
+
   thread_return_t ThreadStart(tid_t os_id,
                               atomic_uintptr_t *signal_thread_is_registered);
 
   uptr stack_top();
   uptr stack_bottom();
   uptr stack_size();
   uptr tls_begin() { return tls_begin_; }
   uptr tls_end() { return tls_end_; }
public:
   void setInDeadlySignal(bool b) { in_deadly_signal_ = b; }
 
   AsanThreadLocalMallocStorage &malloc_storage() { return malloc_storage_; }
   AsanStats &stats() { return stats_; }
 
  private:
   // NOTE: There is no AsanThread constructor. It is allocated
   // via mmap() and *must* be valid in zero-initialized state.
-  void SetThreadStackAndTls();
+
+  void SetThreadStackAndTls(const InitOptions *options);
+
   void ClearShadowForThreadStackAndTLS();
   FakeStack *AsyncSignalSafeLazyInitFakeStack();
 
   struct StackBounds {
     uptr bottom;
     uptr top;
   };
   StackBounds GetStackBounds() const;

compiler-rt/trunk/lib/asan/asan_thread.cc

 #include "sanitizer_common/sanitizer_stackdepot.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 #include "lsan/lsan_common.h"
 
 namespace __asan {
 
 // AsanThreadContext implementation.
 
-struct CreateThreadContextArgs {
-  AsanThread *thread;
-  StackTrace *stack;
-};
-
 void AsanThreadContext::OnCreated(void *arg) {
   CreateThreadContextArgs *args = static_cast<CreateThreadContextArgs*>(arg);
   if (args->stack)
     stack_id = StackDepotPut(*args->stack);
   thread = args->thread;
   thread->set_context(this);
 }
 

 AsanThread *AsanThread::Create(thread_callback_t start_routine, void *arg,
                                u32 parent_tid, StackTrace *stack,
                                bool detached) {
   uptr PageSize = GetPageSizeCached();
   uptr size = RoundUpTo(sizeof(AsanThread), PageSize);
   AsanThread *thread = (AsanThread*)MmapOrDie(size, __func__);
   thread->start_routine_ = start_routine;
   thread->arg_ = arg;
-  CreateThreadContextArgs args = { thread, stack };
+  AsanThreadContext::CreateThreadContextArgs args = {thread, stack};
   asanThreadRegistry().CreateThread(*reinterpret_cast<uptr *>(thread), detached,
                                     parent_tid, &args);
 
   return thread;
 }
 
 void AsanThread::TSDDtor(void *tsd) {
   AsanThreadContext *context = (AsanThreadContext*)tsd;
stack_size_log =
         Max(stack_size_log, static_cast<uptr>(flags()->min_uar_stack_size_log));
     fake_stack_ = FakeStack::Create(stack_size_log);
     SetTLSFakeStack(fake_stack_);
     return fake_stack_;
   }
   return nullptr;
 }
 
-void AsanThread::Init() {
+void AsanThread::Init(const InitOptions *options) {
   next_stack_top_ = next_stack_bottom_ = 0;
   atomic_store(&stack_switching_, false, memory_order_release);
   fake_stack_ = nullptr;  // Will be initialized lazily if needed.
   CHECK_EQ(this->stack_size(), 0U);
-  SetThreadStackAndTls();
+  SetThreadStackAndTls(options);
   CHECK_GT(this->stack_size(), 0U);
   CHECK(AddrIsInMem(stack_bottom_));
   CHECK(AddrIsInMem(stack_top_ - 1));
   ClearShadowForThreadStackAndTLS();
   int local = 0;
   VReport(1, "T%d: stack [%p,%p) size 0x%zx; local=%p\n", tid(),
           (void *)stack_bottom_, (void *)stack_top_, stack_top_ - stack_bottom_,
           &local);
thread_return_t AsanThread::ThreadStart(
   // cleaned up by a user-defined TSD destructor. Thus, calling Destroy() before
   // the TSD destructors have run might cause false positives in LSan.
   if (!SANITIZER_POSIX)
     this->Destroy();
 
   return res;
 }
 
-void AsanThread::SetThreadStackAndTls() {
+AsanThread *CreateMainThread() {
+  AsanThread *main_thread = AsanThread::Create(
+      /* start_routine */ nullptr, /* arg */ nullptr, /* parent_tid */ 0,
+      /* stack */ nullptr, /* detached */ true);
+  SetCurrentThread(main_thread);
+  main_thread->ThreadStart(internal_getpid(),
+                           /* signal_thread_is_registered */ nullptr);
+  return main_thread;
+}
+
+// This implementation doesn't use the argument, which is just passed down
+// from the caller of Init (which see, above).  It's only there to support
+// OS-specific implementations that need more information passed through.
+void AsanThread::SetThreadStackAndTls(const InitOptions *options) {
+  DCHECK_EQ(options, nullptr);
   uptr tls_size = 0;
   uptr stack_size = 0;
   GetThreadStackAndTls(tid() == 0, const_cast<uptr *>(&stack_bottom_),
                        const_cast<uptr *>(&stack_size), &tls_begin_, &tls_size);
   stack_top_ = stack_bottom_ + stack_size;
   tls_end_ = tls_begin_ + tls_size;
   dtls_ = DTLS_Get();
 

compiler-rt/trunk/lib/sanitizer_common/sanitizer_thread_registry.cc

void ThreadContextBase::SetJoined(void *arg) {
   CHECK_EQ(false, detached);
   CHECK_EQ(ThreadStatusFinished, status);
   status = ThreadStatusDead;
   user_id = 0;
   OnJoined(arg);
 }
 
 void ThreadContextBase::SetFinished() {
-  if (!detached)
-    status = ThreadStatusFinished;
+  // ThreadRegistry::FinishThread calls here in ThreadStatusCreated state
+  // for a thread that never actually started.  In that case the thread
+  // should go to ThreadStatusFinished regardless of whether it was created
+  // as detached.
+  if (!detached || status == ThreadStatusCreated) status = ThreadStatusFinished;
   OnFinished();
 }
 
 void ThreadContextBase::SetStarted(tid_t _os_id, bool _workerthread,
                                    void *arg) {
   status = ThreadStatusRunning;
   os_id = _os_id;
   workerthread = _workerthread;
void ThreadRegistry::JoinThread(u32 tid, void *arg) {
   if (tctx->status == ThreadStatusInvalid) {
     Report("%s: Join of non-existent thread\n", SanitizerToolName);
     return;
   }
   tctx->SetJoined(arg);
   QuarantinePush(tctx);
 }
 
+// Normally this is called when the thread is about to exit.  If
+// called in ThreadStatusCreated state, then this thread was never
+// really started.  We just did CreateThread for a prospective new
+// thread before trying to create it, and then failed to actually
+// create it, and so never called StartThread.
 void ThreadRegistry::FinishThread(u32 tid) {
   BlockingMutexLock l(&mtx_);
   CHECK_GT(alive_threads_, 0);
   alive_threads_--;
-  CHECK_GT(running_threads_, 0);
-  running_threads_--;
   CHECK_LT(tid, n_contexts_);
   ThreadContextBase *tctx = threads_[tid];
   CHECK_NE(tctx, 0);
-  CHECK_EQ(ThreadStatusRunning, tctx->status);
+  bool dead = tctx->detached;
+  if (tctx->status == ThreadStatusRunning) {
+    CHECK_GT(running_threads_, 0);
+    running_threads_--;
+  } else {
+    // The thread never really existed.
+    CHECK_EQ(tctx->status, ThreadStatusCreated);
+    dead = true;
+  }
   tctx->SetFinished();
-  if (tctx->detached) {
+  if (dead) {
     tctx->SetDead();
     QuarantinePush(tctx);
   }
 }
 
 void ThreadRegistry::StartThread(u32 tid, tid_t os_id, bool workerthread,
                                  void *arg) {
   BlockingMutexLock l(&mtx_);