This is an archive of the discontinued LLVM Phabricator instance.

[Profile] Avoid race condition when dumping GCDA files.
AcceptedPublic

Authored by calixte on Nov 15 2018, 1:50 PM.

Details

Reviewers
marco-c
vsk
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)

Event Timeline

calixte created this revision.Nov 15 2018, 1:50 PM
Herald added subscribers: Restricted Project, llvm-commits, delcypher. · View Herald TranscriptNov 15 2018, 1:50 PM
vsk added a comment.Nov 15 2018, 2:01 PM

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 edited the summary of this revision. (Show Details)Nov 15 2018, 2:25 PM
calixte added inline comments.
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)

calixte edited the summary of this revision. (Show Details)Nov 15 2018, 2:35 PM
vsk added inline comments.Nov 15 2018, 2:52 PM
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 added inline comments.Nov 16 2018, 4:35 AM
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 updated this revision to Diff 174508.Nov 17 2018, 8:52 AM
  • Update code to take into account what's wrong when fd==-1
  • Add a test for GCOV_PREFIX

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 updated this revision to Diff 175705.Nov 28 2018, 9:17 AM

Make a more concise code.

marco-c accepted this revision.Dec 3 2018, 8:11 AM
marco-c added inline comments.
lib/profile/InstrProfilingUtil.h
27 ↗(On Diff #175705)

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

This revision is now accepted and ready to land.Dec 3 2018, 8:11 AM

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

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.

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.