[profile] Remove fork management from code coverage

[calixte]

Summary

In order to avoid crash when using fork in a multhreaded environment, just remove that stuff in waiting for a better proposal.

[jfb]

Doing fork will still share an output file, right? Doe we currently end up interleaving content from both forks, or does one of them overwrite the other entirely?
I understand that crashing is undesirable, but wrong data isn't really any better.

compiler-rt/test/profile/Posix/instrprof-gcov-execlp.test

1

Here and for the other test, can you link to the relevant bug explaining why it's unsupported?

[calixte]

In D75436#1903566, @jfb wrote:

Doing fork will still share an output file, right? Doe we currently end up interleaving content from both forks, or does one of them overwrite the other entirely?
I understand that crashing is undesirable, but wrong data isn't really any better.

There is a crash when there are two forks in two threads because of the global variables used to dump in compiler_rt/lib/profile/GCDAProfiling.cpp.
We could probably just remove all the global variables used to dump and creating them in __llvm_gcov_writeout and pass them as arguments to the different functions used to dump.
But we still have the global lists (writeout_fn_list or flush_fn_list) containing the functions to call when writing out or flushing.
These lists can be updated concurently: 2 threads are loading 2 CUs and then the global ctors for each of them are executed and so they're updating concurrently theses list. Is it correct ?
When forking in a thread when another is loading a CU may result in a wrong list too in the child process.
So it's why I think we need to lock when the list are updated and when there is a fork.

Anyway, currently we can crash because of fork (reproductible in clang 9) and we discovered it when we tryed to switch from gcc to clang on our linux toolchains.
I think we must avoid it: I really prefer having inaccurate counters instead of a crash.
And let me work on this again to fix these issues and then have something good: no crash and accurate counters.
FYI, we actively tested my previous patches in our CI on our test-suites with ccov enabled and everything was fine.

compiler-rt/test/profile/Posix/instrprof-gcov-execlp.test

+UNSUPPORTED: true
+
 RUN: mkdir -p %t.d
 RUN: cd %t.d
 
 RUN: %clang --coverage -o %t %S/../Inputs/instrprof-gcov-execlp.c
 RUN: test -f instrprof-gcov-execlp.gcno
 
 RUN: rm -f instrprof-gcov-execlp.gcda
 RUN: %run %t
 RUN: llvm-cov gcov -b -c instrprof-gcov-execlp.gcda
 RUN: FileCheck --match-full-lines --strict-whitespace --input-file instrprof-gcov-execlp.c.gcov %S/../Inputs/instrprof-gcov-execlp.c.gcov

compiler-rt/test/profile/Posix/instrprof-gcov-execvp.test

+UNSUPPORTED: true
+
 RUN: mkdir -p %t.d
 RUN: cd %t.d
 
 RUN: %clang --coverage -o %t %S/../Inputs/instrprof-gcov-execvp.c
 RUN: test -f instrprof-gcov-execvp.gcno
 
 RUN: rm -f instrprof-gcov-execvp.gcda
 RUN: %run %t
 RUN: llvm-cov gcov -b -c instrprof-gcov-execvp.gcda
 RUN: FileCheck --match-full-lines --strict-whitespace --input-file instrprof-gcov-execvp.c.gcov %S/../Inputs/instrprof-gcov-execvp.c.gcov

llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp

private:
   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 *>>);
 
-  void AddFlushBeforeForkAndExec();
-
   enum class GCovFileType { GCNO, GCDA };
   std::string mangleName(const DICompileUnit *CU, GCovFileType FileType);
 
   GCOVOptions Options;
 
   // Reversed, NUL-terminated copy of Options.Version.
   char ReversedVersion[5];
   // Checksum, produced by hash of EdgeDestinations

 }
 
 bool GCOVProfiler::runOnModule(
     Module &M, std::function<const TargetLibraryInfo &(Function &F)> GetTLI) {
   this->M = &M;
   this->GetTLI = std::move(GetTLI);
   Ctx = &M.getContext();
 
-  AddFlushBeforeForkAndExec();
-
   FilterRe = createRegexesFromString(Options.Filter);
   ExcludeRe = createRegexesFromString(Options.Exclude);
 
   if (Options.EmitNotes) emitProfileNotes();
   if (Options.EmitData) return emitProfileArcs();
   return false;
 }
 
static bool shouldKeepInEntry(BasicBlock::iterator It) {
   if (isa<DbgInfoIntrinsic>(*It)) return true;
   if (auto *II = dyn_cast<IntrinsicInst>(It)) {
     if (II->getIntrinsicID() == llvm::Intrinsic::localescape) return true;
   }
 
   return false;
 }
 
-void GCOVProfiler::AddFlushBeforeForkAndExec() {
-  SmallVector<Instruction *, 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);
-          }
-        }
-      }
-    }
-  }
-
-  // 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);
-    FunctionType *FTy = FunctionType::get(Builder.getVoidTy(), {}, false);
-    FunctionCallee GCOVFlush = M->getOrInsertFunction("__gcov_flush", FTy);
-    Builder.CreateCall(GCOVFlush);
-    I->getParent()->splitBasicBlock(I);
-  }
-}
-
 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) {
     // Each compile unit gets its own .gcno file. This means that whether we run
     // this pass over the original .o's as they're produced, or run it after
     // LTO, we'll generate the same .gcno files.