Page MenuHomePhabricator

[asan] Refactor thread creation bookkeeping
ClosedPublic

Authored by mcgrathr on Aug 6 2017, 5:39 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mcgrathr created this revision.Aug 6 2017, 5:39 PM
vitalybuka added inline comments.Aug 6 2017, 9:43 PM
lib/sanitizer_common/sanitizer_thread_registry.cc
258 ↗(On Diff #109946)

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

mcgrathr updated this revision to Diff 110101.Aug 7 2017, 3:23 PM
mcgrathr marked an inline comment as done.

Avoided new argument to FinishThread.

vitalybuka added inline comments.Aug 7 2017, 3:55 PM
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 added inline comments.Aug 7 2017, 5:10 PM
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 added inline comments.Aug 8 2017, 10:28 AM
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 marked an inline comment as done.Aug 8 2017, 11:55 AM
mcgrathr added inline comments.
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 this revision to Diff 110246.Aug 8 2017, 11:57 AM

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

vitalybuka added inline comments.Aug 8 2017, 1:39 PM
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 accepted this revision.Aug 8 2017, 1:52 PM
alekseyshl added inline comments.
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);
}
This revision is now accepted and ready to land.Aug 8 2017, 1:52 PM
mcgrathr marked 3 inline comments as done.Aug 8 2017, 4:56 PM
mcgrathr updated this revision to Diff 110307.Aug 8 2017, 5:04 PM

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

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 accepted this revision.Aug 8 2017, 5:32 PM

dead returned back?

vitalybuka requested changes to this revision.Aug 8 2017, 5:33 PM

Is this a merge mistake or intentional?

This revision now requires changes to proceed.Aug 8 2017, 5:33 PM
vitalybuka added inline comments.Aug 8 2017, 5:35 PM
lib/asan/asan_thread.cc
290 ↗(On Diff #110101)

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

dead returned back?

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

vitalybuka accepted this revision.Aug 8 2017, 5:38 PM
This revision is now accepted and ready to land.Aug 8 2017, 5:38 PM
This revision was automatically updated to reflect the committed changes.
krytarowski added inline comments.
compiler-rt/trunk/lib/asan/asan_thread.cc
277

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

mcgrathr added inline comments.Aug 9 2017, 11:31 AM
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 added inline comments.Aug 9 2017, 11:37 AM
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().