This is an archive of the discontinued LLVM Phabricator instance.

[gcov] Fix simultaneous .gcda creation
ClosedPublic

Authored by kawashima-fj on Mar 15 2020, 6:27 PM.

Details

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.

Diff Detail

Event Timeline

kawashima-fj created this revision.Mar 15 2020, 6:27 PM
Herald added subscribers: Restricted Project, jfb. · View Herald TranscriptMar 15 2020, 6:27 PM

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
void added inline comments.Mar 23 2020, 12:14 PM
compiler-rt/lib/profile/GCDAProfiling.c
378

Is this comment still necessary?

marco-c accepted this revision.Mar 23 2020, 5:37 PM
marco-c added inline comments.
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).

This revision is now accepted and ready to land.Mar 23 2020, 5:37 PM

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

This revision was automatically updated to reflect the committed changes.

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