[llvm] 546918c - Revert "[compiler-rt] Add a critical section when flushing gcov counters"
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 26 04:30:57 PST 2020
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))
More information about the llvm-commits
mailing list