[PATCH] D36385: [asan] Refactor thread creation bookkeeping

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl accepted this revision.
alekseyshl added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/asan/asan_thread.cc:290
+                       const_cast<uptr *>(&stack_size),
+                       &tls_begin_, &tls_size);
   stack_top_ = stack_bottom_ + stack_size;
----------------
mcgrathr wrote:
> alekseyshl wrote:
> > What was wrong with the wrapping here?
> 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.
It's exactly at 80 chars limit we enforce in sanitizers, please change it back to reduce the unnecessary churn.


================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.cc:276
+    CHECK_EQ(tctx->status, ThreadStatusCreated);
+    tctx->detached = true;
+  }
----------------
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);
  }



https://reviews.llvm.org/D36385