<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 23, 2016 at 8:15 PM, 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">Yeah, this test will break if %run attempts remote execution.<br>
<br>
Here's an idea for a less brittle (and way less ambitious) test:<br>
<br>
```<br>
// test-hostname-expansion.c<br>
<br>
// RUN: %cc_profgen test-hostname-expansion.c -o %t.exe<br>
// RUN: LLVM_PROFILE_FILE="%t.%h.profraw" %run %t.exe "%t.%h.profraw"<br>
// RUN: rm %t.`uname -n`.profraw<br>
<br>
// REQUIRES: shell<br>
<br>
int main(int argc, char **argv) {<br>
__llvm_profile_set_filename(argv[1]);<br>
return 0;<br>
}<br>
```<br>
<br>
I.e, give up on the idea of actually testing remote execution, and just check that the hostname expansion is faithful.<br></blockquote><div><br></div><div>Faithful to what? %h is fundamentally expanded on the target. `uname -n` occurs on the host. They are not necessarily related in any way.</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Unrelated to Sean's point: why is "%clang_profuse=%t.profdata" in the test? I don't think it adds anything.<br>
<br>
WDYT?<br>
<br>
thanks,<br>
vedant<br>
<div class="HOEnZb"><div class="h5"><br>
> On Jun 23, 2016, at 7:48 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
><br>
> Hi Vedant, Daniel,<br>
><br>
> The test case in this patch is fundamentally flawed: it runs `uname -n` on the host, while `%h` expands on the target. No relation between them can be assumed.<br>
><br>
> Unfortunately, I don't really have a concrete suggestion for improving the test case: we would have to have some sort of special knowledge of what %run is actually configured to do in order to "verify" anything about the filename.<br>
><br>
> -- Sean Silva<br>
><br>
> On Fri, Jan 29, 2016 at 3:52 PM, Vedant Kumar via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: vedantk<br>
> Date: Fri Jan 29 17:52:11 2016<br>
> New Revision: 259272<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=259272&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=259272&view=rev</a><br>
> Log:<br>
> [profile] Support hostname expansion in LLVM_PROFILE_FILE<br>
><br>
> This patch adds support for expanding "%h" out to the machine hostname<br>
> in the LLVM_PROFILE_FILE environment variable.<br>
><br>
> Patch by Daniel Waters!<br>
><br>
> Differential Revision: <a href="http://reviews.llvm.org/D16371" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16371</a><br>
><br>
> Added:<br>
> compiler-rt/trunk/test/profile/instrprof-hostname.c<br>
> Modified:<br>
> compiler-rt/trunk/lib/profile/InstrProfilingFile.c<br>
> compiler-rt/trunk/lib/profile/InstrProfilingPort.h<br>
><br>
> Modified: compiler-rt/trunk/lib/profile/InstrProfilingFile.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingFile.c?rev=259272&r1=259271&r2=259272&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingFile.c?rev=259272&r1=259271&r2=259272&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/profile/InstrProfilingFile.c (original)<br>
> +++ compiler-rt/trunk/lib/profile/InstrProfilingFile.c Fri Jan 29 17:52:11 2016<br>
> @@ -14,9 +14,22 @@<br>
> #include <stdio.h><br>
> #include <stdlib.h><br>
> #include <string.h><br>
> +#ifdef COMPILER_RT_HAS_UNAME<br>
> +#include <sys/utsname.h><br>
> +#endif<br>
><br>
> #define UNCONST(ptr) ((void *)(uintptr_t)(ptr))<br>
><br>
> +#ifdef COMPILER_RT_HAS_UNAME<br>
> +int GetHostName(char *Name, int Len) {<br>
> + struct utsname N;<br>
> + int R;<br>
> + if (!(R = uname(&N)))<br>
> + strncpy(Name, N.nodename, Len);<br>
> + return R;<br>
> +}<br>
> +#endif<br>
> +<br>
> /* Return 1 if there is an error, otherwise return 0. */<br>
> static uint32_t fileWriter(ProfDataIOVec *IOVecs, uint32_t NumIOVecs,<br>
> void **WriterCtx) {<br>
> @@ -114,9 +127,10 @@ int getpid(void);<br>
> static int setFilenamePossiblyWithPid(const char *Filename) {<br>
> #define MAX_PID_SIZE 16<br>
> char PidChars[MAX_PID_SIZE] = {0};<br>
> - int NumPids = 0, PidLength = 0;<br>
> + int NumPids = 0, PidLength = 0, NumHosts = 0, HostNameLength = 0;<br>
> char *Allocated;<br>
> int I, J;<br>
> + char Hostname[COMPILER_RT_MAX_HOSTLEN];<br>
><br>
> /* Reset filename on NULL, except with env var which is checked by caller. */<br>
> if (!Filename) {<br>
> @@ -126,19 +140,29 @@ static int setFilenamePossiblyWithPid(co<br>
><br>
> /* Check the filename for "%p", which indicates a pid-substitution. */<br>
> for (I = 0; Filename[I]; ++I)<br>
> - if (Filename[I] == '%' && Filename[++I] == 'p')<br>
> - if (!NumPids++) {<br>
> - PidLength = snprintf(PidChars, MAX_PID_SIZE, "%d", getpid());<br>
> - if (PidLength <= 0)<br>
> - return -1;<br>
> + if (Filename[I] == '%') {<br>
> + if (Filename[++I] == 'p') {<br>
> + if (!NumPids++) {<br>
> + PidLength = snprintf(PidChars, MAX_PID_SIZE, "%d", getpid());<br>
> + if (PidLength <= 0)<br>
> + return -1;<br>
> + }<br>
> + } else if (Filename[I] == 'h') {<br>
> + if (!NumHosts++)<br>
> + if (COMPILER_RT_GETHOSTNAME(Hostname, COMPILER_RT_MAX_HOSTLEN))<br>
> + return -1;<br>
> + HostNameLength = strlen(Hostname);<br>
> }<br>
> - if (!NumPids) {<br>
> + }<br>
> +<br>
> + if (!(NumPids || NumHosts)) {<br>
> setFilename(Filename, 0);<br>
> return 0;<br>
> }<br>
><br>
> /* Allocate enough space for the substituted filename. */<br>
> - Allocated = malloc(I + NumPids*(PidLength - 2) + 1);<br>
> + Allocated = malloc(I + NumPids*(PidLength - 2) +<br>
> + NumHosts*(HostNameLength - 2) + 1);<br>
> if (!Allocated)<br>
> return -1;<br>
><br>
> @@ -149,6 +173,10 @@ static int setFilenamePossiblyWithPid(co<br>
> memcpy(Allocated + J, PidChars, PidLength);<br>
> J += PidLength;<br>
> }<br>
> + else if (Filename[I] == 'h') {<br>
> + memcpy(Allocated + J, Hostname, HostNameLength);<br>
> + J += HostNameLength;<br>
> + }<br>
> /* Drop any unknown substitutions. */<br>
> } else<br>
> Allocated[J++] = Filename[I];<br>
><br>
> Modified: compiler-rt/trunk/lib/profile/InstrProfilingPort.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingPort.h?rev=259272&r1=259271&r2=259272&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingPort.h?rev=259272&r1=259271&r2=259272&view=diff</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/lib/profile/InstrProfilingPort.h (original)<br>
> +++ compiler-rt/trunk/lib/profile/InstrProfilingPort.h Fri Jan 29 17:52:11 2016<br>
> @@ -22,6 +22,14 @@<br>
><br>
> #define COMPILER_RT_SECTION(Sect) __attribute__((section(Sect)))<br>
><br>
> +#define COMPILER_RT_MAX_HOSTLEN 128<br>
> +#ifdef _MSC_VER<br>
> +#define COMPILER_RT_GETHOSTNAME(Name, Len) gethostname(Name, Len)<br>
> +#else<br>
> +#define COMPILER_RT_GETHOSTNAME(Name, Len) GetHostName(Name, Len)<br>
> +#define COMPILER_RT_HAS_UNAME 1<br>
> +#endif<br>
> +<br>
> #if COMPILER_RT_HAS_ATOMICS == 1<br>
> #ifdef _MSC_VER<br>
> #include <windows.h><br>
><br>
> Added: compiler-rt/trunk/test/profile/instrprof-hostname.c<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/instrprof-hostname.c?rev=259272&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/profile/instrprof-hostname.c?rev=259272&view=auto</a><br>
> ==============================================================================<br>
> --- compiler-rt/trunk/test/profile/instrprof-hostname.c (added)<br>
> +++ compiler-rt/trunk/test/profile/instrprof-hostname.c Fri Jan 29 17:52:11 2016<br>
> @@ -0,0 +1,13 @@<br>
> +// RUN: %clang_profgen -o %t -O3 %s<br>
> +// RUN: env LLVM_PROFILE_FILE=%h.%t-%h.profraw_%h %run %t<br>
> +// RUN: llvm-profdata merge -o %t.profdata `uname -n`.%t-`uname -n`.profraw_`uname -n`<br>
> +// RUN: %clang_profuse=%t.profdata -o - -S -emit-llvm %s | FileCheck %s<br>
> +// REQUIRES: shell<br>
> +<br>
> +int main(int argc, const char *argv[]) {<br>
> + // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD1:[0-9]+]]<br>
> + if (argc > 2)<br>
> + return 1;<br>
> + return 0;<br>
> +}<br>
> +// CHECK: ![[PD1]] = !{!"branch_weights", i32 1, i32 2}<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>
<br>
</div></div></blockquote></div><br></div></div>