[PATCH] D74953: [profile] Don't dump counters when forking and don't reset when calling exec** functions

[calixte via Phabricator via llvm-commits]

calixte added inline comments.


================
Comment at: compiler-rt/lib/profile/GCDAProfiling.c:671
+    pid_t child_pid = getpid();
+    if (child_pid != parent_pid) {
+      // The pid changed so we've a fork
----------------
marco-c wrote:
> Nit: do we need this check or can we just use the earlier one on pid == 0?
I added this check to be sure we had a true fork in case of someone wrote its own fork function.


================
Comment at: compiler-rt/lib/profile/GCDAProfiling.c:675
+      // No need to lock here since we just forked and cannot have any other
+      // threads.
+      struct fn_node *curr = reset_fn_list.head;
----------------
marco-c wrote:
> What if we have a thread in another process making modifications to the list?
When forking, only the thread in the parent process is copied so in the child process we've only one thread.
When forking, the parent and the child process share the same memory until some change (Copy-on-Write), so even if in the parent process a list is changed then the memory will be copied before the change and then the child process will have an unchanged list. 


================
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:635
 void GCOVProfiler::AddFlushBeforeForkAndExec() {
-  SmallVector<Instruction *, 2> ForkAndExecs;
+  SmallVector<std::pair<bool, CallInst *>, 2> ForkAndExecs;
   for (auto &F : M->functions()) {
----------------
marco-c wrote:
> Since we are now mostly doing different things on forks and execs, we could remove this vector and just do the operations directly instead of adding to the vec.
If we make the insertions of new code in the second for loop, we'll invalidate the iterator used in this loop. 


================
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:671
+        // We split just after the fork to have a counter for the lines after
+        // Anyway there's a bug:
+        // void foo() { fork(); }
----------------
marco-c wrote:
> Isn't this bug fixed by splitting the block?
With the example shew here, the split is in foo but not in bar, blah() should be in another block


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74953/new/

https://reviews.llvm.org/D74953