[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

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

[mcgrathr]

Avoided new argument to FinishThread.

[vitalybuka]

lib/sanitizer_common/sanitizer_thread_registry.cc

269–275

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

269–275

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

What was wrong with the wrapping here?

lib/sanitizer_common/sanitizer_thread_registry.cc

255

What does 'aborted' refer to?

276

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

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

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

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

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

275

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

"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 ↗

(On Diff #110314)

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

[mcgrathr]

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

277 ↗

(On Diff #110314)

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 ↗

(On Diff #110314)

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().

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);

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);
     }

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.
+  // Should be called from the thread itself when called with default zeros.
+  void Init(uptr stack_bottom = 0, uptr stack_size = 0);
   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();
+
+  // Should be called from the thread itself when called with default zeros.
+  void SetThreadStackAndTls(uptr stack_bottom = 0, uptr stack_size = 0);
+
   void ClearShadowForThreadStackAndTLS();
   FakeStack *AsyncSignalSafeLazyInitFakeStack();
 
   struct StackBounds {
     uptr bottom;
     uptr top;
   };
   StackBounds GetStackBounds() const;

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(uptr init_stack_bottom, uptr init_stack_size) {
   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(init_stack_bottom, init_stack_size);
   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 arguments, which are just passed
+// through from the caller of Init (which see, above).  They're only there
+// to support OS-specific implementations that can do this setup from the
+// creator thread before the new thread actually starts.
+void AsanThread::SetThreadStackAndTls(uptr init_stack_bottom,
+                                      uptr init_stack_size) {
+  DCHECK_EQ(init_stack_bottom, 0);
+  DCHECK_EQ(init_stack_size, 0);
   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);
+                       const_cast<uptr *>(&stack_size),
+                       &tls_begin_, &tls_size);
   stack_top_ = stack_bottom_ + stack_size;
   tls_end_ = tls_begin_ + tls_size;
   dtls_ = DTLS_Get();
 
   int local;
   CHECK(AddrIsInStack((uptr)&local));
 }
 

lib/sanitizer_common/sanitizer_thread_registry.h

public:
   ThreadContextBase *FindThreadContextLocked(FindThreadCallback cb,
                                              void *arg);
   ThreadContextBase *FindThreadContextByOsIDLocked(tid_t os_id);
 
   void SetThreadName(u32 tid, const char *name);
   void SetThreadNameByUserId(uptr user_id, const char *name);
   void DetachThread(u32 tid, void *arg);
   void JoinThread(u32 tid, void *arg);
-  void FinishThread(u32 tid);
+  void FinishThread(u32 tid, bool aborted = false);
   void StartThread(u32 tid, tid_t os_id, bool workerthread, void *arg);
 
  private:
   const ThreadContextFactory context_factory_;
   const u32 max_threads_;
   const u32 thread_quarantine_size_;
   const u32 max_reuse_;
 

lib/sanitizer_common/sanitizer_thread_registry.cc

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);
 }
 
-void ThreadRegistry::FinishThread(u32 tid) {
+// Normally this is called when the thread is about to exit.
+// If aborted is true, 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.
+void ThreadRegistry::FinishThread(u32 tid, bool aborted) {
   BlockingMutexLock l(&mtx_);
   CHECK_GT(alive_threads_, 0);
   alive_threads_--;
+  if (!aborted) {
     CHECK_GT(running_threads_, 0);
     running_threads_--;
+  }
   CHECK_LT(tid, n_contexts_);
   ThreadContextBase *tctx = threads_[tid];
   CHECK_NE(tctx, 0);
+  if (aborted) {
+    // The thread never really existed.
+    CHECK_EQ(ThreadStatusCreated, tctx->status);
+    tctx->detached = false;
+  } else {
     CHECK_EQ(ThreadStatusRunning, tctx->status);
+  }
   tctx->SetFinished();
-  if (tctx->detached) {
+  if (aborted || tctx->detached) {
     tctx->SetDead();
     QuarantinePush(tctx);
   }
 }
 
 void ThreadRegistry::StartThread(u32 tid, tid_t os_id, bool workerthread,
                                  void *arg) {
   BlockingMutexLock l(&mtx_);