[PATCH] D155290: [PGO] Use Unique Profile Files when New Processes are Forked

[Qiongsi Wu via Phabricator via cfe-commits]

qiongsiwu1 marked 2 inline comments as done.
qiongsiwu1 added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:440
+  if (NeedsProfileRT || needsGCovInstrumentation(Args))
+    CmdArgs.push_back("-lpthreads");
+
----------------
w2yehia wrote:
> w2yehia wrote:
> > The change in `compiler-rt/lib/profile/InstrProfilingFile.c` affects non-AIX platforms too, so won't they need `-lpthreads` too?
> > 
> In order to avoid adding -lpthreads, we can leave it to the user to decide whether to add or not (i.e. they would add -lpthreads to their application's link command if they want to).
> And so to avoid a link error due to undefined symbol, we can define `pthread_atfork` as a weak empty stub in the compiler-rt.
> The change in `compiler-rt/lib/profile/InstrProfilingFile.c` affects non-AIX platforms too, so won't they need `-lpthreads` too?
> 

This is a good question. On Linux we are ok because `pthread_atfork` is actually implemented by `__register_atfork`, which is available in `libc.so`, so we do not need `-lpthreads`. I am not sure about other platforms. @davidxl @vsk @ellis may I get your comments? 


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:440
+  if (NeedsProfileRT || needsGCovInstrumentation(Args))
+    CmdArgs.push_back("-lpthreads");
+
----------------
qiongsiwu1 wrote:
> w2yehia wrote:
> > w2yehia wrote:
> > > The change in `compiler-rt/lib/profile/InstrProfilingFile.c` affects non-AIX platforms too, so won't they need `-lpthreads` too?
> > > 
> > In order to avoid adding -lpthreads, we can leave it to the user to decide whether to add or not (i.e. they would add -lpthreads to their application's link command if they want to).
> > And so to avoid a link error due to undefined symbol, we can define `pthread_atfork` as a weak empty stub in the compiler-rt.
> > The change in `compiler-rt/lib/profile/InstrProfilingFile.c` affects non-AIX platforms too, so won't they need `-lpthreads` too?
> > 
> 
> This is a good question. On Linux we are ok because `pthread_atfork` is actually implemented by `__register_atfork`, which is available in `libc.so`, so we do not need `-lpthreads`. I am not sure about other platforms. @davidxl @vsk @ellis may I get your comments? 
> In order to avoid adding -lpthreads, we can leave it to the user to decide whether to add or not (i.e. they would add -lpthreads to their application's link command if they want to).
> And so to avoid a link error due to undefined symbol, we can define `pthread_atfork` as a weak empty stub in the compiler-rt.

@w2yehia and I discussed offline, and we think it is ok to pass in `-lpthread` on AIX. There should not be any functional issues. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155290/new/

https://reviews.llvm.org/D155290