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

[Florian via Phabricator via llvm-commits]

Florian added a comment.

Hello Dmitry,

thanks for your feedback. The test compiles fine on my machine, it looks like your linker is missing C++ symbols. I converted the test to be C instead of C++ and hope it works now.

Regarding the shutdown-sequence:

Firstly, tsan_rtl_proc.h defines clean methods for creating and destroying a Process class (memory allocation and initialization).
Processor *ProcCreate()
void ProcDestroy(Processor *proc)
Maybe it would be beneficial to do the same for the thread state?

Secondly, I got a question regarding responsibilities in ThreadStates. The classes ThreadState and ThreadContext seem to represent a logical thread, i.e. a logical strand of control. Process represents a 'real' thread, i.e. an operating system controlled execution entity like a single pthread. Would it be a worthwhile effort for me to pull responsibilities of logical and 'real' threads apart?

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?

Best regards,
Florian



================
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:
> 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).


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

https://reviews.llvm.org/D76073