[gcov] Fix simultaneous .gcda creation

[kawashima-fj]

Summary

The intent of the llvm_gcda_start_file function is that only
one process create the .gcda file and initialize it to be updated
by other processes later.

Before this change, if multiple processes are started simultaneously,
some of them may initialize the file because both the first and
second open calls may succeed in a race condition and new_file
becomes 1 in those processes. This leads incorrect coverage counter
values. This often happens in MPI (Message Passing Interface) programs.
The test program added in this change is a simple reproducer.

This change ensures only one process creates/initializes the file by
using the O_EXCL flag.

[kawashima-fj]

The failure of the pre-merge check is the following clang-tidy warning. I modified only indentation of the line, and compiler-rt does not follow variable naming rules in the LLVM coding standard. So it does not have a problem.

compiler-rt/lib/profile/GCDAProfiling.c:364:15:
warning: invalid case style for variable 'errnum' [readability-identifier-naming]
          int errnum = errno;
              ^~~~~~
              Errnum

[kawashima-fj]

Ping.

[void]

compiler-rt/lib/profile/GCDAProfiling.c

378

Is this comment still necessary?

[marco-c]

compiler-rt/lib/profile/GCDAProfiling.c

351–356

Nit: I think the code would be easier to read if you inverted the conditions (if (fd != -1) {)

378

Yeah, it should still be necessary to serialize writing to the GCDA files (the patch only makes the creation of the file atomic).

[kawashima-fj]

Thanks. I updated the patch to inverted the conditions (if (fd != -1) {).

[marco-c]

I wonder if this fixed the scenario from https://reviews.llvm.org/D54599 too

compiler-rt/lib/profile/GCDAProfiling.c

void llvm_gcda_start_file(const char *orig_filename, const char version[4],
   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);
 
   if (fd == -1) {
-    /* Try opening the file, creating it if necessary. */
+    /* Try creating the file. */
+    fd = open(filename, O_RDWR | O_CREAT | O_EXCL | O_BINARY, 0644);
+    if (fd != -1) {
       new_file = 1;
       mode = "w+b";
-    fd = open(filename, O_RDWR | O_CREAT | O_BINARY, 0644);
-    if (fd == -1) {
+    } else {
       /* Try creating the directories first then opening the file. */
       __llvm_profile_recursive_mkdir(filename);
-      fd = open(filename, O_RDWR | O_CREAT | O_BINARY, 0644);
+      fd = open(filename, O_RDWR | O_CREAT | O_EXCL | O_BINARY, 0644);
+      if (fd != -1) {
+        new_file = 1;
+        mode = "w+b";
+      } else {
+        /* Another process may have created the file just now.
+         * Try opening it without O_CREAT and O_EXCL. */
+        fd = open(filename, O_RDWR | O_BINARY);
         if (fd == -1) {
           /* Bah! It's hopeless. */
           int errnum = errno;
           fprintf(stderr, "profiling: %s: cannot open: %s\n", filename,
                   strerror(errnum));
           return;
         }
       }
     }
+  }
 
   /* 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);
 
   /* Initialize the write buffer. */
   write_buffer = NULL;

compiler-rt/test/profile/Inputs/instrprof-gcov-parallel.driver.c

+#include <stdatomic.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#define CHILDREN 7
+
+int main(int argc, char *argv[]) {
+  _Atomic int *sync = mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE,
+                           MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+  if (sync == MAP_FAILED)
+    return 1;
+  *sync = 0;
+
+  for (int i = 0; i < CHILDREN; i++) {
+    pid_t pid = fork();
+    if (!pid) {
+      // child
+      while (*sync == 0)
+        ; // wait the parent in order to call execl simultaneously
+      execl(argv[1], argv[1], NULL);
+    } else if (pid == -1) {
+      *sync = 1; // release all children
+      return 1;
+    }
+  }
+
+  // parent
+  *sync = 1; // start the program in all children simultaneously
+  for (int i = 0; i < CHILDREN; i++)
+    wait(NULL);
+
+  return 0;
+}

compiler-rt/test/profile/Inputs/instrprof-gcov-parallel.target.c

+#define COUNT 101
+
+static volatile int aaa;
+
+int main(int argc, char *argv[]) {
+  for (int i = 0; i < COUNT; i++)
+    aaa++;
+  return 0;
+}

compiler-rt/test/profile/Posix/instrprof-gcov-parallel.test

+RUN: mkdir -p %t.d
+RUN: cd %t.d
+
+RUN: %clang -o %t.driver %S/../Inputs/instrprof-gcov-parallel.driver.c
+RUN: %clang --coverage -o %t.target %S/../Inputs/instrprof-gcov-parallel.target.c
+RUN: test -f instrprof-gcov-parallel.target.gcno
+
+RUN: rm -f instrprof-gcov-parallel.target.gcda
+RUN: %run %t.driver %t.target
+RUN: llvm-cov gcov instrprof-gcov-parallel.target.gcda
+RUN: FileCheck --input-file instrprof-gcov-parallel.target.c.gcov %s
+
+# Test if the .gcda file is correctly created from one of child processes
+# and counters of all processes are recorded correctly.
+# 707 = CHILDREN * COUNT
+CHECK: 707: {{[0-9]+}}: aaa++;