[profile] Don't dump counters when forking and don't reset when calling exec** functions

[calixte]

Summary

There is no need to write out gcdas when forking because we can just reset the counters in the parent process.
Let say a counter is N before the fork, then fork and this counter is set to 0 in the child process.
In the parent process, the counter is incremented by P and in the child process it's incremented by C.
When dump is ran at exit, parent process will dump N+P for the given counter and the child process will dump 0+C, so when the gcdas are merged the resulting counter will be N+P+C.
About exec** functions, since the current process is replaced by an another one there is no need to reset the counters but just write out the gcdas since the counters are definitely lost.
To avoid to have lists in a bad state, we just lock them during the fork and the flush (if called explicitely) and lock them when an element is added.

[marco-c]

Also, as we discussed, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93623#c5 regarding exec.

compiler-rt/lib/profile/GCDAProfiling.c

677

Could you expand the comment explaining a situation where this could fail if we didn't lock?

683

Nit: do we need this check or can we just use the earlier one on pid == 0?

687

What if we have a thread in another process making modifications to the list?

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

635

Since we are now mostly doing different things on forks and execs, we could remove this vector and just do the operations directly instead of adding to the vec.

671

Isn't this bug fixed by splitting the block?

[calixte]

If exec** fails we need to reset the counters since they've have dumped just before to avoid to count them twice.

[calixte]

Add more comments to explain why we need to lock around the fork

compiler-rt/lib/profile/GCDAProfiling.c

683

I added this check to be sure we had a true fork in case of someone wrote its own fork function.

687

When forking, only the thread in the parent process is copied so in the child process we've only one thread.
When forking, the parent and the child process share the same memory until some change (Copy-on-Write), so even if in the parent process a list is changed then the memory will be copied before the change and then the child process will have an unchanged list.

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

635

If we make the insertions of new code in the second for loop, we'll invalidate the iterator used in this loop.

671

With the example shew here, the split is in foo but not in bar, blah() should be in another block

[marco-c]

Could you test the last iteration of the patch on Mozilla's CI (with the workaround for the mismatch in LLVM version used by Rust)?

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

635

M->getOrInsertFunction is what would invalidate the iterator?

You could clean things up a bit by having two vectors then, one for forks and one for execs.

686

Replace with // No need to reset the counters since they'll be lost after the exec**

[calixte]

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

635

Builder.Create** will invalidate the iterator

[calixte]

Fix a typo

clang/lib/Driver/ToolChains/Darwin.cpp

