Author: Hans Wennborg Date: 2020-02-26T13:27:44+01:00 New Revision: 546918cbb4b2231c65cf91fceb738a59d03af7bf
URL: https://github.com/llvm/llvm-project/commit/546918cbb4b2231c65cf91fceb738a59d03af7bf DIFF: https://github.com/llvm/llvm-project/commit/546918cbb4b2231c65cf91fceb738a59d03af7bf.diff LOG: Revert "[compiler-rt] Add a critical section when flushing gcov counters" See discussion on PR44792. This reverts commit 02ce9d8ef5a84bc884de4105eae5f8736ef67634. It also reverts the follow-up commits 8f46269f0 "[profile] Don't dump counters when forking and don't reset when calling exec** functions" 62c7d8402 "[profile] gcov_mutex must be static" Added: Modified: clang/lib/Driver/ToolChains/Darwin.cpp compiler-rt/lib/profile/GCDAProfiling.c llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp Removed: ################################################################################ diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp index c9f87e0e4355..b32fad791bd3 100644 --- a/clang/lib/Driver/ToolChains/Darwin.cpp +++ b/clang/lib/Driver/ToolChains/Darwin.cpp @@ -1144,10 +1144,8 @@ void Darwin::addProfileRTLibs(const ArgList &Args, 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"); diff --git a/compiler-rt/lib/profile/GCDAProfiling.c b/compiler-rt/lib/profile/GCDAProfiling.c index 98815dab541b..498c05900bf2 100644 --- a/compiler-rt/lib/profile/GCDAProfiling.c +++ b/compiler-rt/lib/profile/GCDAProfiling.c @@ -32,9 +32,8 @@ #include <windows.h> #include "WindowsMMap.h" #else -#include <sys/file.h> #include <sys/mman.h> -#include <unistd.h> +#include <sys/file.h> #endif #if defined(__FreeBSD__) && defined(__i386__) @@ -65,18 +64,6 @@ typedef unsigned long long uint64_t; /* #define DEBUG_GCDAPROFILING */ -#ifndef _WIN32 -#include <pthread.h> -static 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_mutex = SRWLOCK_INIT; -static __inline void gcov_lock() { AcquireSRWLockExclusive(&gcov_mutex); } -static __inline void gcov_unlock() { ReleaseSRWLockExclusive(&gcov_mutex); } -#endif - /* * --- GCOV file format I/O primitives --- */ @@ -132,12 +119,6 @@ struct fn_list writeout_fn_list; */ 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; @@ -638,96 +619,36 @@ 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_lock(); - struct fn_node* curr = flush_fn_list.head; while (curr) { curr->fn(); curr = curr->next; } - - 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. - // For example, a thread is making a fork while another one is - // loading a CU and so executing global initializer in this case - // the child process could inherit a bad list (e.g. bad tail) - // or could have the malloc in a wrong state. - 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 (one could have its own fork function) - // 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_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) { +void llvm_gcov_init(fn_ptr wfn, fn_ptr ffn) { 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); } } diff --git a/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp b/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp index ee57a5493261..2a302b547dc4 100644 --- a/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp +++ b/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp @@ -115,8 +115,7 @@ class GCOVProfiler { // list. Function * insertCounterWriteout(ArrayRef<std::pair<GlobalVariable *, MDNode *>>); - Function *insertReset(ArrayRef<std::pair<GlobalVariable *, MDNode *>>); - Function *insertFlush(Function *ResetF); + Function *insertFlush(ArrayRef<std::pair<GlobalVariable *, MDNode *>>); void AddFlushBeforeForkAndExec(); @@ -632,74 +631,35 @@ static bool shouldKeepInEntry(BasicBlock::iterator It) { } void GCOVProfiler::AddFlushBeforeForkAndExec() { - SmallVector<std::pair<bool, CallInst *>, 2> ForkAndExecs; + 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)) { - 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}); - } + 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); } } } } } - 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 they'll be lost after the exec** - FunctionType *FTy = FunctionType::get(Builder.getVoidTy(), {}, false); - 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); - } - } + // 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); } } @@ -891,8 +851,7 @@ bool GCOVProfiler::emitProfileArcs() { } Function *WriteoutF = insertCounterWriteout(CountersBySP); - Function *ResetF = insertReset(CountersBySP); - Function *FlushF = insertFlush(ResetF); + Function *FlushF = insertFlush(CountersBySP); // 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 @@ -910,14 +869,16 @@ bool GCOVProfiler::emitProfileArcs() { IRBuilder<> Builder(BB); FTy = FunctionType::get(Type::getVoidTy(*Ctx), false); - Type *Params[] = {PointerType::get(FTy, 0), PointerType::get(FTy, 0), - PointerType::get(FTy, 0)}; + Type *Params[] = { + PointerType::get(FTy, 0), + PointerType::get(FTy, 0) + }; FTy = FunctionType::get(Builder.getVoidTy(), Params, false); - // Initialize the environment and register the local writeout, flush and - // reset functions. + // Initialize the environment and register the local writeout and flush + // functions. FunctionCallee GCOVInit = M->getOrInsertFunction("llvm_gcov_init", FTy); - Builder.CreateCall(GCOVInit, {WriteoutF, FlushF, ResetF}); + Builder.CreateCall(GCOVInit, {WriteoutF, FlushF}); Builder.CreateRetVoid(); appendToGlobalCtors(*M, F, 0); @@ -1230,43 +1191,8 @@ Function *GCOVProfiler::insertCounterWriteout( return WriteoutF; } -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) { +Function *GCOVProfiler:: +insertFlush(ArrayRef<std::pair<GlobalVariable*, MDNode*> > CountersBySP) { FunctionType *FTy = FunctionType::get(Type::getVoidTy(*Ctx), false); Function *FlushF = M->getFunction("__llvm_gcov_flush"); if (!FlushF) @@ -1280,13 +1206,20 @@ Function *GCOVProfiler::insertFlush(Function *ResetF) { 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!"); - Builder.CreateCall(WriteoutF); - Builder.CreateCall(ResetF); + 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); + } Type *RetTy = FlushF->getReturnType(); if (RetTy == Type::getVoidTy(*Ctx)) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits