[PATCH] D17676: Add some minimal portability code paths for PS4.

[Sean Silva via llvm-commits]

On Sat, Feb 27, 2016 at 10:43 AM, Xinliang David Li <davidxl at google.com>
wrote:

>
>
> On Fri, Feb 26, 2016 at 11:53 PM, Sean Silva <chisophugis at gmail.com>
> wrote:
>
>> silvas created this revision.
>> silvas added reviewers: davidxl, MaggieYi, phillip.power, filcab.
>> silvas added subscribers: llvm-commits, probinson, slingn,
>> simon.f.whittaker.
>>
>> Hi David, SCE folks,
>>
>> What is implemented in this patch is enough for the upstream libprofile to
>> work for PGO with the PS4 game codebase I tested ("game7" for you SCE
>> folks; this is with a standalone build of compiler-rt).
>>
>> The first change, which is simple, is to stub out gethostname. PS4
>> doesn't have a simple analog for this that doesn't bring in extra
>> OS libraries, so for now we do not support `%h` expansion.
>> This is consistent with internal B#136272.
>>
>> The second change implies future work, but is a simple change at present.
>> PS4 does not have `getenv`, so for now we will introduce a shim.
>> This obviously makes it impossible for many of the tests to be run since
>> they require setting `LLVM_PROFILE_FILE=`.
>>
>> I see two paths forward:
>>
>> 1. In the tests we are already wrapping execution with `%run` and so by
>>    setting a PS4-specific expansion for `%run` we can pass the information
>>    in another way We can adapt the getenv shim as appropriate.
>>    We will need to experiment with this internally.
>>    Maggie, Phillip, Filipe? Any ideas? Maybe ping me internally since we
>>    may need to get into some PS4 vagaries. I'm thinking a fake getenv
>>    library that uses some side channel for communication.
>>
>> 2. Another possibility which is more verbose is to use a separate clang
>>    invocation with `-profile-generate=<filename>` to set the filename in
>>    each test.
>>    This might require redundant clang invocations though which may be
>>    undesirable for upstream. David, thoughts?
>>    Also, this is a fairly libprofile-specific workaround, so it e.g.
>>    doesn't help Filipe's ASan work.
>>    Overall, this approach sounds like a bit of a hack to me.
>>
>
> One possibility is detect this in cmake and in lit.cfg, set clang_profgen
> to use -fprofile-instr-generate= form.
>

Great. We do something similar for `%clang_profuse=%t.profraw` and so we
could do the same thing.

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:
http://llvm.org/klaus/compiler-rt/blob/master/test/profile/instrprof-value-prof.c#L-11

But upon closer inspection, not many tests seem to do that. Would the extra
clang invocations in such tests be acceptable?

-- Sean Silva


>
>> Small detail:
>> InstrProfilingPort.h seems like the natural place for the getenv shim,
>> but GCDAProfiling.c needs it as well. InstrProfilingUtil.h is currently
>> the only header common between InstrProfilingFile.c and GCDAProfiling.c.
>> I can move the shim to InstrProfilingPort.h and add an include to
>> GCDAProfiling.c as per your preference David.
>>
>> http://reviews.llvm.org/D17676
>>
>> Files:
>>   lib/profile/InstrProfilingPort.h
>>   lib/profile/InstrProfilingUtil.h
>>
>> Index: lib/profile/InstrProfilingUtil.h
>> ===================================================================
>> --- lib/profile/InstrProfilingUtil.h
>> +++ lib/profile/InstrProfilingUtil.h
>> @@ -13,4 +13,9 @@
>>  /*! \brief Create a directory tree. */
>>  void __llvm_profile_recursive_mkdir(char *Pathname);
>>
>> +/* PS4 doesn't have getenv. Define a shim. */
>> +#if __PS4__
>> +static inline char *getenv(const char *name) { return 0; }
>> +#endif /* #if __PS4__ */
>> +
>>  #endif  /* PROFILE_INSTRPROFILINGUTIL_H */
>> Index: lib/profile/InstrProfilingPort.h
>> ===================================================================
>> --- lib/profile/InstrProfilingPort.h
>> +++ lib/profile/InstrProfilingPort.h
>> @@ -25,6 +25,8 @@
>>  #define COMPILER_RT_MAX_HOSTLEN 128
>>  #ifdef _MSC_VER
>>  #define COMPILER_RT_GETHOSTNAME(Name, Len) gethostname(Name, Len)
>> +#elif defined(__PS4__)
>> +#define COMPILER_RT_GETHOSTNAME(Name, Len) (-1)
>>  #else
>>  #define COMPILER_RT_GETHOSTNAME(Name, Len) GetHostName(Name, Len)
>>  #define COMPILER_RT_HAS_UNAME 1
>>
>>
>> This looks fine.
>
> thanks,
>
> David
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160227/4c8f6248/attachment.html>