[PATCH] D54599: [Profile] Avoid race condition when dumping GCDA files.
Marco Castelluccio via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 19 01:36:38 PST 2018
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
More information about the llvm-commits
mailing list