In D82342#2126288, @rsanthir.quic wrote:

@Florian since we've given it a couple days, would it be ok to make the commit now?

I integrated your feedback and put a separate callback for just the clean up. To not do any further refactoring I placed it in OnFinished where also other resources of ThreadState are freed.
The cleanup accesses non of the previously freed resources and does not free any resources needed in the destructor called after it or the stat aggregation, which are the only functions accessing ThreadState afterwards.

Making the ThreadFinish call into platform specific code caused all kinds of further issues.
Now the ThreadState itself owns and manages the ThreadSignalContext, as it is required on all supported platforms anyways.

In D76073#1943110, @dvyukov wrote:

In D76073#1942712, @JDevlieghere wrote:

It appears this broke the LLDB bot: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/13127/testReport/junit/

Can you please take a look?

Florian, please take a look. There are some crashes on darwin.

In D76073#1941010, @dvyukov wrote:

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

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

Hello Dmitry,

• The clean-up callback for the platform code is moved to a different part of the thread destruction sequence.
• Convert the test case to C instead of C++