void Darwin::addProfileRTLibs(const ArgList &Args,
   bool ForGCOV = needsGCovInstrumentation(Args);
 
   // If we have a symbol export directive and we're linking in the profile
   // runtime, automatically export symbols necessary to implement some of the
   // runtime's functionality.
   if (hasExportSymbolDirective(Args)) {
     if (ForGCOV) {
       addExportedSymbol(CmdArgs, "___gcov_flush");
+      addExportedSymbol(CmdArgs, "___gcov_fork");
       addExportedSymbol(CmdArgs, "_flush_fn_list");
       addExportedSymbol(CmdArgs, "_writeout_fn_list");
+      addExportedSymbol(CmdArgs, "_reset_fn_list");
     } else {
       addExportedSymbol(CmdArgs, "___llvm_profile_filename");
       addExportedSymbol(CmdArgs, "___llvm_profile_raw_version");
     }
     addExportedSymbol(CmdArgs, "_lprofDirMode");
   }
 
   // Align __llvm_prf_{cnts,data} sections to the maximum expected page

compiler-rt/lib/profile/GCDAProfiling.c

 #include <stdlib.h>
 #include <string.h>
 
 #if defined(_WIN32)
 #define WIN32_LEAN_AND_MEAN
 #include <windows.h>
 #include "WindowsMMap.h"
 #else
-#include <sys/mman.h>
 #include <sys/file.h>
+#include <sys/mman.h>
+#include <unistd.h>
 #endif
 
 #if defined(__FreeBSD__) && defined(__i386__)
 #define I386_FREEBSD 1
 #else
 #define I386_FREEBSD 0
 #endif
 

 typedef unsigned char uint8_t;
 typedef unsigned int uint32_t;
 typedef unsigned long long uint64_t;
 #endif
 
 #include "InstrProfiling.h"
 #include "InstrProfilingUtil.h"
 
+/* #define DEBUG_GCDAPROFILING */
+
 #ifndef _WIN32
 #include <pthread.h>
-static pthread_mutex_t gcov_flush_mutex = PTHREAD_MUTEX_INITIALIZER;
-static __inline void gcov_flush_lock() {
-  pthread_mutex_lock(&gcov_flush_mutex);
-}
-static __inline void gcov_flush_unlock() {
-  pthread_mutex_unlock(&gcov_flush_mutex);
-}
+pthread_mutex_t gcov_mutex = PTHREAD_MUTEX_INITIALIZER;
+static __inline void gcov_lock() { pthread_mutex_lock(&gcov_mutex); }
+static __inline void gcov_unlock() { pthread_mutex_unlock(&gcov_mutex); }
 #else
 #include <windows.h>
-static SRWLOCK gcov_flush_mutex = SRWLOCK_INIT;
-static __inline void gcov_flush_lock() {
-  AcquireSRWLockExclusive(&gcov_flush_mutex);
-}
-static __inline void gcov_flush_unlock() {
-  ReleaseSRWLockExclusive(&gcov_flush_mutex);
-}
+SRWLOCK gcov_mutex = SRWLOCK_INIT;
+static __inline void gcov_lock() { AcquireSRWLockExclusive(&gcov_mutex); }
+static __inline void gcov_unlock() { ReleaseSRWLockExclusive(&gcov_mutex); }
 #endif
 
-/* #define DEBUG_GCDAPROFILING */
 /*
  * --- GCOV file format I/O primitives ---
  */
 
 /*
  * The current file name we're outputting. Used primarily for error logging.
  */
 static char *filename = NULL;

  */
 struct fn_list writeout_fn_list;
 
 /*
  *  A list of flush functions that our __gcov_flush() function should call, shared between all dynamic objects.
  */
 struct fn_list flush_fn_list;
 
+/*
+ *  A list of reset functions that our __gcov_reset() function should call,
+ * shared between all dynamic objects.
+ */
+struct fn_list reset_fn_list;
+
 static void fn_list_insert(struct fn_list* list, fn_ptr fn) {
   struct fn_node* new_node = malloc(sizeof(struct fn_node));
   new_node->fn = fn;
   new_node->next = NULL;
   new_node->id = CURRENT_ID;
 
   if (!list->head) {
     list->head = list->tail = new_node;
void llvm_delete_writeout_function_list(void) {
   fn_list_remove(&writeout_fn_list);
 }
 
 COMPILER_RT_VISIBILITY
 void llvm_register_flush_function(fn_ptr fn) {
   fn_list_insert(&flush_fn_list, fn);
 }
 
+COMPILER_RT_VISIBILITY
+void llvm_register_reset_function(fn_ptr fn) {
+  fn_list_insert(&reset_fn_list, fn);
+}
+
+COMPILER_RT_VISIBILITY
+void llvm_reset_counters(void) {
+  struct fn_node *curr = reset_fn_list.head;
+
+  while (curr) {
+    if (curr->id == CURRENT_ID) {
+      curr->fn();
+    }
+    curr = curr->next;
+  }
+}
+
 void __gcov_flush() {
-  gcov_flush_lock();
+  gcov_lock();
 
   struct fn_node* curr = flush_fn_list.head;
 
   while (curr) {
     curr->fn();
     curr = curr->next;
   }
 
-  gcov_flush_unlock();
+  gcov_unlock();
 }
 
+#if !defined(_WIN32)
+pid_t __gcov_fork() {
+  pid_t parent_pid = getpid();
+  pid_t pid;
+
+  gcov_lock();
+  // Avoid a concurrent modification of the lists during the fork
+  pid = fork();
+  gcov_unlock();
+
+  if (pid == 0) {
+    pid_t child_pid = getpid();
+    if (child_pid != parent_pid) {
+      // The pid changed so we've a fork
+      // Just reset the counters for this child process
+      // No need to lock here since we just forked and cannot have any other
+      // threads.
+      llvm_reset_counters();
+    }
+  }
+  return pid;
+}
+#endif
+
 COMPILER_RT_VISIBILITY
 void llvm_delete_flush_function_list(void) {
   fn_list_remove(&flush_fn_list);
 }
 
 COMPILER_RT_VISIBILITY
-void llvm_gcov_init(fn_ptr wfn, fn_ptr ffn) {
+void llvm_delete_reset_function_list(void) { fn_list_remove(&reset_fn_list); }
+
+COMPILER_RT_VISIBILITY
+void llvm_gcov_init(fn_ptr wfn, fn_ptr ffn, fn_ptr rfn) {
   static int atexit_ran = 0;
 
+  gcov_lock();
+
   if (wfn)
     llvm_register_writeout_function(wfn);
 
   if (ffn)
     llvm_register_flush_function(ffn);
 
+  if (rfn)
+    llvm_register_reset_function(rfn);
+
+  gcov_unlock();
+
   if (atexit_ran == 0) {
     atexit_ran = 1;
 
     /* Make sure we write out the data and delete the data structures. */
     atexit(llvm_delete_flush_function_list);
     atexit(llvm_delete_writeout_function_list);
+    atexit(llvm_delete_reset_function_list);
     atexit(llvm_writeout_files);
   }
 }
 
 #endif

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

private:
   FunctionCallee getEmitArcsFunc(const TargetLibraryInfo *TLI);
   FunctionCallee getSummaryInfoFunc();
   FunctionCallee getEndFileFunc();
 
   // Add the function to write out all our counters to the global destructor
   // list.
   Function *
   insertCounterWriteout(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);
-  Function *insertFlush(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);
+  Function *insertReset(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);
+  Function *insertFlush(Function *ResetF);
 
   void AddFlushBeforeForkAndExec();
 
   enum class GCovFileType { GCNO, GCDA };
   std::string mangleName(const DICompileUnit *CU, GCovFileType FileType);
 
   GCOVOptions Options;
 
static bool shouldKeepInEntry(BasicBlock::iterator It) {
   if (auto *II = dyn_cast<IntrinsicInst>(It)) {
     if (II->getIntrinsicID() == llvm::Intrinsic::localescape) return true;
   }
 
   return false;
 }
 
 void GCOVProfiler::AddFlushBeforeForkAndExec() {
-  SmallVector<Instruction *, 2> ForkAndExecs;
+  SmallVector<std::pair<bool, CallInst *>, 2> ForkAndExecs;
   for (auto &F : M->functions()) {
     auto *TLI = &GetTLI(F);
     for (auto &I : instructions(F)) {
       if (CallInst *CI = dyn_cast<CallInst>(&I)) {
         if (Function *Callee = CI->getCalledFunction()) {
           LibFunc LF;
-          if (TLI->getLibFunc(*Callee, LF) &&
-              (LF == LibFunc_fork || LF == LibFunc_execl ||
-               LF == LibFunc_execle || LF == LibFunc_execlp ||
-               LF == LibFunc_execv || LF == LibFunc_execvp ||
-               LF == LibFunc_execve || LF == LibFunc_execvpe ||
-               LF == LibFunc_execvP)) {
-            ForkAndExecs.push_back(&I);
+          if (TLI->getLibFunc(*Callee, LF)) {
+            if (LF == LibFunc_fork) {
+#if !defined(_WIN32)
+              ForkAndExecs.push_back({true, CI});
+#endif
+            } else if (LF == LibFunc_execl || LF == LibFunc_execle ||
+                       LF == LibFunc_execlp || LF == LibFunc_execv ||
+                       LF == LibFunc_execvp || LF == LibFunc_execve ||
+                       LF == LibFunc_execvpe || LF == LibFunc_execvP) {
+              ForkAndExecs.push_back({false, CI});
+            }
           }
         }
       }
     }
   }
 
-  // We need to split the block after the fork/exec call
-  // because else the counters for the lines after will be
-  // the same as before the call.
-  for (auto I : ForkAndExecs) {
-    IRBuilder<> Builder(I);
+  for (auto F : ForkAndExecs) {
+    IRBuilder<> Builder(F.second);
+    BasicBlock *Parent = F.second->getParent();
+    auto NextInst = ++F.second->getIterator();
+
+    if (F.first) {
+      // We've a fork so just reset the counters in the child process
+      FunctionType *FTy = FunctionType::get(Builder.getInt32Ty(), {}, false);
+      FunctionCallee GCOVFork = M->getOrInsertFunction("__gcov_fork", FTy);
+      F.second->setCalledFunction(GCOVFork);
+      if (NextInst != Parent->end()) {
+        // We split just after the fork to have a counter for the lines after
+        // Anyway there's a bug:
+        // void foo() { fork(); }
+        // void bar() { foo(); blah(); }
+        // then "blah();" will be called 2 times but showed as 1
+        // because "blah()" belongs to the same block as "foo();"
+        Parent->splitBasicBlock(NextInst);
+
+        // back() is a br instruction with a debug location
+        // equals to the one from NextAfterFork
+        // So to avoid to have two debug locs on two blocks just change it
+        DebugLoc Loc = F.second->getDebugLoc();
+        Parent->back().setDebugLoc(Loc);
+      }
+    } else {
+      // Since the process is replaced by a new one we need to write out gcdas
+      // No need to reset the counters since there'll be lost after the exec**
       FunctionType *FTy = FunctionType::get(Builder.getVoidTy(), {}, false);
-    FunctionCallee GCOVFlush = M->getOrInsertFunction("__gcov_flush", FTy);
-    Builder.CreateCall(GCOVFlush);
-    I->getParent()->splitBasicBlock(I);
+      FunctionCallee WriteoutF =
+          M->getOrInsertFunction("llvm_writeout_files", FTy);
+      Builder.CreateCall(WriteoutF);
+      if (NextInst != Parent->end()) {
+        DebugLoc Loc = F.second->getDebugLoc();
+        Builder.SetInsertPoint(&*NextInst);
+        // If the exec** fails we must reset the counters since they've been
+        // dumped
+        FunctionCallee ResetF =
+            M->getOrInsertFunction("llvm_reset_counters", FTy);
+        Builder.CreateCall(ResetF)->setDebugLoc(Loc);
+        Parent->splitBasicBlock(NextInst);
+        Parent->back().setDebugLoc(Loc);
+      }
+    }
   }
 }
 
 void GCOVProfiler::emitProfileNotes() {
   NamedMDNode *CU_Nodes = M->getNamedMetadata("llvm.dbg.cu");
   if (!CU_Nodes) return;
 
   for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {
for (auto &F : M->functions()) {
             Count = Builder.CreateAdd(Count, Builder.getInt64(1));
             Builder.CreateStore(Count, Counter);
           }
         }
       }
     }
 
     Function *WriteoutF = insertCounterWriteout(CountersBySP);
-    Function *FlushF = insertFlush(CountersBySP);
+    Function *ResetF = insertReset(CountersBySP);
+    Function *FlushF = insertFlush(ResetF);
 
     // Create a small bit of code that registers the "__llvm_gcov_writeout" to
     // be executed at exit and the "__llvm_gcov_flush" function to be executed
     // when "__gcov_flush" is called.
     FunctionType *FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);
     Function *F = Function::Create(FTy, GlobalValue::InternalLinkage,
                                    "__llvm_gcov_init", M);
     F->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
     F->setLinkage(GlobalValue::InternalLinkage);
     F->addFnAttr(Attribute::NoInline);
     if (Options.NoRedZone)
       F->addFnAttr(Attribute::NoRedZone);
 
     BasicBlock *BB = BasicBlock::Create(*Ctx, "entry", F);
     IRBuilder<> Builder(BB);
 
     FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);
-    Type *Params[] = {
-      PointerType::get(FTy, 0),
-      PointerType::get(FTy, 0)
-    };
+    Type *Params[] = {PointerType::get(FTy, 0), PointerType::get(FTy, 0),
+                      PointerType::get(FTy, 0)};
     FTy = FunctionType::get(Builder.getVoidTy(), Params, false);
 
-    // Initialize the environment and register the local writeout and flush
-    // functions.
+    // Initialize the environment and register the local writeout, flush and
+    // reset functions.
     FunctionCallee GCOVInit = M->getOrInsertFunction("llvm_gcov_init", FTy);
-    Builder.CreateCall(GCOVInit, {WriteoutF, FlushF});
+    Builder.CreateCall(GCOVInit, {WriteoutF, FlushF, ResetF});
     Builder.CreateRetVoid();
 
     appendToGlobalCtors(*M, F, 0);
   }
 
   return Result;
 }
 
Function *GCOVProfiler::insertCounterWriteout(
   IV->addIncoming(NextIV, FileLoopLatch);
 
   Builder.SetInsertPoint(ExitBB);
   Builder.CreateRetVoid();
 
   return WriteoutF;
 }
 
-Function *GCOVProfiler::
-insertFlush(ArrayRef<std::pair<GlobalVariable*, MDNode*> > CountersBySP) {
+Function *GCOVProfiler::insertReset(
+    ArrayRef<std::pair<GlobalVariable *, MDNode *>> CountersBySP) {
+  FunctionType *FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);
+  Function *ResetF = M->getFunction("__llvm_gcov_reset");
+  if (!ResetF)
+    ResetF = Function::Create(FTy, GlobalValue::InternalLinkage,
+                              "__llvm_gcov_reset", M);
+  else
+    ResetF->setLinkage(GlobalValue::InternalLinkage);
+  ResetF->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  ResetF->addFnAttr(Attribute::NoInline);
+  if (Options.NoRedZone)
+    ResetF->addFnAttr(Attribute::NoRedZone);
+
+  BasicBlock *Entry = BasicBlock::Create(*Ctx, "entry", ResetF);
+  IRBuilder<> Builder(Entry);
+
+  // Zero out the counters.
+  for (const auto &I : CountersBySP) {
+    GlobalVariable *GV = I.first;
+    Constant *Null = Constant::getNullValue(GV->getValueType());
+    Builder.CreateStore(Null, GV);
+  }
+
+  Type *RetTy = ResetF->getReturnType();
+  if (RetTy == Type::getVoidTy(*Ctx))
+    Builder.CreateRetVoid();
+  else if (RetTy->isIntegerTy())
+    // Used if __llvm_gcov_reset was implicitly declared.
+    Builder.CreateRet(ConstantInt::get(RetTy, 0));
+  else
+    report_fatal_error("invalid return type for __llvm_gcov_reset");
+
+  return ResetF;
+}
+
+Function *GCOVProfiler::insertFlush(Function *ResetF) {
   FunctionType *FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);
   Function *FlushF = M->getFunction("__llvm_gcov_flush");
   if (!FlushF)
     FlushF = Function::Create(FTy, GlobalValue::InternalLinkage,
                               "__llvm_gcov_flush", M);
   else
     FlushF->setLinkage(GlobalValue::InternalLinkage);
   FlushF->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
   FlushF->addFnAttr(Attribute::NoInline);
   if (Options.NoRedZone)
     FlushF->addFnAttr(Attribute::NoRedZone);
 
   BasicBlock *Entry = BasicBlock::Create(*Ctx, "entry", FlushF);
+  IRBuilder<> Builder(Entry);
 
-  // Write out the current counters.
   Function *WriteoutF = M->getFunction("__llvm_gcov_writeout");
   assert(WriteoutF && "Need to create the writeout function first!");
 
-  IRBuilder<> Builder(Entry);
-  Builder.CreateCall(WriteoutF, {});
-
-  // Zero out the counters.
-  for (const auto &I : CountersBySP) {
-    GlobalVariable *GV = I.first;
-    Constant *Null = Constant::getNullValue(GV->getValueType());
-    Builder.CreateStore(Null, GV);
-  }
+  Builder.CreateCall(WriteoutF);
+  Builder.CreateCall(ResetF);
 
   Type *RetTy = FlushF->getReturnType();
   if (RetTy == Type::getVoidTy(*Ctx))
     Builder.CreateRetVoid();
   else if (RetTy->isIntegerTy())
     // Used if __llvm_gcov_flush was implicitly declared.
     Builder.CreateRet(ConstantInt::get(RetTy, 0));
   else
     report_fatal_error("invalid return type for __llvm_gcov_flush");
 
   return FlushF;
 }