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

[qiongsiwu1]

Summary

Currently, the PGO runtime only creates unique per process profile files when both of the following conditions are true

1. __llvm_profile_initialize is triggered through global variable initialization (e.g. when exec starts an instrumented binary).
2. When there are no existing file patterns set. See https://github.com/llvm/llvm-project/blob/bb6d60bf9dfea82814d9e50c5bb457c47d010611/compiler-rt/lib/profile/InstrProfilingFile.c#L795

This patch changes the behaviour so that a new profile file name is generated as soon as a fork system call creates a new process from an instrumented binary. The patch uses the pthread_atfork hook to register the initialization code, and modifies the file name pattern parsing logic so that when %p is present in the pattern, a unique file name can be generated even though the pattern already exists.

Additionally, the unique profile files are created before the profiling starts. If a process is not terminated gracefully through exit, an empty profile file for that process will exist, indicating that the profiling may be incomplete due to the process's abnormal termination.

Note that the unique profile per process is only generated when %p is present in the profile file name pattern. Additionally, this patch does not support Windows because the implementation relies on pthread.

[davidxl]

compiler-rt/lib/profile/InstrProfilingFile.c

795

what is this change for?

compiler-rt/test/profile/Posix/instrprof-fork.c

11

add some dump and check to make sure profiles are not truncated?

[qiongsiwu1]

Revise the tests to use llvm tools to dump profile data to check for profile validity.

[qiongsiwu1]

compiler-rt/lib/profile/InstrProfilingFile.c

795

This change is not relevant to the feature, rather a small simplification. If I read/tested the code correctly, we already set FilenamePat on line 787 if it is null, so here we no longer need to worry about that case.

compiler-rt/test/profile/Posix/instrprof-fork.c

11

Thanks for the comment! The test now uses llvm-profdata to show that the profile files are valid.

[qiongsiwu1]

Minor comment spelling fix.

[qiongsiwu1]

Ping for review. Thanks!!

[qiongsiwu1]

Updating a test to see if it passes pre-commit CI.

[qiongsiwu1]

Ping for review. Thanks so much!

[qiongsiwu1]

Ping for review. We'd like to get this in to LLVM 17 if possible. Please let me know if there are comments/suggestions. Thanks so much!

[w2yehia]

clang/lib/Driver/ToolChains/AIX.cpp

440

The change in compiler-rt/lib/profile/InstrProfilingFile.c affects non-AIX platforms too, so won't they need -lpthreads too?

440

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.

[qiongsiwu1]

clang/lib/Driver/ToolChains/AIX.cpp

440

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?

440

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.

[qiongsiwu1]

Ping for review. Thanks so much!

[qiongsiwu1]

Ping for review. Thanks so much!

[qiongsiwu1]

Ping for review. Thanks!

Specifically, note that we need to add -lpthread to the tool chain on AIX to make sure the program can link properly. On Linux we do not need this extra option. What about other platforms, such as macOS?

[qiongsiwu1]

Ping for review.

If there are no comments on whether we should add -lpthread for platforms other than AIX, I will land this patch soon, and let the official buildbots check for problems. If there are problems, I will let the bots run a bit (probably a day or so) before reverting to catch as many platforms I need to fix as possible, revert, and fix the patch.

Thanks!

[MaskRay]

In D155290#4572965, @qiongsiwu1 wrote:

Ping for review.

If there are no comments on whether we should add -lpthread for platforms other than AIX, I will land this patch soon, and let the official buildbots check for problems. If there are problems, I will let the bots run a bit (probably a day or so) before reverting to catch as many platforms I need to fix as possible, revert, and fix the patch.

Thanks!

I will take a look as well but please don't land this -lpthread change for non-AIX platforms. I will think about the implication.

[qiongsiwu1]

In D155290#4574797, @MaskRay wrote:

In D155290#4572965, @qiongsiwu1 wrote:

Ping for review.

