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

[Marco Castelluccio via Phabricator via cfe-commits]

marco-c added a comment.

Also, as we discussed, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93623#c5 regarding exec.



================
Comment at: compiler-rt/lib/profile/GCDAProfiling.c:665
+  gcov_lock();
+  // Avoid a concurrent modification of the lists during the fork
+  pid = fork();
----------------
Could you expand the comment explaining a situation where this could fail if we didn't lock?


================
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
----------------
Nit: do we need this check or can we just use the earlier one on pid == 0?


================
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;
----------------
What if we have a thread in another process making modifications to the 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()) {
----------------
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.


================
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(); }
----------------
Isn't this bug fixed by splitting the block?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74953