[PATCH] D76073: [compiler-rt][tsan] Fix: Leak of ThreadSignalContext memory mapping when destroying fibers.

Wed Mar 25 04:17:55 PDT 2020

dvyukov added a comment.

In D76073#1940985 <https://reviews.llvm.org/D76073#1940985>, @dvyukov wrote:

> > Lastly, if I plan to make changes, is it ok to introduce features like virtual classes and smart pointers (on non hot paths) or are there restrictions on these in the codebase?
>
> If it results in a real improvement and does not degrade performance of fast paths.
>  Fast paths were crafted very carefully to avoid any single excessive instruction. A virtual call on most fast paths is unacceptable.
>  Also it needs to make things simpler rather than harder. For example, reference counting may make things more obscure and non-transparent, lead to leaks and ordering bugs. Though, of course, absence of refcounting may lead to all the same problems :)
>  So I guess it will mostly depend on case-by-case basis. For example, we use scoped locks extensively and that's very handy and I guess prevented lots of bugs.

Speaking of unnecessary complexity and refactoring.

The current design of ThreadRegistry always confused me. Reading this change, it makes it very hard to follow logic.

We call ThreadFinish, which calls thread_registry->FinishThread which is in different directory, which always unconditionally calls virtual ThreadContext::OnFinished which is in a different part of tsan_rtl_thread.cc. I always need to open 5 files and do constant switching just to recover this part each time.

We need to remove OnFinished entirely and just do whatever we do there after the thread_registry->FinishThread call right in ThreadFinish.

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

https://reviews.llvm.org/D76073