[compiler-rt] r271864 - [profile] in-process mergeing support (part-2)
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 6 11:17:03 PDT 2016
On Mon, Jun 6, 2016 at 10:06 AM, Vedant Kumar <vsk at apple.com> wrote:
> Hi David,
>
> Thanks for doing this. While this looks good overall, I think it would be
> helpful to have pre-commit review for substantial commits like this one.
>
> A few comments inline --
>
> vedant
>
>
> > On Jun 5, 2016, at 8:18 PM, Xinliang David Li via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > Author: davidxl
> > Date: Sun Jun 5 22:17:58 2016
> > New Revision: 271864
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=271864&view=rev
> > Log:
> > [profile] in-process mergeing support (part-2)
> >
> > (Part-1 merging API is in profile runtime)
> >
> > This patch implements a portable file opening API
> > with exclusive access for the process. In-process
> > profile merge requires profile file update to be
> > atomic/fully sychronized.
> >
> >
> >
> > Added:
> > compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c
> > compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test
> > Modified:
> > compiler-rt/trunk/lib/profile/CMakeLists.txt
> > compiler-rt/trunk/lib/profile/InstrProfilingUtil.c
> > compiler-rt/trunk/lib/profile/InstrProfilingUtil.h
> >
> > Modified: compiler-rt/trunk/lib/profile/CMakeLists.txt
> > URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/CMakeLists.txt?rev=271864&r1=271863&r2=271864&view=diff
> >
> ==============================================================================
> > --- compiler-rt/trunk/lib/profile/CMakeLists.txt (original)
> > +++ compiler-rt/trunk/lib/profile/CMakeLists.txt Sun Jun 5 22:17:58 2016
> > @@ -22,6 +22,22 @@ int main() {
> > }
> > " COMPILER_RT_TARGET_HAS_ATOMICS)
> >
> > +CHECK_CXX_SOURCE_COMPILES("
> > +#if defined(__linux__)
> > +#include <unistd.h>
> > +#endif
> > +#include <fcntl.h>
> > +int fd;
> > +int main() {
> > + struct flock s_flock;
> > +
> > + s_flock.l_type = F_WRLCK;
> > + fcntl(fd, F_SETLKW, &s_flock);
> > + return 0;
> > +}
> > +
> > +" COMPILER_RT_TARGET_HAS_FCNTL_LCK)
> > +
> > add_custom_target(profile)
> >
> > set(PROFILE_SOURCES
> > @@ -55,6 +71,12 @@ if(COMPILER_RT_TARGET_HAS_ATOMICS)
> > -DCOMPILER_RT_HAS_ATOMICS=1)
> > endif()
> >
> > +if(COMPILER_RT_TARGET_HAS_FCNTL_LCK)
> > + set(EXTRA_FLAGS
> > + ${EXTRA_FLAGS}
> > + -DCOMPILER_RT_HAS_FCNTL_LCK=1)
> > +endif()
> > +
> > if(APPLE)
> > add_compiler_rt_runtime(clang_rt.profile
> > STATIC
> >
> > Modified: compiler-rt/trunk/lib/profile/InstrProfilingUtil.c
> > URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingUtil.c?rev=271864&r1=271863&r2=271864&view=diff
> >
> ==============================================================================
> > --- compiler-rt/trunk/lib/profile/InstrProfilingUtil.c (original)
> > +++ compiler-rt/trunk/lib/profile/InstrProfilingUtil.c Sun Jun 5
> 22:17:58 2016
> > @@ -12,9 +12,15 @@
> >
> > #ifdef _WIN32
> > #include <direct.h>
> > +#include <windows.h>
> > #else
> > #include <sys/stat.h>
> > #include <sys/types.h>
> > +#if defined(__linux__)
> > +#include <unistd.h>
> > +#endif
> > +#include <fcntl.h>
> > +#include <errno.h>
> > #endif
> >
> > #ifdef COMPILER_RT_HAS_UNAME
> > @@ -35,7 +41,7 @@ void __llvm_profile_recursive_mkdir(char
> > #ifdef _WIN32
> > _mkdir(path);
> > #else
> > - mkdir(path, 0755); /* Some of these will fail, ignore it. */
> > + mkdir(path, 0755); /* Some of these will fail, ignore it. */
> > #endif
> > path[i] = save;
> > }
> > @@ -70,4 +76,51 @@ int lprofGetHostName(char *Name, int Len
> > }
> > #endif
> >
> > +FILE *lprofOpenFileEx(const char *ProfileName) {
> > + FILE *f;
> > + 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_WRLCK;
> > + fd = open(ProfileName, O_RDWR | O_CREAT, 0666);
> > + if (fd < 0)
> > + return 0;
>
> Could you use NULL?
>
Ok.
>
>
> > +
> > + while (fcntl(fd, F_SETLKW, &s_flock) && errno == EINTR)
> > + continue;
>
> At least on Darwin, fcntl(F_SETLKW) indicates errors by returning -1
> specifically. Any other non-zero return value may not necessarily indicate
> an error.
>
>
FreeBSD and OSX behave slightly different from Linux here, but the second
check make sure only EINTR error will cause the loop to be re-entered.
Anyway, I will change it to
while (fcntl(fd, ....) == -1) {
if (errno != EINTR)
break;
}
> Also, if errno == ENOLCK, it isn't safe to exit this loop and continue on
> to the fdopen. Maybe the right thing to do here is to pull the ``errno ==
> EINTR`` check into a conditional inside of the loop?
>
>
I will add a warning for ENOLCK case.
>
> > +
> > + f = fdopen(fd, "r+b");
> > +#elif defined(_WIN32)
> > + HANDLE h = CreateFile(ProfileName, GENERIC_READ | GENERIC_WRITE, 0, 0,
> > + OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0);
> > + if (h == INVALID_HANDLE_VALUE)
> > + return 0;
>
> Ditto, please use NULL for clarity.
>
>
OK.
>
> > +
> > + fd = _open_osfhandle((intptr_t)h, 0);
> > + if (fd == -1) {
> > + CloseHandle(h);
> > + return 0;
>
> Here too ^
>
>
> > + }
> >
> > + f = _fdopen(fd, "r+b");
> > + if (f == 0) {
> > + CloseHandle(h);
> > + return 0;
>
> Here too ^
>
>
> > + }
> > +#else
> > + /* Worst case no locking applied. */
> > + PROF_WARN("Concurrent file access is not supported : %s\n", "lack
> file locking");
> > + fd = open(ProfileName, O_RDWR | O_CREAT, 0666);
> > + if (fd < 0)
> > + return 0;
>
> Here too ^
>
>
> > + f = fdopen(fd, "r+b");
> > +#endif
> > +
> > + return f;
> > +}
> >
> > Modified: compiler-rt/trunk/lib/profile/InstrProfilingUtil.h
> > URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingUtil.h?rev=271864&r1=271863&r2=271864&view=diff
> >
> ==============================================================================
> > --- compiler-rt/trunk/lib/profile/InstrProfilingUtil.h (original)
> > +++ compiler-rt/trunk/lib/profile/InstrProfilingUtil.h Sun Jun 5
> 22:17:58 2016
> > @@ -11,10 +11,15 @@
> > #define PROFILE_INSTRPROFILINGUTIL_H
> >
> > #include <stddef.h>
> > +#include <stdio.h>
> >
> > /*! \brief Create a directory tree. */
> > void __llvm_profile_recursive_mkdir(char *Pathname);
> >
> > +/*! 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; }
> >
> > Added: compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c
> > URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c?rev=271864&view=auto
> >
> ==============================================================================
> > --- compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c (added)
> > +++ compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c Sun Jun 5
> 22:17:58 2016
> > @@ -0,0 +1,59 @@
> > +/* This is a test case where the parent process forks 10
> > + * children which contend to write to the same file. With
> > + * file locking support, the data from each child should not
> > + * be lost.
> > + */
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <unistd.h>
> > +#include <sys/wait.h>
> > +
> > +extern FILE *lprofOpenFileEx(const char *);
> > +int main(int argc, char *argv[]) {
> > + pid_t tid;
> > + FILE *F;
> > + const char *FN;
> > + int child[10];
> > + int c;
> > + int i;
> > +
> > + if (argc < 2) {
> > + fprintf(stderr, "Requires one argument \n");
> > + exit(1);
> > + }
> > + FN = argv[1];
> > + truncate(FN, 0);
>
> Why are the truncations and file-seeks necessary?
>
>
The file is opened with 'r+b' for reading/merging with the new API.
Without seeking, the previous data will be overwritten. Since we do
seeking, initial truncation is also needed otherwise leftover data in
previous runs may be a problem. Of course we can teach the test to do the
cleanup first.
>
> > +
> > + for (i = 0; i < 10; i++) {
> > + c = fork();
> > + // in child:
> > + if (c == 0) {
> > + FILE *F = lprofOpenFileEx(FN);
> > + if (!F) {
> > + fprintf(stderr, "Can not open file %s from child\n", FN);
> > + exit(1);
> > + }
> > + fseek(F, 0, SEEK_END);
> > + fprintf(F, "Dump from Child %d\n", i + 11);
>
> Why the `+ 11` here?
>
No special meaning. I can remove it for clarity.
David
>
>
> > + fclose(F);
> > + exit(0);
> > + } else {
> > + child[i] = c;
> > + }
> > + }
> > +
> > + // In parent
> > + for (i = 0; i < 10; i++) {
> > + int child_status;
> > + if ((tid = waitpid(child[i], &child_status, 0) == -1))
> > + break;
> > + }
> > + F = lprofOpenFileEx(FN);
> > + if (!F) {
> > + fprintf(stderr, "Can not open file %s from parent\n", FN);
> > + exit(1);
> > + }
> > + fseek(F, 0, SEEK_END);
> > + fprintf(F, "Dump from parent %d\n", i + 11);
> > + return 0;
> > +}
> >
> > Added: compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test
> > URL:
> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test?rev=271864&view=auto
> >
> ==============================================================================
> > --- compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test (added)
> > +++ compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test Sun Jun
> 5 22:17:58 2016
> > @@ -0,0 +1,16 @@
> > +RUN: mkdir -p %t.d
> > +RUN: %clang_profgen -fprofile-instr-generate
> %S/../Inputs/instrprof-file_ex.c -o %t
> > +RUN: %run %t %t.d/run.dump
> > +RUN: sort %t.d/run.dump | FileCheck %s
> > +
> > +CHECK: Dump from Child 11
> > +CHECK-NEXT: Dump from Child 12
> > +CHECK-NEXT: Dump from Child 13
> > +CHECK-NEXT: Dump from Child 14
> > +CHECK-NEXT: Dump from Child 15
> > +CHECK-NEXT: Dump from Child 16
> > +CHECK-NEXT: Dump from Child 17
> > +CHECK-NEXT: Dump from Child 18
> > +CHECK-NEXT: Dump from Child 19
> > +CHECK-NEXT: Dump from Child 20
> > +CHECK-NEXT: Dump from parent 21
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160606/993475f9/attachment-0001.html>
More information about the llvm-commits
mailing list