[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

357–374

Why try to create the file again?

[calixte]

lib/profile/GCDAProfiling.c

357–374

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

357–374

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

357–374

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?

357

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

359

Nit: another instead of an other

377

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

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.

lib/profile/GCDAProfiling.c

 
 /* A file in this case is a translation unit. Each .o file built with line
  * profiling enabled will emit to a different file. Only one file may be
  * started at a time.
  */
 COMPILER_RT_VISIBILITY
 void llvm_gcda_start_file(const char *orig_filename, const char version[4],
                           uint32_t checksum) {
-  const char *mode = "r+b";
+  const char *mode;
+  int errnum = 0;
   filename = mangle_filename(orig_filename);
 
   /* Try just opening the file. */
   new_file = 0;
   fd = open(filename, O_RDWR | O_BINARY);
 
   if (fd == -1) {
-    /* Try opening the file, creating it if necessary. */
-    new_file = 1;
-    mode = "w+b";
-    fd = open(filename, O_RDWR | O_CREAT | O_BINARY, 0644);
+    /* Try opening the file, creating it if necessary.
+       To avoid a race condition, we use O_EXCL. */
+    fd = open(filename, O_RDWR | O_CREAT | O_EXCL | O_BINARY, 0644);
     if (fd == -1) {
+      errnum = errno;
+      if (errnum == EEXIST) {
+        /* In the meantime, an other process has created the file. */
+        fd = open(filename, O_RDWR | O_BINARY);
+        errnum = errno;
+      } else if (errnum == ENOENT) {
         /* Try creating the directories first then opening the file. */
         __llvm_profile_recursive_mkdir(filename);
-      fd = open(filename, O_RDWR | O_CREAT | O_BINARY, 0644);
-      if (fd == -1) {
-        /* Bah! It's hopeless. */
-        int errnum = errno;
-        fprintf(stderr, "profiling: %s: cannot open: %s\n", filename,
-                strerror(errnum));
-        return;
-      }
-    }
-  }
+        fd = open(filename, O_RDWR | O_CREAT | O_EXCL | O_BINARY, 0644);
+        if (fd == -1) {
+          errnum = errno;
+          if (errnum == EEXIST) {
+            fd = open(filename, O_RDWR | O_BINARY);
+            errnum = errno;
+          }
+        } else {
+          new_file = 1;
+        }
+      }
+    } else {
+      new_file = 1;
+    }
+  }
+
 
   /* Try to flock the file to serialize concurrent processes writing out to the
    * same GCDA. This can fail if the filesystem doesn't support it, but in that
    * case we'll just carry on with the old racy behaviour and hope for the best.
    */
-  lprofLockFd(fd);
+  if (lprofLockFd(fd) != 0) {
+    errnum = errno;
+    fprintf(stderr, "profiling: %s: cannot lock: %s\n", filename,
+            strerror(errnum));
+  }
+  mode = new_file == 1 ? "w+b" : "r+b";
   output_file = fdopen(fd, mode);
 
   /* Initialize the write buffer. */
   write_buffer = NULL;
   cur_buffer_size = 0;
   cur_pos = 0;
 
   if (new_file) {

lib/profile/InstrProfilingUtil.c

if (errno != EINTR) {
       if (errno == ENOLCK) {
         return -1;
       }
       break;
     }
   }
   return 0;
 #else
-  flock(fd, LOCK_EX);
-  return 0;
+  return flock(fd, LOCK_EX);
 #endif
 }
 
 COMPILER_RT_VISIBILITY int lprofUnlockFd(int fd) {
 #ifdef COMPILER_RT_HAS_FCNTL_LCK
   struct flock s_flock;
 
   s_flock.l_whence = SEEK_SET;
   s_flock.l_start = 0;
   s_flock.l_len = 0; /* Until EOF.  */
   s_flock.l_pid = getpid();
   s_flock.l_type = F_UNLCK;
 
   while (fcntl(fd, F_SETLKW, &s_flock) == -1) {
     if (errno != EINTR) {
       if (errno == ENOLCK) {
         return -1;
       }
       break;
     }
   }
   return 0;
 #else
-  flock(fd, LOCK_UN);
-  return 0;
+  return flock(fd, LOCK_UN);
 #endif
 }
 
 COMPILER_RT_VISIBILITY FILE *lprofOpenFileEx(const char *ProfileName) {
   FILE *f;
   int fd;
 #ifdef COMPILER_RT_HAS_FCNTL_LCK
   fd = open(ProfileName, O_RDWR | O_CREAT, 0666);

test/profile/Inputs/instrprof-gcov-prefix.c

+int main() {
+    return 0;
+}

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

+RUN: mkdir -p %t.d
+RUN: cd %t.d
+
+RUN: %clang --coverage -o %t %S/../Inputs/instrprof-gcov-prefix.c
+RUN: test -f instrprof-gcov-prefix.gcno
+
+RUN: rm -fr %t.d/dir1
+RUN: GCOV_PREFIX=%t.d/dir1/dir2/dir3 %run %t
+RUN: GCOV_PREFIX=%t.d/dir1/dir2/dir3 %run %t
+RUN: test -f %t.d/dir1/dir2/dir3/`pwd`/instrprof-gcov-prefix.gcda
+RUN: rm -fr %t.d/dir1