If there are no comments on whether we should add -lpthread for platforms other than AIX, I will land this patch soon, and let the official buildbots check for problems. If there are problems, I will let the bots run a bit (probably a day or so) before reverting to catch as many platforms I need to fix as possible, revert, and fix the patch.

Thanks!

I will take a look as well but please don't land this -lpthread change for non-AIX platforms. I will think about the implication.

Thanks @MaskRay ! I am putting this patch on hold till I hear back. The patch does not alter non-AIX platforms at the moment.

[MaskRay]

compiler-rt/lib/profile/InstrProfilingFile.c

94

Revert unneeded change.

253

include stdbool.h and use bool

1027

unneeded change

compiler-rt/test/profile/Posix/instrprof-fork.c

4

I usually use RUN: rm -rf %t && mkdir %t && cd %t and then use something like -o t to place the executable under %t as well.

22

parent and child have the same logic. Use:

if (pid == -1)
  return 1;
printf("%ld.profraw\n", (long)getpid());

[MaskRay]

In D155290#4576643, @qiongsiwu1 wrote:

In D155290#4574797, @MaskRay wrote:

In D155290#4572965, @qiongsiwu1 wrote:

Ping for review.

If there are no comments on whether we should add -lpthread for platforms other than AIX, I will land this patch soon, and let the official buildbots check for problems. If there are problems, I will let the bots run a bit (probably a day or so) before reverting to catch as many platforms I need to fix as possible, revert, and fix the patch.

Thanks!

I will take a look as well but please don't land this -lpthread change for non-AIX platforms. I will think about the implication.

Thanks @MaskRay ! I am putting this patch on hold till I hear back. The patch does not alter non-AIX platforms at the moment.

