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

[Roland McGrath via Phabricator via llvm-commits]

mcgrathr added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_thread_registry.cc:265
   CHECK_NE(tctx, 0);
-  CHECK_EQ(ThreadStatusRunning, tctx->status);
+  bool dead = tctx->detached;
+  if (tctx->status == ThreadStatusRunning) {
----------------
vitalybuka wrote:
> can you remove dead and just have 
> 
> 
> ```
> if (tctx->detached) {
>    tctx->detached = false;
>     tctx->SetDead();
>     QuarantinePush(tctx);
> }
> ```
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.



https://reviews.llvm.org/D36385