[PATCH] D54599: [Profile] Avoid race condition when dumping GCDA files.

[Marco Castelluccio via Phabricator via llvm-commits]

marco-c added a comment.

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.



================
Comment at: lib/profile/GCDAProfiling.c:345
+  const char *mode;
+  int errnum = 0;
   filename = mangle_filename(orig_filename);
----------------
Why do we need errnum? Couldn't we always use errno?


================
Comment at: lib/profile/GCDAProfiling.c:357
     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;
+      errnum = errno;
+      if (errnum == EEXIST) {
----------------
Need to test this on Windows to make sure that errno is set correctly there.


================
Comment at: lib/profile/GCDAProfiling.c:359
+      if (errnum == EEXIST) {
+        /* In the meantime, an other process has created the file. */
+        fd = open(filename, O_RDWR | O_BINARY);
----------------
Nit: `another` instead of `an other`


================
Comment at: lib/profile/GCDAProfiling.c:377
+    } else {
+      new_file = 1;
     }
----------------
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.


================
Comment at: test/profile/Posix/instrprof-gcov-prefix.test:11
+RUN: test -f %t.d/dir1/dir2/dir3/`pwd`/instrprof-gcov-prefix.gcda
+RUN: rm -fr %t.d/dir1
----------------
Could you also test that the gcda file is not created where it usually is when GCOV_PREFIX is not set?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D54599