<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><div class="">Hi Calixte,</div><div class=""><br class=""></div>This commit is causing test failures on macOS: <a href="http://lab.llvm.org:8080/green/job/clang-stage1-RA/7116/" class="">http://lab.llvm.org:8080/green/job/clang-stage1-RA/7116/</a><div class=""><br class=""></div><div class="">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 <span style="caret-color: rgb(0, 0, 0); color: rgb(0, 0, 0); white-space: pre-wrap;" class="">instrprof-darwin-exports.c.</span></div><div class=""><font color="#000000" class=""><span style="caret-color: rgb(0, 0, 0); white-space: pre-wrap;" class=""><br class=""></span></font></div><div class=""><font color="#000000" class=""><span style="caret-color: rgb(0, 0, 0); white-space: pre-wrap;" class="">Thanks!</span></font></div><div class=""><font color="#000000" class=""><span style="caret-color: rgb(0, 0, 0); white-space: pre-wrap;" class="">Erik</span></font></div><div class=""><font color="#000000" class=""><span style="caret-color: rgb(0, 0, 0); white-space: pre-wrap;" class=""><br class=""></span></font><div><blockquote type="cite" class=""><div class="">On Feb 24, 2020, at 1:38 AM, Calixte Denizet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><br class="">Author: Calixte Denizet<br class="">Date: 2020-02-24T10:38:33+01:00<br class="">New Revision: 8f46269f0c1cc93b080931fd9dfeffc4d364004b<br class=""><br class="">URL: <a href="https://github.com/llvm/llvm-project/commit/8f46269f0c1cc93b080931fd9dfeffc4d364004b" class="">https://github.com/llvm/llvm-project/commit/8f46269f0c1cc93b080931fd9dfeffc4d364004b</a><br class="">DIFF: <a href="https://github.com/llvm/llvm-project/commit/8f46269f0c1cc93b080931fd9dfeffc4d364004b.diff" class="">https://github.com/llvm/llvm-project/commit/8f46269f0c1cc93b080931fd9dfeffc4d364004b.diff</a><br class=""><br class="">LOG: [profile] Don't dump counters when forking and don't reset when calling exec** functions<br class=""><br class="">Summary:<br class="">There is no need to write out gcdas when forking because we can just reset the counters in the parent process.<br class="">Let say a counter is N before the fork, then fork and this counter is set to 0 in the child process.<br class="">In the parent process, the counter is incremented by P and in the child process it's incremented by C.<br class="">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.<br class="">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.<br class="">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.<br class=""><br class="">Reviewers: marco-c<br class=""><br class="">Reviewed By: marco-c<br class=""><br class="">Subscribers: hiraditya, cfe-commits, #sanitizers, llvm-commits, sylvestre.ledru<br class=""><br class="">Tags: #clang, #sanitizers, #llvm<br class=""><br class="">Differential Revision: <a href="https://reviews.llvm.org/D74953" class="">https://reviews.llvm.org/D74953</a><br class=""><br class="">Added: <br class=""><br class=""><br class="">Modified: <br class=""> clang/lib/Driver/ToolChains/Darwin.cpp<br class=""> compiler-rt/lib/profile/GCDAProfiling.c<br class=""> llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp<br class=""><br class="">Removed: <br class=""><br class=""><br class=""><br class="">################################################################################<br class="">diff --git a/clang/lib/Driver/ToolChains/Darwin.cpp b/clang/lib/Driver/ToolChains/Darwin.cpp<br class="">index b32fad791bd3..c9f87e0e4355 100644<br class="">--- a/clang/lib/Driver/ToolChains/Darwin.cpp<br class="">+++ b/clang/lib/Driver/ToolChains/Darwin.cpp<br class="">@@ -1144,8 +1144,10 @@ void Darwin::addProfileRTLibs(const ArgList &Args,<br class=""> if (hasExportSymbolDirective(Args)) {<br class=""> if (ForGCOV) {<br class=""> addExportedSymbol(CmdArgs, "___gcov_flush");<br class="">+ addExportedSymbol(CmdArgs, "___gcov_fork");<br class=""> addExportedSymbol(CmdArgs, "_flush_fn_list");<br class=""> addExportedSymbol(CmdArgs, "_writeout_fn_list");<br class="">+ addExportedSymbol(CmdArgs, "_reset_fn_list");<br class=""> } else {<br class=""> addExportedSymbol(CmdArgs, "___llvm_profile_filename");<br class=""> addExportedSymbol(CmdArgs, "___llvm_profile_raw_version");<br class=""><br class="">diff --git a/compiler-rt/lib/profile/GCDAProfiling.c b/compiler-rt/lib/profile/GCDAProfiling.c<br class="">index 81f2cdd26450..7844b419b93c 100644<br class="">--- a/compiler-rt/lib/profile/GCDAProfiling.c<br class="">+++ b/compiler-rt/lib/profile/GCDAProfiling.c<br class="">@@ -32,8 +32,9 @@<br class=""> #include <windows.h><br class=""> #include "WindowsMMap.h"<br class=""> #else<br class="">-#include <sys/mman.h><br class=""> #include <sys/file.h><br class="">+#include <sys/mman.h><br class="">+#include <unistd.h><br class=""> #endif<br class=""><br class=""> #if defined(__FreeBSD__) && defined(__i386__)<br class="">@@ -62,27 +63,20 @@ typedef unsigned long long uint64_t;<br class=""> #include "InstrProfiling.h"<br class=""> #include "InstrProfilingUtil.h"<br class=""><br class="">+/* #define DEBUG_GCDAPROFILING */<br class="">+<br class=""> #ifndef _WIN32<br class=""> #include <pthread.h><br class="">-static pthread_mutex_t gcov_flush_mutex = PTHREAD_MUTEX_INITIALIZER;<br class="">-static __inline void gcov_flush_lock() {<br class="">- pthread_mutex_lock(&gcov_flush_mutex);<br class="">-}<br class="">-static __inline void gcov_flush_unlock() {<br class="">- pthread_mutex_unlock(&gcov_flush_mutex);<br class="">-}<br class="">+pthread_mutex_t gcov_mutex = PTHREAD_MUTEX_INITIALIZER;<br class="">+static __inline void gcov_lock() { pthread_mutex_lock(&gcov_mutex); }<br class="">+static __inline void gcov_unlock() { pthread_mutex_unlock(&gcov_mutex); }<br class=""> #else<br class=""> #include <windows.h><br class="">-static SRWLOCK gcov_flush_mutex = SRWLOCK_INIT;<br class="">-static __inline void gcov_flush_lock() {<br class="">- AcquireSRWLockExclusive(&gcov_flush_mutex);<br class="">-}<br class="">-static __inline void gcov_flush_unlock() {<br class="">- ReleaseSRWLockExclusive(&gcov_flush_mutex);<br class="">-}<br class="">+SRWLOCK gcov_mutex = SRWLOCK_INIT;<br class="">+static __inline void gcov_lock() { AcquireSRWLockExclusive(&gcov_mutex); }<br class="">+static __inline void gcov_unlock() { ReleaseSRWLockExclusive(&gcov_mutex); }<br class=""> #endif<br class=""><br class="">-/* #define DEBUG_GCDAPROFILING */<br class=""> /*<br class=""> * --- GCOV file format I/O primitives ---<br class=""> */<br class="">@@ -138,6 +132,12 @@ struct fn_list writeout_fn_list;<br class=""> */<br class=""> struct fn_list flush_fn_list;<br class=""><br class="">+/*<br class="">+ * A list of reset functions that our __gcov_reset() function should call,<br class="">+ * shared between all dynamic objects.<br class="">+ */<br class="">+struct fn_list reset_fn_list;<br class="">+<br class=""> static void fn_list_insert(struct fn_list* list, fn_ptr fn) {<br class=""> struct fn_node* new_node = malloc(sizeof(struct fn_node));<br class=""> new_node->fn = fn;<br class="">@@ -638,8 +638,25 @@ void llvm_register_flush_function(fn_ptr fn) {<br class=""> fn_list_insert(&flush_fn_list, fn);<br class=""> }<br class=""><br class="">+COMPILER_RT_VISIBILITY<br class="">+void llvm_register_reset_function(fn_ptr fn) {<br class="">+ fn_list_insert(&reset_fn_list, fn);<br class="">+}<br class="">+<br class="">+COMPILER_RT_VISIBILITY<br class="">+void llvm_reset_counters(void) {<br class="">+ struct fn_node *curr = reset_fn_list.head;<br class="">+<br class="">+ while (curr) {<br class="">+ if (curr->id == CURRENT_ID) {<br class="">+ curr->fn();<br class="">+ }<br class="">+ curr = curr->next;<br class="">+ }<br class="">+}<br class="">+<br class=""> void __gcov_flush() {<br class="">- gcov_flush_lock();<br class="">+ gcov_lock();<br class=""><br class=""> struct fn_node* curr = flush_fn_list.head;<br class=""><br class="">@@ -648,30 +665,69 @@ void __gcov_flush() {<br class=""> curr = curr->next;<br class=""> }<br class=""><br class="">- gcov_flush_unlock();<br class="">+ gcov_unlock();<br class=""> }<br class=""><br class="">+#if !defined(_WIN32)<br class="">+pid_t __gcov_fork() {<br class="">+ pid_t parent_pid = getpid();<br class="">+ pid_t pid;<br class="">+<br class="">+ gcov_lock();<br class="">+ // Avoid a concurrent modification of the lists during the fork.<br class="">+ // For example, a thread is making a fork while another one is<br class="">+ // loading a CU and so executing global initializer in this case<br class="">+ // the child process could inherit a bad list (e.g. bad tail)<br class="">+ // or could have the malloc in a wrong state.<br class="">+ pid = fork();<br class="">+ gcov_unlock();<br class="">+<br class="">+ if (pid == 0) {<br class="">+ pid_t child_pid = getpid();<br class="">+ if (child_pid != parent_pid) {<br class="">+ // The pid changed so we've a fork (one could have its own fork function)<br class="">+ // Just reset the counters for this child process<br class="">+ // No need to lock here since we just forked and cannot have any other<br class="">+ // threads.<br class="">+ llvm_reset_counters();<br class="">+ }<br class="">+ }<br class="">+ return pid;<br class="">+}<br class="">+#endif<br class="">+<br class=""> COMPILER_RT_VISIBILITY<br class=""> void llvm_delete_flush_function_list(void) {<br class=""> fn_list_remove(&flush_fn_list);<br class=""> }<br class=""><br class=""> COMPILER_RT_VISIBILITY<br class="">-void llvm_gcov_init(fn_ptr wfn, fn_ptr ffn) {<br class="">+void llvm_delete_reset_function_list(void) { fn_list_remove(&reset_fn_list); }<br class="">+<br class="">+COMPILER_RT_VISIBILITY<br class="">+void llvm_gcov_init(fn_ptr wfn, fn_ptr ffn, fn_ptr rfn) {<br class=""> static int atexit_ran = 0;<br class=""><br class="">+ gcov_lock();<br class="">+<br class=""> if (wfn)<br class=""> llvm_register_writeout_function(wfn);<br class=""><br class=""> if (ffn)<br class=""> llvm_register_flush_function(ffn);<br class=""><br class="">+ if (rfn)<br class="">+ llvm_register_reset_function(rfn);<br class="">+<br class="">+ gcov_unlock();<br class="">+<br class=""> if (atexit_ran == 0) {<br class=""> atexit_ran = 1;<br class=""><br class=""> /* Make sure we write out the data and delete the data structures. */<br class=""> atexit(llvm_delete_flush_function_list);<br class=""> atexit(llvm_delete_writeout_function_list);<br class="">+ atexit(llvm_delete_reset_function_list);<br class=""> atexit(llvm_writeout_files);<br class=""> }<br class=""> }<br class=""><br class="">diff --git a/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp b/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp<br class="">index 2a302b547dc4..ee57a5493261 100644<br class="">--- a/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp<br class="">+++ b/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp<br class="">@@ -115,7 +115,8 @@ class GCOVProfiler {<br class=""> // list.<br class=""> Function *<br class=""> insertCounterWriteout(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);<br class="">- Function *insertFlush(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);<br class="">+ Function *insertReset(ArrayRef<std::pair<GlobalVariable *, MDNode *>>);<br class="">+ Function *insertFlush(Function *ResetF);<br class=""><br class=""> void AddFlushBeforeForkAndExec();<br class=""><br class="">@@ -631,35 +632,74 @@ static bool shouldKeepInEntry(BasicBlock::iterator It) {<br class=""> }<br class=""><br class=""> void GCOVProfiler::AddFlushBeforeForkAndExec() {<br class="">- SmallVector<Instruction *, 2> ForkAndExecs;<br class="">+ SmallVector<std::pair<bool, CallInst *>, 2> ForkAndExecs;<br class=""> for (auto &F : M->functions()) {<br class=""> auto *TLI = &GetTLI(F);<br class=""> for (auto &I : instructions(F)) {<br class=""> if (CallInst *CI = dyn_cast<CallInst>(&I)) {<br class=""> if (Function *Callee = CI->getCalledFunction()) {<br class=""> LibFunc LF;<br class="">- if (TLI->getLibFunc(*Callee, LF) &&<br class="">- (LF == LibFunc_fork || LF == LibFunc_execl ||<br class="">- LF == LibFunc_execle || LF == LibFunc_execlp ||<br class="">- LF == LibFunc_execv || LF == LibFunc_execvp ||<br class="">- LF == LibFunc_execve || LF == LibFunc_execvpe ||<br class="">- LF == LibFunc_execvP)) {<br class="">- ForkAndExecs.push_back(&I);<br class="">+ if (TLI->getLibFunc(*Callee, LF)) {<br class="">+ if (LF == LibFunc_fork) {<br class="">+#if !defined(_WIN32)<br class="">+ ForkAndExecs.push_back({true, CI});<br class="">+#endif<br class="">+ } else if (LF == LibFunc_execl || LF == LibFunc_execle ||<br class="">+ LF == LibFunc_execlp || LF == LibFunc_execv ||<br class="">+ LF == LibFunc_execvp || LF == LibFunc_execve ||<br class="">+ LF == LibFunc_execvpe || LF == LibFunc_execvP) {<br class="">+ ForkAndExecs.push_back({false, CI});<br class="">+ }<br class=""> }<br class=""> }<br class=""> }<br class=""> }<br class=""> }<br class=""><br class="">- // We need to split the block after the fork/exec call<br class="">- // because else the counters for the lines after will be<br class="">- // the same as before the call.<br class="">- for (auto I : ForkAndExecs) {<br class="">- IRBuilder<> Builder(I);<br class="">- FunctionType *FTy = FunctionType::get(Builder.getVoidTy(), {}, false);<br class="">- FunctionCallee GCOVFlush = M->getOrInsertFunction("__gcov_flush", FTy);<br class="">- Builder.CreateCall(GCOVFlush);<br class="">- I->getParent()->splitBasicBlock(I);<br class="">+ for (auto F : ForkAndExecs) {<br class="">+ IRBuilder<> Builder(F.second);<br class="">+ BasicBlock *Parent = F.second->getParent();<br class="">+ auto NextInst = ++F.second->getIterator();<br class="">+<br class="">+ if (F.first) {<br class="">+ // We've a fork so just reset the counters in the child process<br class="">+ FunctionType *FTy = FunctionType::get(Builder.getInt32Ty(), {}, false);<br class="">+ FunctionCallee GCOVFork = M->getOrInsertFunction("__gcov_fork", FTy);<br class="">+ F.second->setCalledFunction(GCOVFork);<br class="">+ if (NextInst != Parent->end()) {<br class="">+ // We split just after the fork to have a counter for the lines after<br class="">+ // Anyway there's a bug:<br class="">+ // void foo() { fork(); }<br class="">+ // void bar() { foo(); blah(); }<br class="">+ // then "blah();" will be called 2 times but showed as 1<br class="">+ // because "blah()" belongs to the same block as "foo();"<br class="">+ Parent->splitBasicBlock(NextInst);<br class="">+<br class="">+ // back() is a br instruction with a debug location<br class="">+ // equals to the one from NextAfterFork<br class="">+ // So to avoid to have two debug locs on two blocks just change it<br class="">+ DebugLoc Loc = F.second->getDebugLoc();<br class="">+ Parent->back().setDebugLoc(Loc);<br class="">+ }<br class="">+ } else {<br class="">+ // Since the process is replaced by a new one we need to write out gcdas<br class="">+ // No need to reset the counters since they'll be lost after the exec**<br class="">+ FunctionType *FTy = FunctionType::get(Builder.getVoidTy(), {}, false);<br class="">+ FunctionCallee WriteoutF =<br class="">+ M->getOrInsertFunction("llvm_writeout_files", FTy);<br class="">+ Builder.CreateCall(WriteoutF);<br class="">+ if (NextInst != Parent->end()) {<br class="">+ DebugLoc Loc = F.second->getDebugLoc();<br class="">+ Builder.SetInsertPoint(&*NextInst);<br class="">+ // If the exec** fails we must reset the counters since they've been<br class="">+ // dumped<br class="">+ FunctionCallee ResetF =<br class="">+ M->getOrInsertFunction("llvm_reset_counters", FTy);<br class="">+ Builder.CreateCall(ResetF)->setDebugLoc(Loc);<br class="">+ Parent->splitBasicBlock(NextInst);<br class="">+ Parent->back().setDebugLoc(Loc);<br class="">+ }<br class="">+ }<br class=""> }<br class=""> }<br class=""><br class="">@@ -851,7 +891,8 @@ bool GCOVProfiler::emitProfileArcs() {<br class=""> }<br class=""><br class=""> Function *WriteoutF = insertCounterWriteout(CountersBySP);<br class="">- Function *FlushF = insertFlush(CountersBySP);<br class="">+ Function *ResetF = insertReset(CountersBySP);<br class="">+ Function *FlushF = insertFlush(ResetF);<br class=""><br class=""> // Create a small bit of code that registers the "__llvm_gcov_writeout" to<br class=""> // be executed at exit and the "__llvm_gcov_flush" function to be executed<br class="">@@ -869,16 +910,14 @@ bool GCOVProfiler::emitProfileArcs() {<br class=""> IRBuilder<> Builder(BB);<br class=""><br class=""> FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);<br class="">- Type *Params[] = {<br class="">- PointerType::get(FTy, 0),<br class="">- PointerType::get(FTy, 0)<br class="">- };<br class="">+ Type *Params[] = {PointerType::get(FTy, 0), PointerType::get(FTy, 0),<br class="">+ PointerType::get(FTy, 0)};<br class=""> FTy = FunctionType::get(Builder.getVoidTy(), Params, false);<br class=""><br class="">- // Initialize the environment and register the local writeout and flush<br class="">- // functions.<br class="">+ // Initialize the environment and register the local writeout, flush and<br class="">+ // reset functions.<br class=""> FunctionCallee GCOVInit = M->getOrInsertFunction("llvm_gcov_init", FTy);<br class="">- Builder.CreateCall(GCOVInit, {WriteoutF, FlushF});<br class="">+ Builder.CreateCall(GCOVInit, {WriteoutF, FlushF, ResetF});<br class=""> Builder.CreateRetVoid();<br class=""><br class=""> appendToGlobalCtors(*M, F, 0);<br class="">@@ -1191,8 +1230,43 @@ Function *GCOVProfiler::insertCounterWriteout(<br class=""> return WriteoutF;<br class=""> }<br class=""><br class="">-Function *GCOVProfiler::<br class="">-insertFlush(ArrayRef<std::pair<GlobalVariable*, MDNode*> > CountersBySP) {<br class="">+Function *GCOVProfiler::insertReset(<br class="">+ ArrayRef<std::pair<GlobalVariable *, MDNode *>> CountersBySP) {<br class="">+ FunctionType *FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);<br class="">+ Function *ResetF = M->getFunction("__llvm_gcov_reset");<br class="">+ if (!ResetF)<br class="">+ ResetF = Function::Create(FTy, GlobalValue::InternalLinkage,<br class="">+ "__llvm_gcov_reset", M);<br class="">+ else<br class="">+ ResetF->setLinkage(GlobalValue::InternalLinkage);<br class="">+ ResetF->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);<br class="">+ ResetF->addFnAttr(Attribute::NoInline);<br class="">+ if (Options.NoRedZone)<br class="">+ ResetF->addFnAttr(Attribute::NoRedZone);<br class="">+<br class="">+ BasicBlock *Entry = BasicBlock::Create(*Ctx, "entry", ResetF);<br class="">+ IRBuilder<> Builder(Entry);<br class="">+<br class="">+ // Zero out the counters.<br class="">+ for (const auto &I : CountersBySP) {<br class="">+ GlobalVariable *GV = I.first;<br class="">+ Constant *Null = Constant::getNullValue(GV->getValueType());<br class="">+ Builder.CreateStore(Null, GV);<br class="">+ }<br class="">+<br class="">+ Type *RetTy = ResetF->getReturnType();<br class="">+ if (RetTy == Type::getVoidTy(*Ctx))<br class="">+ Builder.CreateRetVoid();<br class="">+ else if (RetTy->isIntegerTy())<br class="">+ // Used if __llvm_gcov_reset was implicitly declared.<br class="">+ Builder.CreateRet(ConstantInt::get(RetTy, 0));<br class="">+ else<br class="">+ report_fatal_error("invalid return type for __llvm_gcov_reset");<br class="">+<br class="">+ return ResetF;<br class="">+}<br class="">+<br class="">+Function *GCOVProfiler::insertFlush(Function *ResetF) {<br class=""> FunctionType *FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);<br class=""> Function *FlushF = M->getFunction("__llvm_gcov_flush");<br class=""> if (!FlushF)<br class="">@@ -1206,20 +1280,13 @@ insertFlush(ArrayRef<std::pair<GlobalVariable*, MDNode*> > CountersBySP) {<br class=""> FlushF->addFnAttr(Attribute::NoRedZone);<br class=""><br class=""> BasicBlock *Entry = BasicBlock::Create(*Ctx, "entry", FlushF);<br class="">+ IRBuilder<> Builder(Entry);<br class=""><br class="">- // Write out the current counters.<br class=""> Function *WriteoutF = M->getFunction("__llvm_gcov_writeout");<br class=""> assert(WriteoutF && "Need to create the writeout function first!");<br class=""><br class="">- IRBuilder<> Builder(Entry);<br class="">- Builder.CreateCall(WriteoutF, {});<br class="">-<br class="">- // Zero out the counters.<br class="">- for (const auto &I : CountersBySP) {<br class="">- GlobalVariable *GV = I.first;<br class="">- Constant *Null = Constant::getNullValue(GV->getValueType());<br class="">- Builder.CreateStore(Null, GV);<br class="">- }<br class="">+ Builder.CreateCall(WriteoutF);<br class="">+ Builder.CreateCall(ResetF);<br class=""><br class=""> Type *RetTy = FlushF->getReturnType();<br class=""> if (RetTy == Type::getVoidTy(*Ctx))<br class=""><br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></div></blockquote></div><br class=""></div></body></html>