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

[Florian via Phabricator via llvm-commits]

Florian marked an inline comment as done.
Florian added a comment.

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

> > Would it be a worthwhile effort for me to pull responsibilities of logical and 'real' threads apart?
>
> Yes. The Processor entity was introduced rather late in runtime evolution and only few most critical things were moved to the Processor from ThreadState. Perhaps more things can be moved.
>  Note tsan is also used for Go where Processor maps to a "logical processor" (a unit of GOMAXPROCS) and ThreadState maps to a goroutine. That was the main motivation for the split.


So essentially in Go a ThreadState is used as a Fiber (which they call goroutine and schedule internally)? This makes it behave the same as fibers in C++: you have worker threads that switch between logical fibers.

Does tsan then run its race-detection logic on a per ThreadState/per logical string of execution basis? If so, a clean separation between operating system threads and logical threads would remove the ThreadType::Fiber and treat all ThreadStates equally as a logical string of execution. However, I fear that such a separation of concerns would require too much rework...



================
Comment at: compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:895
+
+  if (thr->tctx->thread_type != ThreadType::Fiber) {
+    CHECK_EQ(thr, cur_thread());
----------------
dvyukov wrote:
> Florian wrote:
> > dvyukov wrote:
> > > I wonder if it's OK to use tctx here. We already called ctx->thread_registry->FinishThread(thr->tid) and tctx is owner by thread_registry. So by now it probably can be already reused for another thread.
> > I looked into it and tctx can indeed be re-used by another thread at this point leading to a race.
> > 
> > The thread context is an essential part of an alive thread state, therefore I moved the call to the PlatformThreadFinish clean up call into the OnFinished callback of the thread state. In here, the thread context is still valid. My only concern is that we then perform a rather 'heavy' clean up work while holding the mutex of the ThreadRegistry (if threads are created/destroyed at a moderate pace this should be fine).
> We could avoid holding the lock around the cleanup by passing ThreadType to the callback. That's the only bit it needs from tctx.
> Currently we know this bit, so we don't access tctx at all. With the new code forget this bit when call ThreadFinish and then we try to restore the knowledge of what we are destroying by looking at tctx.
> 
> 
Yes, good point. I actually had that one typed out as one possible solution to the race. I can submit the updated diff later.


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

https://reviews.llvm.org/D76073