[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

348–349

Why try to create the file again?

[calixte]

lib/profile/GCDAProfiling.c

348–349

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

348–349

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

348–349

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

344

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

348–349

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

350

Nit: another instead of an other

350

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

12

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

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";
   filename = mangle_filename(orig_filename);
-
-  /* Try just opening the file. */
   new_file = 0;
-  fd = open(filename, O_RDWR | O_BINARY);
+  fd = lprofOpenFile(filename, &new_file);
 
   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);
-    if (fd == -1) {
-      /* 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;
-      }
-    }
-  }
+            strerror(errno));
 
   /* 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);
-  output_file = fdopen(fd, mode);
+  if (lprofLockFd(fd) != 0) {
+    fprintf(stderr, "profiling: %s: cannot lock: %s\n", filename,
+            strerror(errno));
+  }
+  output_file = fdopen(fd, new_file ? "w+b" : "r+b");
 
   /* Initialize the write buffer. */
   write_buffer = NULL;
   cur_buffer_size = 0;
   cur_pos = 0;
 
   if (new_file) {
     resize_write_buffer(WRITE_BUFFER_SIZE);

lib/profile/InstrProfilingUtil.h

 /*! Set the mode used when creating profile directories. */
 void __llvm_profile_set_dir_mode(unsigned Mode);
 
 /*! Return the directory creation mode. */
 unsigned __llvm_profile_get_dir_mode(void);
 
 int lprofLockFd(int fd);
 int lprofUnlockFd(int fd);
+int lprofOpenOrCreateFile(const char *filename, int *newfile);
+int lprofOpenFile(char *filename, int *newfile);
 
 /*! Open file \c Filename for read+write with write
  * lock for exclusive access. The caller will block
  * if the lock is already held by another process. */
 FILE *lprofOpenFileEx(const char *Filename);
 /* PS4 doesn't have getenv. Define a shim. */
 #if __ORBIS__
 static inline char *getenv(const char *name) { return NULL; }

lib/profile/InstrProfilingUtil.c

 /*===- InstrProfilingUtil.c - Support library for PGO instrumentation -----===*\
 |*
 |*                     The LLVM Compiler Infrastructure
 |*
 |* This file is distributed under the University of Illinois Open Source
 |* License. See LICENSE.TXT for details.
 |*
 \*===----------------------------------------------------------------------===*/
 
+#include <fcntl.h>
+
 #ifdef _WIN32
 #include <direct.h>
 #include <process.h>
 #include <windows.h>
 #include "WindowsMMap.h"
 #else
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
-#include <fcntl.h>
 #include <errno.h>
 #endif
 
 #ifdef COMPILER_RT_HAS_UNAME
 #include <sys/utsname.h>
 #endif
 
 #include <stdlib.h>
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);

 #endif
 }
 
 COMPILER_RT_VISIBILITY void lprofRestoreSigKill() {
 #if defined(__linux__)
   prctl(PR_SET_PDEATHSIG, SIGKILL);
 #endif
 }
+
+COMPILER_RT_VISIBILITY
+int lprofOpenOrCreateFile(const char *filename, int *newfile) {
+  int fd = open(filename, O_RDWR | O_CREAT | O_EXCL | O_BINARY, 0644);
+  if (fd == -1 && errno == EEXIST) {
+    return open(filename, O_RDWR | O_BINARY);
+  }
+  *newfile = 1;
+  return fd;
+}
+
+COMPILER_RT_VISIBILITY
+int lprofOpenFile(char *filename, int *newfile) {
+  int fd = lprofOpenOrCreateFile(filename, newfile);
+  if (fd == -1) {
+    /* Try creating the directories first then opening the file. */
+    __llvm_profile_recursive_mkdir(filename);
+    *newfile = 0;
+    return lprofOpenOrCreateFile(filename, newfile);
+  }
+  return fd;
+}

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: rm -f instrprof-gcov-prefix.gcno instrprof-gcov-prefix.gcda
+RUN: %clang --coverage -o %t %S/../Inputs/instrprof-gcov-prefix.c
+RUN: test -f instrprof-gcov-prefix.gcno
+RUN: %run %t
+RUN: test -f instrprof-gcov-prefix.gcda
+RUN: rm -f instrprof-gcov-prefix.gcno instrprof-gcov-prefix.gcda
+
+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 instrprof-gcov-prefix.gcda
+RUN: test -f %t.d/dir1/dir2/dir3/`pwd`/instrprof-gcov-prefix.gcda
+RUN: rm -fr %t.d/dir1