[PATCH] D70910: [compiler-rt] Add a critical section when flushing gcov counters

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

In D70910#1776744 <https://reviews.llvm.org/D70910#1776744>, @calixte wrote:

> @jfb sorry but it fixes issues with fork:
>
> - we had a lot of crashes in Firefox CI when dumping the GCDAs and so I tried this patch on the CI and no more weird crashes.
> - and a minimal test case to reproduce it: ``` #include <iostream> #include <thread> #include <vector> #include <unistd.h>
>
>   void foo() { fork(); }
>
>   int main() { std::vector<std::thread> pool;
>
>   for (int i = 0; i < 10; i++) { pool.emplace_back(std::thread(foo)); }
>
>   for (auto & t : pool) { t.join(); }
>
>   return 0; } ```
>
>   If I compile and run it (`clang++-9 -std=c++11 foo.cpp -oo -lpthread --coverage && ./o`) I get systematically a SEGFAULT and with the patch no more crashes. The problem is not with `flush_fn_list` itself but it's when the functions in the list (gcda dumper) are called concurrently.


I don't understand this fix then. Can you please explain more? Specifically, if the problem occurs when `fork`ing a profiled program then a mutex won't help you. I understand that you were having crashes and aren't anymore, but I don't understand what the actual problem was, and why the proposed fix is correct.  If the missing synchronization is in gcda dumpers, why aren't you synchronizing there? What's the actual segfault, why is it crashing? What's the shared state?

At a minimum I'd expect the commit message to explain this... and asking these questions I'm not sure the actual problem has been addressed. Can you please clarify?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70910