[LIBC] Handle multiple calls to `detach` more gracefully

[goldstein.w.n]

Summary

Just return an error for incorrect use rather than almost certainly
creating an inf loop.

Its UB either way, but there is no reason to make the bugs more costly
than they need to be.

[sivachandra]

libc/src/__support/threads/linux/thread.cpp

355

A joinable but exiting thread will not cleanup its resources if we remove this. What problem is your patch trying to address?

[goldstein.w.n]

libc/src/__support/threads/linux/thread.cpp

355

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.

[sivachandra]

libc/src/__support/threads/linux/thread.cpp

355

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.

[goldstein.w.n]

libc/src/__support/threads/linux/thread.cpp

355

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.

[sivachandra]

libc/src/__support/threads/linux/thread.cpp

355

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.

[goldstein.w.n]

libc/src/__support/threads/linux/thread.cpp

355

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.

[sivachandra]

libc/src/__support/threads/linux/thread.cpp

355

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.

[goldstein.w.n]

libc/src/__support/threads/linux/thread.cpp

355

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.

[sivachandra]

libc/src/__support/threads/linux/thread.cpp

355

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.

libc/src/__support/threads/linux/thread.cpp

int Thread::join(ThreadReturnValue &retval) {
 
   return 0;
 }
 
 int Thread::detach() {
   uint32_t joinable_state = uint32_t(DetachState::JOINABLE);
   if (attrib->detach_state.compare_exchange_strong(
           joinable_state, uint32_t(DetachState::DETACHED))) {
-    return int(DetachType::SIMPLE);
+    return 0;
   }
 
-  // If the thread was already detached, then the detach method should not
-  // be called at all. If the thread is exiting, then we wait for it to exit
-  // and free up resources.
-  // TODO: Should we handle this less destructively? Maybe just return EINVAL?
-  wait();
-
-  cleanup_thread_resources(attrib);
-
-  return int(DetachType::CLEANUP);
+  return EINVAL;
 }
 
 void Thread::wait() {
   // The kernel should set the value at the clear tid address to zero.
   // If not, it is a spurious wake and we should continue to wait on
   // the futex.
   auto *clear_tid =
       reinterpret_cast<cpp::Atomic<FutexWordType> *>(attrib->platform_data);

libc/src/pthread/pthread_detach.cpp

 
 namespace __llvm_libc {
 
 static_assert(sizeof(pthread_t) == sizeof(__llvm_libc::Thread),
               "Mismatch between pthread_t and internal Thread.");
 
 LLVM_LIBC_FUNCTION(int, pthread_detach, (pthread_t th)) {
   auto *thread = reinterpret_cast<Thread *>(&th);
-  thread->detach();
-  return 0;
+  return thread->detach();
 }
 
 } // namespace __llvm_libc