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

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added a comment.

This is lots of changes. This makes me nervous and makes it harder for me approve this. Tsan is currently in maintenance mode with no official staffing and I don't remember most of this code well. Keeping changes "safer" makes it much easier to handle.
Say, you reorder thr->~ThreadState() and StatAggregate. And this has no explanation and is hidden in the code movement, so not even really visible. This makes me wonder why, and if you have given this enough consideration and are there other similar hidden changes...
Also moving all this ThreadSignalContext/SignalDesc/ucontext_buffer_t/768/129/65 into abstract logic layer makes me wince. It was specifically kept as "interceptors" stuff for a 8 years. I think the same can be achieved by providing a callback in tsan_interceptors_posix.cc with no major code movement.
If you restrict this to just fixing the bug, it will make both of us less scared of breaking things :)
Refactrorings may go in separate subsequent changes with own proper descriptions. This is much better for review, tests and these are also a unit of rollback. So if just a refactoring is rolled back, that's much better.


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

https://reviews.llvm.org/D76073