[Profile] Avoid race condition when dumping GCDA files.

[calixte]

Summary

Seldom the test instrprof-gcov-fork.test is failing, so:

• add an O_EXCL when trying to create the file to avoid a race condition;
• add an error message when the locking is failing.

The failure is:
/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/profile/Posix/../Inputs/instrprof-gcov-fork.c.gcov:10:15: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT:function func2 called 2 returned 100% blocks executed 100%

^

instrprof-gcov-fork.c.gcov:10:1: note: scanning from here
function func2 called 1 returned 100% blocks executed 100%
^

In this test we call func2 after having forked so it should be called two times (in the parent process and in the cihld process)

[vsk]

Could you share either a link to or the abbreviated text of the test failure?

lib/profile/GCDAProfiling.c

361

Why try to create the file again?

[calixte]

lib/profile/GCDAProfiling.c

361

The first attempt (line 355) can fail because the parent directory is not here or because the file already exists.
So we create the parent dirs (if required) and then try again to create the file and if it fails again then the file exists and then get the fd with fd = open(filename, O_RDWR | O_BINARY) (which shouldn't return -1)

[vsk]

lib/profile/GCDAProfiling.c

361

I'm a bit suspicious of the need to have 4 calls to open(). Could one of them be avoided by calling '__llvm_profile_recursive_mkdir' unconditionally?

How do InstrProfiling{Util,File}.c avoid the same race?

[marco-c]

lib/profile/GCDAProfiling.c

361

I suppose __llvm_profile_recursive_mkdir was added as an optimization (if GCOV_PREFIX is not set, then the directories should exist already).

InstrProfilingFile.c is always creating the directory first and it seems to directly open the file with O_CREAT, but it doesn't seem to handle the race any differently than GCDAProfiling.c (that is, it is not handling the race).

[calixte]

• Update code to take into account what's wrong when fd==-1
• Add a test for GCOV_PREFIX

[marco-c]

The code is a bit confusing now, too many if-else etc.. Let's see what happens if you apply my comments. Maybe you could also move the code to open a file with O_EXCL first and without O_EXCL in a separate function, since you're using it twice.

lib/profile/GCDAProfiling.c

345

Why do we need errnum? Couldn't we always use errno?

361–368

Need to test this on Windows to make sure that errno is set correctly there.

363

Nit: another instead of an other

371

You can set new_file to 1 as before to avoid another else here and earlier. If there is an error new_file will be unused anyway.

test/profile/Posix/instrprof-gcov-prefix.test

11 ↗

(On Diff #174508)

Could you also test that the gcda file is not created where it usually is when GCOV_PREFIX is not set?

[calixte]

Make a more concise code.

[marco-c]

lib/profile/InstrProfilingUtil.h

27 ↗

(On Diff #175705)

You could remove this from the header since it's only used by lprofOpenFile.

[marco-c]

I wonder if https://reviews.llvm.org/D76206 didn't fix this too

[kawashima-fj]

In D54599#1966949, @marco-c wrote:

I wonder if https://reviews.llvm.org/D76206 didn't fix this too

Sorry, I didn't notice this patch.
Probably D76206 and D79556 fix this too.

[calixte]

In D54599#2024449, @kawashima-fj wrote:

In D54599#1966949, @marco-c wrote:

I wonder if https://reviews.llvm.org/D76206 didn't fix this too

Sorry, I didn't notice this patch.
Probably D76206 and D79556 fix this too.

The problem I had with this patch is that I did it in a blind mode... so no way to check that it was fixing something.