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

[Xinliang David Li via llvm-commits]

On Sat, Feb 27, 2016 at 1:18 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> 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?
>

That should not be a problem.

David


>
> -- 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/89d4af51/attachment.html>