[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#1782265 <https://reviews.llvm.org/D70910#1782265>, @calixte wrote:

> @jfb, sorry I thought my commit message was clear enough.
>  So when coverage is enabled a call to `__gcov_flush` is inserted just before `fork`call:
>  https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp#L659
>  So if the parent process is forked in different threads then `__gcov_flush` is called from different threads.
>  And so different global variables are accessed asynchronously:
>  https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/profile/GCDAProfiling.c#L104-L108
>  Initially we saw crashes in our CI: https://bugzilla.mozilla.org/show_bug.cgi?id=1599436
>  The above test case leads to multiple errors: double-free, "cannot merge previous GCDA file..."
>  And in debbuging firefox, I saw `output_file == NULL` (https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/profile/GCDAProfiling.c#L603) where it mustn't.


OK that helps a bit. I still don't understand though: why do you only need to protect flush and nothing else? You're protecting specific variables from being accesses concurrently only though flush, and not through the other paths that access those variables. Are these variables never accessed concurrently from these other functions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70910