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

[Florian via Phabricator via llvm-commits]

Florian added a comment.

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

> 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.


Yes, I absolutely agree. It took me quite a while to figure out that OnFinished is part of the cleanup sequence.
I think it would be beneficial to collect all clean-up code in these main lifecycle functions and make all code that wants to change a ThreadState use them. This way one can follow what happens at specific points of a thread's lifetime by the steps listed in the corresponding function.

If platform specific cleanups are needed on ThreadState transitions, they can get explicit platform callbacks.

  int ThreadCreate(ThreadState *thr, uptr pc, uptr uid, bool detached);
  void ThreadStart(ThreadState *thr, int tid, tid_t os_id,
                   ThreadType thread_type);
  void ThreadFinish(ThreadState *thr);
  ...
  
  Processor *ProcCreate();
  void ProcDestroy(Processor *proc);
  void ProcWire(Processor *proc, ThreadState *thr);
  void ProcUnwire(Processor *proc, ThreadState *thr);

It probably is quite messy to get all nuances right when refactoring. I will give it a first experimental go if my spare time allows this week :)

Should we take this bigger discussion on refactoring to the mailing list and keep this review to fixing the leak?


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

https://reviews.llvm.org/D76073