[compiler-rt] 8f46269 - [profile] Don't dump counters when forking and don't reset when calling exec** functions
Erik Pilkington via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 24 13:52:04 PST 2020
Hi Calixte,
This commit is causing test failures on macOS: http://lab.llvm.org:8080/green/job/clang-stage1-RA/7116/ <http://lab.llvm.org:8080/green/job/clang-stage1-RA/7116/>
It looks like the problem is that the symbol `_gcov_mutex` is now being exported (as it is no longer marked static), but isn’t in the export list. Can you please fix this? The failing test is instrprof-darwin-exports.c.
Thanks!
Erik
> On Feb 24, 2020, at 1:38 AM, Calixte Denizet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
>
> Author: Calixte Denizet
> Date: 2020-02-24T10:38:33+01:00
> New Revision: 8f46269f0c1cc93b080931fd9dfeffc4d364004b
>
> URL: https://github.com/llvm/llvm-project/commit/8f46269f0c1cc93b080931fd9dfeffc4d364004b
> DIFF: https://github.com/llvm/llvm-project/commit/8f46269f0c1cc93b080931fd9dfeffc4d364004b.diff
>
> LOG: [profile] Don't dump counters when forking and don't reset when calling exec** functions
>
> 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.
>
> Reviewers: marco-c
>
> Reviewed By: marco-c
>
> Subscribers: hiraditya, cfe-commits, #sanitizers, llvm-commits, sylvestre.ledru
>
> Tags: #clang, #sanitizers, #llvm
>
> Differential Revision: https://reviews.llvm.org/D74953
>
> 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 b32fad791bd3..c9f87e0e4355 100644
> --- a/clang/lib/Driver/ToolChains/Darwin.cpp
> +++ b/clang/lib/Driver/ToolChains/Darwin.cpp
> @@ -1144,8 +1144,10 @@ 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 81f2cdd26450..7844b419b93c 100644
> --- a/compiler-rt/lib/profile/GCDAProfiling.c
> +++ b/compiler-rt/lib/profile/GCDAProfiling.c
> @@ -32,8 +32,9 @@
> #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__)
> @@ -62,27 +63,20 @@ typedef unsigned long long uint64_t;
> #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 ---
> */
> @@ -138,6 +132,12 @@ 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,8 +638,25 @@ 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;
>
> @@ -648,30 +665,69 @@ void __gcov_flush() {
> 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.
> + // 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_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);
> }
> }
>
> diff --git a/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp b/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp
> index 2a302b547dc4..ee57a5493261 100644
> --- a/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp
> +++ b/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp
> @@ -115,7 +115,8 @@ class GCOVProfiler {
> // 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();
>
> @@ -631,35 +632,74 @@ static bool shouldKeepInEntry(BasicBlock::iterator It) {
> }
>
> 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);
> - FunctionType *FTy = FunctionType::get(Builder.getVoidTy(), {}, false);
> - FunctionCallee GCOVFlush = M->getOrInsertFunction("__gcov_flush", FTy);
> - Builder.CreateCall(GCOVFlush);
> - I->getParent()->splitBasicBlock(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);
> + }
> + }
> }
> }
>
> @@ -851,7 +891,8 @@ bool GCOVProfiler::emitProfileArcs() {
> }
>
> 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
> @@ -869,16 +910,14 @@ bool GCOVProfiler::emitProfileArcs() {
> 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);
> @@ -1191,8 +1230,43 @@ Function *GCOVProfiler::insertCounterWriteout(
> 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)
> @@ -1206,20 +1280,13 @@ insertFlush(ArrayRef<std::pair<GlobalVariable*, MDNode*> > CountersBySP) {
> 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))
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200224/95e604f8/attachment.html>
More information about the llvm-commits
mailing list