The pthread_atfork undefined reference will cause a problem using glibc<2.34 where pthread_atfork lives in libpthread instead of libc.
I think non-AIX-non-Windows systems can live with an extra -lpthread. For ELF systems (not Apple's among non-AIX-non-Windows systems), -lpthread must be after libclang_rt.profile.a.

compiler-rt/test/profile/Posix/instrprof-fork.c

22

Please ignore my previous comment. I think we want to have a deterministic parent / child output order. Consider adding a wait for the parent process so that we always see the child printf("%ld.profraw\n", (long)getpid()); output before the parent one.

We should inspect more information of the two raw profile files. Create a function only called by the parent and another function only called by the child. Check their counters.

We also need a test if the child process spawns another process.

[MaskRay]

Actually, I am not sure changing %p to create multiple raw profile files should be the default.
Currently, child counters are attributed to the raw profile file derived from the parent process id. Thanks to online profile merging we get just one raw profile file (though counters before fork may be doubly incremented).

Using %p should not magically change the behavior and I am unsure the result is better for PGO.
Can you elaborate separate profile files are going to be useful?
It we decide to provide such a mode, it needs to be opt-in by adding a new format specifier or API.

[qiongsiwu1]

In D155290#4582825, @MaskRay wrote:

Using %p should not magically change the behavior and I am unsure the result is better for PGO.
If we decide to provide such a mode, it needs to be opt-in by adding a new format specifier or API.
Can you elaborate separate profile files are going to be useful?

Thanks again for taking a look at this patch!

Sure! We are generating separate profiles through %p for two reasons. First, we think that %p implies a profile per process, even if exec is not called after fork. It is unexpected that with %p, a program that forks child processes only create one profile file from the parent. This is why we did not create a new API or create a new pattern. Second, we are trying to catch non-gracefully terminated processes during profile generate. We observed a scenario where a hot function having all 0 counters. The reasons was the child process was created by a fork (but no exec followed), and then was terminated probably by a signal, instead of going through exit(). Such a process did not have a chance to write back the profile for merging. The parent process took a different call path and never called the hot function. We would like to detect such a situation for a particular process. Hence this patch creates an empty profile file if a process is not terminated gracefully. Using %p is probably the most natural choice when debugging such issues.

One can argue that we may look into the continuous mode to manage abnormal terminations during profile generate. I have not looked too closely at that, but we think it is still useful for a user to tell that some processes did not write any profile back, for debugging, or for diagnostics.

[phosek]

Can we avoid the use of pthread_atfork and the dependency on pthreads? Using pthreads inside the profile runtime implementation is problematic in the case where you want to instrument the libc itself which is used on some platforms and it's one of the reasons why we would like to eliminate the dependency on libc altogether and use sanitizer_common instead, see https://discourse.llvm.org/t/using-c-and-sanitizer-common-in-profile-runtime/58629.

In D155290#4585607, @qiongsiwu1 wrote:

In D155290#4582825, @MaskRay wrote:

Using %p should not magically change the behavior and I am unsure the result is better for PGO.
If we decide to provide such a mode, it needs to be opt-in by adding a new format specifier or API.
Can you elaborate separate profile files are going to be useful?

Thanks again for taking a look at this patch!

Sure! We are generating separate profiles through %p for two reasons. First, we think that %p implies a profile per process, even if exec is not called after fork. It is unexpected that with %p, a program that forks child processes only create one profile file from the parent. This is why we did not create a new API or create a new pattern. Second, we are trying to catch non-gracefully terminated processes during profile generate. We observed a scenario where a hot function having all 0 counters. The reasons was the child process was created by a fork (but no exec followed), and then was terminated probably by a signal, instead of going through exit(). Such a process did not have a chance to write back the profile for merging. The parent process took a different call path and never called the hot function. We would like to detect such a situation for a particular process. Hence this patch creates an empty profile file if a process is not terminated gracefully. Using %p is probably the most natural choice when debugging such issues.

One can argue that we may look into the continuous mode to manage abnormal terminations during profile generate. I have not looked too closely at that, but we think it is still useful for a user to tell that some processes did not write any profile back, for debugging, or for diagnostics.

Handling cases like these is exactly why the continous mode was created. Your solution only covers a single case, but there's many scenarios where process can terminate abnormally and fail to invoke atexit hooks which would results in profile not being written out.

[qiongsiwu1]

Thanks for chiming in @phosek !

In D155290#4587438, @phosek wrote:

Can we avoid the use of pthread_atfork and the dependency on pthreads? Using pthreads inside the profile runtime implementation is problematic in the case where you want to instrument the libc itself which is used on some platforms and it's one of the reasons why we would like to eliminate the dependency on libc altogether and use sanitizer_common instead, see https://discourse.llvm.org/t/using-c-and-sanitizer-common-in-profile-runtime/58629.

Yes that is something I will try. The case illustrated in https://discourse.llvm.org/t/using-c-and-sanitizer-common-in-profile-runtime/58629 makes sense. Is there something similar to pthread_atfork in sanitizer_common you would recommend? This functionality needs a hook at fork, and it does not really rely on pthreads.

In D155290#4585607, @qiongsiwu1 wrote:

In D155290#4582825, @MaskRay wrote:

Using %p should not magically change the behavior and I am unsure the result is better for PGO.
If we decide to provide such a mode, it needs to be opt-in by adding a new format specifier or API.
Can you elaborate separate profile files are going to be useful?

Thanks again for taking a look at this patch!

Sure! We are generating separate profiles through %p for two reasons. First, we think that %p implies a profile per process, even if exec is not called after fork. It is unexpected that with %p, a program that forks child processes only create one profile file from the parent. This is why we did not create a new API or create a new pattern. Second, we are trying to catch non-gracefully terminated processes during profile generate. We observed a scenario where a hot function having all 0 counters. The reasons was the child process was created by a fork (but no exec followed), and then was terminated probably by a signal, instead of going through exit(). Such a process did not have a chance to write back the profile for merging. The parent process took a different call path and never called the hot function. We would like to detect such a situation for a particular process. Hence this patch creates an empty profile file if a process is not terminated gracefully. Using %p is probably the most natural choice when debugging such issues.

One can argue that we may look into the continuous mode to manage abnormal terminations during profile generate. I have not looked too closely at that, but we think it is still useful for a user to tell that some processes did not write any profile back, for debugging, or for diagnostics.

Handling cases like these is exactly why the continous mode was created. Your solution only covers a single case, but there's many scenarios where process can terminate abnormally and fail to invoke atexit hooks which would results in profile not being written out.

Yes I agree there is overlap with continuous mode. That said, there is still the first reason that we need to implement this feature, i.e. no new profile file is created at fork when %p is present in the file name pattern. My understanding is that %p implies a profile file per process created, see the new test case compiler-rt/test/profile/Posix/instrprof-fork.c. My expectation when running the test case is that we should have two different profile files, while currently we only have one profile file created. As @MaskRay pointed out in his comment for this test case, the profile files generated for the two processes can be very different. In the context of fork without exec, the current implementation will only generate one profile file. Effectively the result is identical with the profile generated without %p, if I understand the current implementation correctly. Additionally, continuous mode is not available on all platforms (for example AIX which is our use case). We are working on getting the continuous mode online, but %p as a debugging feature in the context of fork without exec seems to be useful.

With this said, if it is indeed our intention that new profile files should not be created upon fork when %p is present in the file pattern, I am happy to choose alternative routes. I would love to know why we have such an intention before moving on. I understand that this is an existing feature so we may be hesitant to change its behavior, and we would rather not have dependency on pthread, which is an implementation issue. Are there additional reasons that we prefer the current behavior?

Thanks again for the input!

clang/lib/Driver/ToolChains/AIX.cpp

if (Args.hasFlag(options::OPT_fxl_pragma_pack,
                    options::OPT_fno_xl_pragma_pack, true))
     CC1Args.push_back("-fxl-pragma-pack");
 }
 
 void AIX::addProfileRTLibs(const llvm::opt::ArgList &Args,
                            llvm::opt::ArgStringList &CmdArgs) const {
   // Add linker option -u__llvm_profile_runtime to cause runtime
   // initialization to occur.
-  if (needsProfileRT(Args))
+  bool NeedsProfileRT = needsProfileRT(Args);
+  if (NeedsProfileRT)
     CmdArgs.push_back(Args.MakeArgString(
         Twine("-u", llvm::getInstrProfRuntimeHookVarName())));
+
+  if (NeedsProfileRT || needsGCovInstrumentation(Args))
+    CmdArgs.push_back("-lpthreads");
+
   ToolChain::addProfileRTLibs(Args, CmdArgs);
 }
 
 ToolChain::CXXStdlibType AIX::GetDefaultCXXStdlibType() const {
   return ToolChain::CST_Libcxx;
 }
 
 ToolChain::RuntimeLibType AIX::GetDefaultRuntimeLibType() const {
   return ToolChain::RLT_CompilerRT;
 }
 
 auto AIX::buildAssembler() const -> Tool * { return new aix::Assembler(*this); }
 
 auto AIX::buildLinker() const -> Tool * { return new aix::Linker(*this); }

