Dead lock when forking with coverage enabled in multithreaded environment

[calixteman]

Bugzilla Link	44792
Resolution	FIXED
Resolved on	May 07, 2020 11:21
Version	unspecified
OS	Linux
Blocks	#44654 #43900
CC	@zmodem,@jfbastien,@marco-c,@sylvestre,@tstellar,@vedantk
Fixed by commit(s)	bec223a f082d91

Extended Description

When forking, the mutex defined here:
https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/profile/GCDAProfiling.c#L67
must be reinitialized in child process else the mutex in a locked stated can be duplicate and then when a thread in child process attempts to lock it then it wait for ever.

[calixteman]

assigned to @marco-c

[calixteman]

Patch: https://reviews.llvm.org/D74071

[calixteman]

Finally the patch is there:
https://reviews.llvm.org/D74953

[calixteman]

:hans, is it possible to get this patch in llvm 10 ?

[zmodem]

:hans, is it possible to get this patch in llvm 10 ?

I'm guessing (please let me know if I'm wrong) that this is not fixing a recent regression. Since we're so late in the process, I'm hesitant to merge it. Perhaps it would be a good candidate for 10.0.1.

[zmodem]

:hans, is it possible to get this patch in llvm 10 ?

I'm guessing (please let me know if I'm wrong) that this is not fixing a
recent regression. Since we're so late in the process, I'm hesitant to merge
it. Perhaps it would be a good candidate for 10.0.1.

(unblocking to reflect this)

[calixteman]

:hans, it's to fix a regression I introduced here:
https://reviews.llvm.org/D70910

The mutex isn't reinitialized when forking.
So I made a first fix: https://reviews.llvm.org/D74071

but to avoid to lock too much I made the patch:
https://reviews.llvm.org/D74953
with a follow-up: https://reviews.llvm.org/D75080 (to fix a mac test)

so it'd be painful to have a regression in clang 10.

[zmodem]

:hans, it's to fix a regression I introduced here:
https://reviews.llvm.org/D70910

I see.

jfb, you had comments on that patch. Do the follow-ups make sense to you?

The mutex isn't reinitialized when forking.
So I made a first fix: https://reviews.llvm.org/D74071

This one didn't land yet. Do I understand correctly that it was replaced by the first patch below?

but to avoid to lock too much I made the patch:
https://reviews.llvm.org/D74953
with a follow-up: https://reviews.llvm.org/D75080 (to fix a mac test)

so it'd be painful to have a regression in clang 10.

Okay, these just landed so let's give them some time on master first at least.

Can you confirm whether this was tested in Mozilla's CI?

[calixteman]

yes we actively tested the patch on mozilla's CI and everything is ok.

[jfbastien]

:hans, it's to fix a regression I introduced here:
https://reviews.llvm.org/D70910

I see.

jfb, you had comments on that patch. Do the follow-ups make sense to you?

The first one didn't, I'm not sure the second one will :)

CC'ing Vedant who might be able to help with this.

[vedantk]

I'd like to respectfully suggest that we back out all the changes related to adding special handling for gcov + fork() on llvm/master, as well as on the llvm-10 branch. (This is just my 2c, fwiw I'm /not/ terribly familiar with gcov, I just work on clang's coverage implementation.)

Imho this special fork support needs more time to bake.

I have pointed out in the past that this functionality seems to be quite flaky. After multiple attempts to contact the contributors, we could not get the sporadic test failures addressed (see the list threads re: r347779), and as a result the test has been disabled on Darwin & Linux (9639f [profile] Mark instrprof-gcov-fork.test UNSUPPORTED on Darwin as well).

I have doubts about whether this special handling can really interoperate well when instrumented and un-instrumented code is mixed.

And I have basic questions about why the critical section added in D70910 is only present in one API, when it seems like any number of the gcov APIs could race for the same global.

I think the best course would be to first reset llvm to a known good state, and then solicit advice on llvm-dev about the best way to tackle this problem.

[zmodem]

Sounds good to me. On master:

commit 546918c
Author: Hans Wennborg hans@chromium.org
Date: Wed Feb 26 13:23:56 2020 +0100

Revert "[compiler-rt] Add a critical section when flushing gcov counters"

See discussion on llvm/llvm-project#44137 .

This reverts commit 02ce9d8ef5a84bc884de4105eae5f8736ef67634.

It also reverts the follow-up commits
8f46269f0 "[profile] Don't dump counters when forking and don't reset when calling exec** functions"
62c7d8402 "[profile] gcov_mutex must be static"

On 10.x:

commit 57a064f
Author: Hans Wennborg hans@chromium.org
Date: Wed Feb 26 13:31:58 2020 +0100

Revert "[compiler-rt] Add a critical section when flushing gcov counters"

See the discussion on llvm/llvm-project#44137 .

This reverts commit 02ce9d8ef5a84bc884de4105eae5f8736ef67634.

I believe this gets us back to the state things were in in the 9.x releases.

[marco-c]

I'd like to respectfully suggest that we back out all the changes related to
adding special handling for gcov + fork() on llvm/master, as well as on the
llvm-10 branch. (This is just my 2c, fwiw I'm /not/ terribly familiar with
gcov, I just work on clang's coverage implementation.)

Imho this special fork support needs more time to bake.

I have pointed out in the past that this functionality seems to be quite
flaky. After multiple attempts to contact the contributors, we could not get
the sporadic test failures addressed (see the list threads re: r347779), and
as a result the test has been disabled on Darwin & Linux (9639f [profile]
Mark instrprof-gcov-fork.test UNSUPPORTED on Darwin as well).

I have doubts about whether this special handling can really interoperate
well when instrumented and un-instrumented code is mixed.

And I have basic questions about why the critical section added in D70910 is
only present in one API, when it seems like any number of the gcov APIs
could race for the same global.

I think the best course would be to first reset llvm to a known good state,
and then solicit advice on llvm-dev about the best way to tackle this
problem.

I understand the need to let these patches bake more and not rush them in 10, so I agree with backing them out of 10.
I think they can be landed in 11 though, so that there is enough bake time.

There might be races that are not fixed by the patch, but the patch definitely fixes at least a race (a very common one), so I would argue it is still useful. Maybe not complete, but an improvement over the status quo (indeed, with these patches we no longer see problems in a coverage instrumented Firefox).

We can then figure out if other critical sections are needed (note that GCC is also implementing things similarly to us and they also are only locking in __gcov_flush).

[jfbastien]

The issue I tried to raise in review, and which Vedant seems to raise as well, is that we can't just fix these issues as they come without looking at the bigger picture, and addressing the entire issue surrounding fork and gcov. We need a big picture of what is expected to work, and how to make sure it does work reliably in all these cases. Ideally, we'd also be able to provide clean build diagnostics or runtime failures when the unsupported use cases are met.

Doing anything else risks making the entire thing more brittle un fun and unexpected ways.

[calixteman]

The first patch I made aimed to fix a crash we discovered in our CI when trying to switch to clang under linux (it was ok under windows because of no-fork).
So I'll write a patch asap to remove fork stuff else the failure will be still there in clang 10 (and it's present in clang 9).