[Lldb-commits] [lldb] [lldb][NFC] Unify thread detaching by making HostThreadPosix override Reset() (PR #179470)

[via lldb-commits]

daniilavdeev wrote:

> I don't think it is, because anyone calling Reset() on a HostThreadPosix is now also getting a pthread_detach. Which I thought would end up fixing the bug.

The previous version of `Reset()` results in losing the `pthread_t` entirely, which leads to memory leaks. A semantically correct approach would be to ensure that all resources are freed when the thread completes, that is what `pthread_detach` function guarantee.

`HostThreadPosix` has neither a copy constructor nor a copy assignment operator. The only way to access the underlying thread is to call `Release()` method, that returns the stored `pthread_t`, leaving an invalid `pthread_t` in the HostThreadPosix instance. That is to say, HostThreadPosix is the **only** place where the thread is stored.

Meanwhile, the `Reset()` function is similar to the `Release()` method, but unlike `Release()` it doesn't even return the `pthread_t` to users of this function. It indicates that the semantic of `Reset()` is to completely discard the access to a particular thread. Given that the application irreversibly loses the access to the thread, this is the last place where it is possible to prevent a memory leak.

> If this were NFC, it would override the method and then immediately call the base class' method.

Fair point, I agree that these changes do seem to affect the observable behavior, so probably I should remove NFC tag from the commit message.

https://github.com/llvm/llvm-project/pull/179470