[compiler-rt] r271864 - [profile] in-process mergeing support (part-2)
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 6 10:06:48 PDT 2016
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?
> +
> + 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.
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?
> +
> + 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.
> +
> + 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?
> +
> + 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?
> + 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
More information about the llvm-commits
mailing list