[libc-commits] [PATCH] D148294: [LIBC] Handle multiple calls to `detach` more gracefully

[Noah Goldstein via Phabricator via libc-commits]

goldstein.w.n added inline comments.


================
Comment at: libc/src/__support/threads/linux/thread.cpp:355
-
-  cleanup_thread_resources(attrib);
-
----------------
sivachandra wrote:
> goldstein.w.n wrote:
> > sivachandra wrote:
> > > A joinable but exiting thread will not cleanup its resources if we remove this. What problem is your patch trying to address?
> > Hmm? AFAICT this is only the handle for `pthread_detach ()`, how does it affect the cleanup routine? Its just handling the UB case of multiple calls to `pthread_detach ()` without almost certainty causing an infinite loop.
> > 
> > If thread is joinable then `Thread::join()` does cleanup.
> > If thread is not joinable then `thread_exist()` does cleanup.
> > 
> > 
> > Hmm? AFAICT this is only the handle for `pthread_detach ()`, how does it affect the cleanup routine? Its just handling the UB case of multiple calls to `pthread_detach ()` without almost certainty causing an infinite loop.
> > 
> > If thread is joinable then `Thread::join()` does cleanup.
> > If thread is not joinable then `thread_exist()` does cleanup.
> 
> Consider this:
> 
> A joinable thread has finished so its status has changed to `EXITING` but no other thread has called `join` or `detach` yet. In this case, `thread_exit` will not cleanup thread's resources as the call to one of `join` or `detach` will have to succeed. If `detach` is called, then cleanup becomes its responsibility. Likewise, if `join` is called, then cleanup becomes its responsibility.
> 
> So, this cleanup here is for the case when `detach` is called on an already finished thread.
> > Hmm? AFAICT this is only the handle for `pthread_detach ()`, how does it affect the cleanup routine? Its just handling the UB case of multiple calls to `pthread_detach ()` without almost certainty causing an infinite loop.
> > 
> > If thread is joinable then `Thread::join()` does cleanup.
> > If thread is not joinable then `thread_exist()` does cleanup.
> 
> Consider this:
> 
> A joinable thread has finished so its status has changed to `EXITING` but no other thread has called `join` or `detach` yet. In this case, `thread_exit` will not cleanup thread's resources as the call to one of `join` or `detach` will have to succeed. If `detach` is called, then cleanup becomes its responsibility. Likewise, if `join` is called, then cleanup becomes its responsibility.
> 
> So, this cleanup here is for the case when `detach` is called on an already finished thread.

Okay, thats fair. Was thinking of detached as only on `pthread_self()`. How about just return `EINVAL` if current state is `DETACHED`? The thing is this is a not super uncommon race and failing with infinite loop just seems unpleasant / harder to detect for the user.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148294/new/

https://reviews.llvm.org/D148294