<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 6, 2016 at 10:06 AM, Vedant Kumar <span dir="ltr"><<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi David,<br>
<br>
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.<br>
<br>
A few comments inline --<br>
<br>
vedant<br>
<br>
<br>
> On Jun 5, 2016, at 8:18 PM, Xinliang David Li via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
><br>
> Author: davidxl<br>
> Date: Sun Jun 5 22:17:58 2016<br>
> New Revision: 271864<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=271864&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=271864&view=rev</a><br>
> Log:<br>
> [profile] in-process mergeing support (part-2)<br>
><br>
> (Part-1 merging API is in profile runtime)<br>
><br>
> This patch implements a portable file opening API<br>
> with exclusive access for the process. In-process<br>
> profile merge requires profile file update to be<br>
> atomic/fully sychronized.<br>
><br>
><br>
><br>
> Added:<br>
> compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c<br>
> compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test<br>
> Modified:<br>
> compiler-rt/trunk/lib/profile/CMakeLists.txt<br>
> compiler-rt/trunk/lib/profile/InstrProfilingUtil.c<br>
> compiler-rt/trunk/lib/profile/InstrProfilingUtil.h<br>
><br>
> Modified: compiler-rt/trunk/lib/profile/CMakeLists.txt<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/CMakeLists.txt?rev=271864&r1=271863&r2=271864&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/CMakeLists.txt?rev=271864&r1=271863&r2=271864&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/profile/CMakeLists.txt (original)<br>
> +++ compiler-rt/trunk/lib/profile/CMakeLists.txt Sun Jun 5 22:17:58 2016<br>
> @@ -22,6 +22,22 @@ int main() {<br>
> }<br>
> " COMPILER_RT_TARGET_HAS_ATOMICS)<br>
><br>
> +CHECK_CXX_SOURCE_COMPILES("<br>
> +#if defined(__linux__)<br>
> +#include <unistd.h><br>
> +#endif<br>
> +#include <fcntl.h><br>
> +int fd;<br>
> +int main() {<br>
> + struct flock s_flock;<br>
> +<br>
> + s_flock.l_type = F_WRLCK;<br>
> + fcntl(fd, F_SETLKW, &s_flock);<br>
> + return 0;<br>
> +}<br>
> +<br>
> +" COMPILER_RT_TARGET_HAS_FCNTL_LCK)<br>
> +<br>
> add_custom_target(profile)<br>
><br>
> set(PROFILE_SOURCES<br>
> @@ -55,6 +71,12 @@ if(COMPILER_RT_TARGET_HAS_ATOMICS)<br>
> -DCOMPILER_RT_HAS_ATOMICS=1)<br>
> endif()<br>
><br>
> +if(COMPILER_RT_TARGET_HAS_FCNTL_LCK)<br>
> + set(EXTRA_FLAGS<br>
> + ${EXTRA_FLAGS}<br>
> + -DCOMPILER_RT_HAS_FCNTL_LCK=1)<br>
> +endif()<br>
> +<br>
> if(APPLE)<br>
> add_compiler_rt_runtime(clang_rt.profile<br>
> STATIC<br>
><br>
> Modified: compiler-rt/trunk/lib/profile/InstrProfilingUtil.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingUtil.c?rev=271864&r1=271863&r2=271864&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingUtil.c?rev=271864&r1=271863&r2=271864&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/profile/InstrProfilingUtil.c (original)<br>
> +++ compiler-rt/trunk/lib/profile/InstrProfilingUtil.c Sun Jun 5 22:17:58 2016<br>
> @@ -12,9 +12,15 @@<br>
><br>
> #ifdef _WIN32<br>
> #include <direct.h><br>
> +#include <windows.h><br>
> #else<br>
> #include <sys/stat.h><br>
> #include <sys/types.h><br>
> +#if defined(__linux__)<br>
> +#include <unistd.h><br>
> +#endif<br>
> +#include <fcntl.h><br>
> +#include <errno.h><br>
> #endif<br>
><br>
> #ifdef COMPILER_RT_HAS_UNAME<br>
> @@ -35,7 +41,7 @@ void __llvm_profile_recursive_mkdir(char<br>
> #ifdef _WIN32<br>
> _mkdir(path);<br>
> #else<br>
> - mkdir(path, 0755); /* Some of these will fail, ignore it. */<br>
> + mkdir(path, 0755); /* Some of these will fail, ignore it. */<br>
> #endif<br>
> path[i] = save;<br>
> }<br>
> @@ -70,4 +76,51 @@ int lprofGetHostName(char *Name, int Len<br>
> }<br>
> #endif<br>
><br>
> +FILE *lprofOpenFileEx(const char *ProfileName) {<br>
> + FILE *f;<br>
> + int fd;<br>
> +#ifdef COMPILER_RT_HAS_FCNTL_LCK<br>
> + struct flock s_flock;<br>
> +<br>
> + s_flock.l_whence = SEEK_SET;<br>
> + s_flock.l_start = 0;<br>
> + s_flock.l_len = 0; /* Until EOF. */<br>
> + s_flock.l_pid = getpid();<br>
> +<br>
> + s_flock.l_type = F_WRLCK;<br>
> + fd = open(ProfileName, O_RDWR | O_CREAT, 0666);<br>
> + if (fd < 0)<br>
> + return 0;<br>
<br>
Could you use NULL?<br></blockquote><div><br></div><div>Ok.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> +<br>
> + while (fcntl(fd, F_SETLKW, &s_flock) && errno == EINTR)<br>
> + continue;<br>
<br>
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.<br>
<br></blockquote><div><br></div><div>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</div><div><br></div><div> while (fcntl(fd, ....) == -1) {</div><div> if (errno != EINTR)</div><div> break;</div><div> }</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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?<br>
<br></blockquote><div><br></div><div><br></div><div>I will add a warning for ENOLCK case.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + f = fdopen(fd, "r+b");<br>
> +#elif defined(_WIN32)<br>
> + HANDLE h = CreateFile(ProfileName, GENERIC_READ | GENERIC_WRITE, 0, 0,<br>
> + OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0);<br>
> + if (h == INVALID_HANDLE_VALUE)<br>
> + return 0;<br>
<br>
Ditto, please use NULL for clarity.<br>
<br></blockquote><div><br></div><div>OK.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + fd = _open_osfhandle((intptr_t)h, 0);<br>
> + if (fd == -1) {<br>
> + CloseHandle(h);<br>
> + return 0;<br>
<br>
Here too ^<br>
<br>
<br>
> + }<br>
><br>
> + f = _fdopen(fd, "r+b");<br>
> + if (f == 0) {<br>
> + CloseHandle(h);<br>
> + return 0;<br>
<br>
Here too ^<br>
<br>
<br>
> + }<br>
> +#else<br>
> + /* Worst case no locking applied. */<br>
> + PROF_WARN("Concurrent file access is not supported : %s\n", "lack file locking");<br>
> + fd = open(ProfileName, O_RDWR | O_CREAT, 0666);<br>
> + if (fd < 0)<br>
> + return 0;<br>
<br>
Here too ^<br>
<br>
<br>
> + f = fdopen(fd, "r+b");<br>
> +#endif<br>
> +<br>
> + return f;<br>
> +}<br>
><br>
> Modified: compiler-rt/trunk/lib/profile/InstrProfilingUtil.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingUtil.h?rev=271864&r1=271863&r2=271864&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingUtil.h?rev=271864&r1=271863&r2=271864&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/profile/InstrProfilingUtil.h (original)<br>
> +++ compiler-rt/trunk/lib/profile/InstrProfilingUtil.h Sun Jun 5 22:17:58 2016<br>
> @@ -11,10 +11,15 @@<br>
> #define PROFILE_INSTRPROFILINGUTIL_H<br>
><br>
> #include <stddef.h><br>
> +#include <stdio.h><br>
><br>
> /*! \brief Create a directory tree. */<br>
> void __llvm_profile_recursive_mkdir(char *Pathname);<br>
><br>
> +/*! Open file \c Filename for read+write with write<br>
> + * lock for exclusive access. The caller will block<br>
> + * if the lock is already held by another process. */<br>
> +FILE *lprofOpenFileEx(const char *Filename);<br>
> /* PS4 doesn't have getenv. Define a shim. */<br>
> #if __ORBIS__<br>
> static inline char *getenv(const char *name) { return NULL; }<br>
><br>
> Added: compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c?rev=271864&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c?rev=271864&view=auto</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c (added)<br>
> +++ compiler-rt/trunk/test/profile/Inputs/instrprof-file_ex.c Sun Jun 5 22:17:58 2016<br>
> @@ -0,0 +1,59 @@<br>
> +/* This is a test case where the parent process forks 10<br>
> + * children which contend to write to the same file. With<br>
> + * file locking support, the data from each child should not<br>
> + * be lost.<br>
> + */<br>
> +#include <stdio.h><br>
> +#include <stdlib.h><br>
> +#include <unistd.h><br>
> +#include <sys/wait.h><br>
> +<br>
> +extern FILE *lprofOpenFileEx(const char *);<br>
> +int main(int argc, char *argv[]) {<br>
> + pid_t tid;<br>
> + FILE *F;<br>
> + const char *FN;<br>
> + int child[10];<br>
> + int c;<br>
> + int i;<br>
> +<br>
> + if (argc < 2) {<br>
> + fprintf(stderr, "Requires one argument \n");<br>
> + exit(1);<br>
> + }<br>
> + FN = argv[1];<br>
> + truncate(FN, 0);<br>
<br>
Why are the truncations and file-seeks necessary?<br>
<br></blockquote><div><br></div><div>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.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + for (i = 0; i < 10; i++) {<br>
> + c = fork();<br>
> + // in child:<br>
> + if (c == 0) {<br>
> + FILE *F = lprofOpenFileEx(FN);<br>
> + if (!F) {<br>
> + fprintf(stderr, "Can not open file %s from child\n", FN);<br>
> + exit(1);<br>
> + }<br>
> + fseek(F, 0, SEEK_END);<br>
> + fprintf(F, "Dump from Child %d\n", i + 11);<br>
<br>
Why the `+ 11` here?<br></blockquote><div><br></div><div><br></div><div>No special meaning. I can remove it for clarity.</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
> + fclose(F);<br>
> + exit(0);<br>
> + } else {<br>
> + child[i] = c;<br>
> + }<br>
> + }<br>
> +<br>
> + // In parent<br>
> + for (i = 0; i < 10; i++) {<br>
> + int child_status;<br>
> + if ((tid = waitpid(child[i], &child_status, 0) == -1))<br>
> + break;<br>
> + }<br>
> + F = lprofOpenFileEx(FN);<br>
> + if (!F) {<br>
> + fprintf(stderr, "Can not open file %s from parent\n", FN);<br>
> + exit(1);<br>
> + }<br>
> + fseek(F, 0, SEEK_END);<br>
> + fprintf(F, "Dump from parent %d\n", i + 11);<br>
> + return 0;<br>
> +}<br>
><br>
> Added: compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test?rev=271864&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test?rev=271864&view=auto</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test (added)<br>
> +++ compiler-rt/trunk/test/profile/Linux/instrprof-file_ex.test Sun Jun 5 22:17:58 2016<br>
> @@ -0,0 +1,16 @@<br>
> +RUN: mkdir -p %t.d<br>
> +RUN: %clang_profgen -fprofile-instr-generate %S/../Inputs/instrprof-file_ex.c -o %t<br>
> +RUN: %run %t %t.d/run.dump<br>
> +RUN: sort %t.d/run.dump | FileCheck %s<br>
> +<br>
> +CHECK: Dump from Child 11<br>
> +CHECK-NEXT: Dump from Child 12<br>
> +CHECK-NEXT: Dump from Child 13<br>
> +CHECK-NEXT: Dump from Child 14<br>
> +CHECK-NEXT: Dump from Child 15<br>
> +CHECK-NEXT: Dump from Child 16<br>
> +CHECK-NEXT: Dump from Child 17<br>
> +CHECK-NEXT: Dump from Child 18<br>
> +CHECK-NEXT: Dump from Child 19<br>
> +CHECK-NEXT: Dump from Child 20<br>
> +CHECK-NEXT: Dump from parent 21<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></div><br></div></div>