compiler-rt/lib/profile/InstrProfilingFile.c

 #include <process.h>
 #else
 #include <sys/file.h>
 #include <sys/mman.h>
 #include <unistd.h>
 #if defined(__linux__)
 #include <sys/types.h>
 #endif
+/* For pthread_atfork(). Not supported on Windows. */
+#include <pthread.h>
 #endif
 
 #include "InstrProfiling.h"
 #include "InstrProfilingInternal.h"
 #include "InstrProfilingPort.h"
 #include "InstrProfilingUtil.h"
 
 /* From where is profile name specified.
typedef struct lprofFilename {
    * by the %[0-9]m specifier. For instance %2m enables merging using
    * 2 profile data files. %1m is equivalent to %m. Also %m specifier
    * can only appear once at the end of the name pattern. */
   unsigned MergePoolSize;
   ProfileNameSpecifier PNS;
 } lprofFilename;
 
 static lprofFilename lprofCurFilename = {0,   0, 0, {0}, NULL,
                                          {0}, 0, 0, 0,   PNS_unknown};
-
 static int ProfileMergeRequested = 0;
 static int getProfileFileSizeForMerging(FILE *ProfileFile,
                                         uint64_t *ProfileFileSize);
 
 #if defined(__APPLE__)
 static const int ContinuousModeSupported = 1;
 static const int UseBiasVar = 0;
 static const char *FileOpenMode = "a+b";

 static void setProfileFile(FILE *File) { ProfileFile = File; }
 
 static int getCurFilenameLength(void);
 static const char *getCurFilename(char *FilenameBuf, int ForceUseBuf);
 static unsigned doMerging(void) {
   return lprofCurFilename.MergePoolSize || isProfileMergeRequested();
 }
 
