<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Feb 27, 2016 at 10:43 AM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Fri, Feb 26, 2016 at 11:53 PM, Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">silvas created this revision.<br>
silvas added reviewers: davidxl, MaggieYi, phillip.power, filcab.<br>
silvas added subscribers: llvm-commits, probinson, slingn, simon.f.whittaker.<br>
<br>
Hi David, SCE folks,<br>
<br>
What is implemented in this patch is enough for the upstream libprofile to<br>
work for PGO with the PS4 game codebase I tested ("game7" for you SCE<br>
folks; this is with a standalone build of compiler-rt).<br>
<br>
The first change, which is simple, is to stub out gethostname. PS4<br>
doesn't have a simple analog for this that doesn't bring in extra<br>
OS libraries, so for now we do not support `%h` expansion.<br>
This is consistent with internal B#136272.<br>
<br>
The second change implies future work, but is a simple change at present.<br>
PS4 does not have `getenv`, so for now we will introduce a shim.<br>
This obviously makes it impossible for many of the tests to be run since<br>
they require setting `LLVM_PROFILE_FILE=`.<br>
<br>
I see two paths forward:<br>
<br>
1. In the tests we are already wrapping execution with `%run` and so by<br>
setting a PS4-specific expansion for `%run` we can pass the information<br>
in another way We can adapt the getenv shim as appropriate.<br>
We will need to experiment with this internally.<br>
Maggie, Phillip, Filipe? Any ideas? Maybe ping me internally since we<br>
may need to get into some PS4 vagaries. I'm thinking a fake getenv<br>
library that uses some side channel for communication.<br>
<br>
2. Another possibility which is more verbose is to use a separate clang<br>
invocation with `-profile-generate=<filename>` to set the filename in<br>
each test.<br>
This might require redundant clang invocations though which may be<br>
undesirable for upstream. David, thoughts?<br>
Also, this is a fairly libprofile-specific workaround, so it e.g.<br>
doesn't help Filipe's ASan work.<br>
Overall, this approach sounds like a bit of a hack to me.<br></blockquote><div><br></div></div></div><div>One possibility is detect this in cmake and in lit.cfg, set clang_profgen to use -fprofile-instr-generate= form. </div></div></div></div></blockquote><div><br></div><div>Great. We do something similar for `%clang_profuse=%t.profraw` and so we could do the same thing.</div><div><br></div><div>The limitation I was imagining was that we cannot run the same binary with different LLVM_PROFILE_FILE settings (or other settings). An example is: <a href="http://llvm.org/klaus/compiler-rt/blob/master/test/profile/instrprof-value-prof.c#L-11">http://llvm.org/klaus/compiler-rt/blob/master/test/profile/instrprof-value-prof.c#L-11</a></div><div><br></div><div>But upon closer inspection, not many tests seem to do that. Would the extra clang invocations in such tests be acceptable?</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Small detail:<br>
InstrProfilingPort.h seems like the natural place for the getenv shim,<br>
but GCDAProfiling.c needs it as well. InstrProfilingUtil.h is currently<br>
the only header common between InstrProfilingFile.c and GCDAProfiling.c.<br>
I can move the shim to InstrProfilingPort.h and add an include to<br>
GCDAProfiling.c as per your preference David.<br>
<br>
<a href="http://reviews.llvm.org/D17676" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17676</a><br>
<br>
Files:<br>
lib/profile/InstrProfilingPort.h<br>
lib/profile/InstrProfilingUtil.h<br>
<br>
Index: lib/profile/InstrProfilingUtil.h<br>
===================================================================<br>
--- lib/profile/InstrProfilingUtil.h<br>
+++ lib/profile/InstrProfilingUtil.h<br>
@@ -13,4 +13,9 @@<br>
/*! \brief Create a directory tree. */<br>
void __llvm_profile_recursive_mkdir(char *Pathname);<br>
<br>
+/* PS4 doesn't have getenv. Define a shim. */<br>
+#if __PS4__<br>
+static inline char *getenv(const char *name) { return 0; }<br>
+#endif /* #if __PS4__ */<br>
+<br>
#endif /* PROFILE_INSTRPROFILINGUTIL_H */<br>
Index: lib/profile/InstrProfilingPort.h<br>
===================================================================<br>
--- lib/profile/InstrProfilingPort.h<br>
+++ lib/profile/InstrProfilingPort.h<br>
@@ -25,6 +25,8 @@<br>
#define COMPILER_RT_MAX_HOSTLEN 128<br>
#ifdef _MSC_VER<br>
#define COMPILER_RT_GETHOSTNAME(Name, Len) gethostname(Name, Len)<br>
+#elif defined(__PS4__)<br>
+#define COMPILER_RT_GETHOSTNAME(Name, Len) (-1)<br>
#else<br>
#define COMPILER_RT_GETHOSTNAME(Name, Len) GetHostName(Name, Len)<br>
#define COMPILER_RT_HAS_UNAME 1<br>
<br>
<br></blockquote></div></div><div>This looks fine.</div><div><br></div><div>thanks,</div><div><br></div><div>David </div></div><br></div></div>
</blockquote></div><br></div></div>