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

Fri Apr 14 12:38:22 PDT 2023

sivachandra added inline comments.

================
Comment at: libc/src/__support/threads/linux/thread.cpp:355
-
-  cleanup_thread_resources(attrib);
-
----------------
goldstein.w.n wrote:
> sivachandra wrote:
> > goldstein.w.n wrote:
> > > sivachandra wrote:
> > > > goldstein.w.n wrote:
> > > > > 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.
> > > > So, IIUC, what you are saying is that, before the call to `wait`, you want to add something like this:
> > > > 
> > > > ```
> > > > if (joinable_state == uint32_t(DetachState::DETACHED))
> > > >   return EINVAL;
> > > > ```
> > > > 
> > > > I think it is OK but I am not sure it would be the cleanest thing to do. Consider a case where user code is calling `detach` twice on a thread. If the second `detach` happens after the thread has finished and cleaned up itself, then what you read into `joinable_state` is garbage. In fact, the program should crash because thread attrib's are on the thread stack which was likely already unmaped. So, that conditional is assuming that the thread was still running when the attribs were read. I am not sure making such assumptions is the correct thing to do.
> > > > So, IIUC, what you are saying is that, before the call to `wait`, you want to add something like this:
> > > > 
> > > > ```
> > > > if (joinable_state == uint32_t(DetachState::DETACHED))
> > > >   return EINVAL;
> > > > ```
> > > > 
> > > > I think it is OK but I am not sure it would be the cleanest thing to do. Consider a case where user code is calling `detach` twice on a thread. If the second `detach` happens after the thread has finished and cleaned up itself, then what you read into `joinable_state` is garbage. In fact, the program should crash because thread attrib's are on the thread stack which was likely already unmaped. So, that conditional is assuming that the thread was still running when the attribs were read. I am not sure making such assumptions is the correct thing to do.
> > > 
> > > Well its UB either way. We are still reading from attribs on the second detach when we try the cas from `JOINABLE`. In the UB case we aren't making string guarantees about whats going to happen, but we can at least try and handle it gracefully.
> > > 
> > > I don't have any strong opinion about whether we handle the case with `abort` or `EINVAL`, I just think `wait` which can cause an infinite loop (if the thread is calling detach on itself) is not really a great way to handle a not too uncommon bug. I tend towards `EINVAL` so its up to the user if they want to `abort` or handle however, but `abort` seems perfectly reasonable as well.
> > > I don't have any strong opinion about whether we handle the case with `abort` or `EINVAL`, I just think `wait` which can cause an infinite loop (if the thread is calling detach on itself) is not really a great way to handle a not too uncommon bug. I tend towards `EINVAL` so its up to the user if they want to `abort` or handle however, but `abort` seems perfectly reasonable as well.
> > 
> > If a joinable thread is calling `detach` on itself just once, it should work just fine (there will be no call to `wait`). But, if a detached thread is calling `detach` on itself, it is UB. Are you trying to address the latter case? All of this boils down to user code discipline. If a library has to support incorrect user code, then we need evidence (like bug reports et al.) to facilitate widespread reliance on a particular behavior.
> > 
> > 
> > > I don't have any strong opinion about whether we handle the case with `abort` or `EINVAL`, I just think `wait` which can cause an infinite loop (if the thread is calling detach on itself) is not really a great way to handle a not too uncommon bug. I tend towards `EINVAL` so its up to the user if they want to `abort` or handle however, but `abort` seems perfectly reasonable as well.
> > 
> > If a joinable thread is calling `detach` on itself just once, it should work just fine (there will be no call to `wait`). But, if a detached thread is calling `detach` on itself, it is UB. Are you trying to address the latter case? All of this boils down to user code discipline. If a library has to support incorrect user code, then we need evidence (like bug reports et al.) to facilitate widespread reliance on a particular behavior.
> > 
> > 
> 
> The goal is not the support the UB case, its to make the UB case throw a more explicit error when its obviously detectable.
> 
> Its like the `ESRCH`. Libraries are not required to return `ESRCH` if a user passes an invalid tid, but if it can easily detect it why not? There is a sense of prefered behavior in some UB cases. You can't rely on it, but its still useful.
> Its like the `ESRCH`. Libraries are not required to return `ESRCH` if a user passes an invalid tid, but if it can easily detect it why not? There is a sense of prefered behavior in some UB cases. You can't rely on it, but its still useful.

If that "preferred behavior" is reliably reliable (yes, reliably reliable), then we *can* support those "why not" cases. In this case, it is not a reliably reliable behavior. It is more of a hopeful attempt with no guarantees of repeatability.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148294