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 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