+// Flag to indicate if a new profile name should be used when a new
+// process is forked. The flag cannot be true on Windows,
+// because the implementation relies on pthread.
+static int ResetNameAtFork = 0;
+
 /* Return 1 if there is an error, otherwise return  0.  */
 static uint32_t fileWriter(ProfDataWriter *This, ProfDataIOVec *IOVecs,
                            uint32_t NumIOVecs) {
   uint32_t I;
   FILE *File = (FILE *)This->WriterCtx;
   char Zeroes[sizeof(uint64_t)] = {0};
   for (I = 0; I < NumIOVecs; I++) {
     if (IOVecs[I].Data) {
static void truncateCurrentFile(void) {
   int Length;
 
   Length = getCurFilenameLength();
   FilenameBuf = (char *)COMPILER_RT_ALLOCA(Length + 1);
   Filename = getCurFilename(FilenameBuf, 0);
   if (!Filename)
     return;
 
-  /* Only create the profile directory and truncate an existing profile once.
-   * In continuous mode, this is necessary, as the profile is written-to by the
-   * runtime initializer. */
+  /* Only create the profile directory once. In continuous mode, this is
+   * necessary, as the profile is written-to by the runtime initializer. On the
+   * other hand, truncate an existing profile once if the runtime does not
+   * reset the profile file name at fork, because the runtime creates empty
+   * profile files for each processes forked to indicate profiling has started.
+   */
   int initialized = getenv(LPROF_INIT_ONCE_ENV) != NULL;
-  if (initialized)
+  if (initialized && !ResetNameAtFork)
     return;
+
+  if (!initialized) {
 #if defined(_WIN32)
     _putenv(LPROF_INIT_ONCE_ENV "=" LPROF_INIT_ONCE_ENV);
 #else
     setenv(LPROF_INIT_ONCE_ENV, LPROF_INIT_ONCE_ENV, 1);
 #endif
 
     /* Create the profile dir (even if online merging is enabled), so that
      * the profile file can be set up if continuous mode is enabled. */
     createProfileDir(Filename);
+  }
 
   /* By pass file truncation to allow online raw profile merging. */
   if (lprofCurFilename.MergePoolSize)
     return;
 
   /* Truncate the file.  Later we'll reopen and append. */
   File = fopen(Filename, "w");
   if (!File)
static void parseAndSetFilename(const char *FilenamePat,
 
   /* The old profile name specifier takes precedence over the old one. */
   if (PNS < OldPNS)
     return;
 
   if (!FilenamePat)
     FilenamePat = DefaultProfileName;
 
-  if (OldFilenamePat && !strcmp(OldFilenamePat, FilenamePat)) {
+#if !defined(_WIN32)
+  ResetNameAtFork = strstr(FilenamePat, "%p") != NULL;
+#endif
+
+  if (OldFilenamePat && !strcmp(OldFilenamePat, FilenamePat) &&
+      !ResetNameAtFork) {
     lprofCurFilename.PNS = PNS;
     return;
   }
 
-  /* When PNS >= OldPNS, the last one wins. */
-  if (!FilenamePat || parseFilenamePattern(FilenamePat, CopyFilenamePat))
+  if (parseFilenamePattern(FilenamePat, CopyFilenamePat))
     resetFilenameToDefault();
+
+  /* When PNS >= OldPNS, the last one wins. */
   lprofCurFilename.PNS = PNS;
 
   if (!OldFilenamePat) {
     if (getenv("LLVM_PROFILE_VERBOSE"))
       PROF_NOTE("Set profile file path to \"%s\" via %s.\n",
                 lprofCurFilename.FilenamePat, getPNSStr(PNS));
   } else {
     if (getenv("LLVM_PROFILE_VERBOSE"))

 }
 
 /* This method is invoked by the runtime initialization hook
  * InstrProfilingRuntime.o if it is linked in.
  */
 COMPILER_RT_VISIBILITY
 void __llvm_profile_initialize(void) {
   __llvm_profile_initialize_file();
+
+#if !defined(_WIN32)
+  if (ResetNameAtFork)
+    pthread_atfork(NULL, NULL, __llvm_profile_initialize_file);
+#endif
+
   if (!__llvm_profile_is_continuous_mode_enabled())
     __llvm_profile_register_write_file_atexit();
 }
 
 /* This API is directly called by the user application code. It has the
  * highest precedence compared with LLVM_PROFILE_FILE environment variable
  * and command line option -fprofile-instr-generate=<profile_name>.
  */
 COMPILER_RT_VISIBILITY
 void __llvm_profile_set_filename(const char *FilenamePat) {
   if (__llvm_profile_is_continuous_mode_enabled())
     return;
+
   parseAndSetFilename(FilenamePat, PNS_runtime_api, 1);
 }
 
 /* The public API for writing profile data into the file with name
  * set by previous calls to __llvm_profile_set_filename or
  * __llvm_profile_override_default_filename or
  * __llvm_profile_initialize_file. */
 COMPILER_RT_VISIBILITY

compiler-rt/test/profile/Posix/instrprof-fork-int.c

+/// Testing fork without exec. The child is interrupted.
+/// Make sure we have an empty profile file.
+// UNSUPPORTED: target={{.*windows.*}}
+// RUN: rm -rf %t.d
+// RUN: mkdir -p %t.d && cd %t.d
+// RUN: %clang_pgogen %s -o %t.exe
+// RUN: LLVM_PROFILE_FILE="%%p.profraw" %t.exe > %t.out
+// RUN: not llvm-profdata show `cat %t.out` 2>&1 | FileCheck %s
+
+// CHECK: error: {{[0-9]+}}.profraw: empty raw profile file
+
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+int main(int argc, char **argv) {
+  pid_t pid;
+  pid = fork();
+  if (!pid) {
+    printf("%ld.profraw\n", (long)getpid());
+    fflush(stdout);
+    raise(SIGSEGV);
+  }
+  return 0;
+}

compiler-rt/test/profile/Posix/instrprof-fork.c

+/// Testing fork without exec. The runtime should generate two profile files.
+// UNSUPPORTED: target={{.*windows.*}}
+// RUN: rm -rf %t.d
+// RUN: mkdir -p %t.d && cd %t.d
+// RUN: %clang_pgogen %s -o %t.exe
+// RUN: LLVM_PROFILE_FILE="%%p.profraw" %t.exe > %t.out
+// RUN: PROFILE1=`head -n 1 %t.out`
+// RUN: llvm-profdata show $PROFILE1 | FileCheck %s
+// RUN: PROFILE2=`tail -n 1 %t.out`
+// RUN: llvm-profdata show $PROFILE2 | FileCheck %s
+
+// CHECK: Instrumentation level: IR  entry_first = 0
+// CHECK-NEXT: Total functions: 1
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+int main(int argc, char **argv) {
+  pid_t pid;
+  pid = fork();
+  if (!pid) {
+    printf("%ld.profraw\n", (long)getpid());
+  } else {
+    printf("%ld.profraw\n", (long)getpid());
+  }
+  return 0;
+}