[gcov] Move llvm_writeout_files from atexit to a static destructor

[MaskRay]

Summary

atexit registered functions run earlier so __attribute__((destructor))
annotated functions cannot be tracked.

Set a priority of 100 (compatible with GCC 7 onwards) to track
destructors and destructors whose priorities are greater than 100.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7970

[calixte]

LGTM

[marco-c]

There is only a problem with the (possibly very rare) case where calling exec* fails:

• the process calls exec*;
• llvm_writeout_files is called, which removes the writeout list;
• exec* fails;
• the process continues, the coverage is lost because when it ends there is no writeout list anymore.

For reference, we inject llvm_writeout_files before exec* here: https://github.com/llvm/llvm-project/blob/16cc759ebd56fddd2a9d2437b810ced885ebda73/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp#L684.

[MaskRay]

In D82253#2106360, @marco-c wrote:

There is only a problem with the (possibly very rare) case where calling exec* fails:

• the process calls exec*;
• llvm_writeout_files is called, which removes the writeout list;
• exec* fails;
• the process continues, the coverage is lost because when it ends there is no writeout list anymore.

For reference, we inject llvm_writeout_files before exec* here: https://github.com/llvm/llvm-project/blob/16cc759ebd56fddd2a9d2437b810ced885ebda73/llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp#L684.

How does the patch cause a new problem? This is a pre-existing problem with the previous atexit approach.

[calixte]

It's because you moved fn_list_remove(&writeout_fn_list); in llvm_writeout_files(void)

[MaskRay]

In D82253#2107066, @calixte wrote:

It's because you moved fn_list_remove(&writeout_fn_list); in llvm_writeout_files(void)

Can you elaborate how deleting atexit(llvm_delete_writeout_function_list); can be a problem? Please also propose a solution if the very rare use case fails. You can add a test if you want to demonstrate this is a feature we want to support.

I have to move it because we need to postpone llvm_writeout_files to a later stage. The function (via a static destructor) still runs if exec* fails.

[marco-c]

How does the patch cause a new problem? This is a pre-existing problem with the previous atexit approach.

Can you elaborate how deleting atexit(llvm_delete_writeout_function_list); can be a problem?

With the atexit approach, we were not deleting the writeout function list before exec.

With atexit:

• process calls exec;
• we write out the files;
• exec fails;
• at some point the process ends;
• we write out the files (so we don't lose coverage information collected between when exec failed and now);
• we delete the writeout function list.

With static destructor:

• process calls exec;
• we write out the files;
• we delete the writeout function list;
• exec fails;
• at some point the process ends;
• we can't write out the files because the writeout function list was deleted (and so we lose coverage information).

Please also propose a solution if the very rare use case fails. You can add a test if you want to demonstrate this is a feature we want to support.

Aren't these usually responsibility of the author of the proposed change? :)

Anyway, a couple possible solutions:

1. Don't merge llvm_delete_writeout_function_list into llvm_writeout_files, make llvm_delete_writeout_function_list a static destructor too (making it run after llvm_writeout_files);
2. Create a new function llvm_writeout_destructor (please find a better name if you do this!), which is a static destructor and calls llvm_writeout_files and then llvm_delete_writeout_function_list. So before exec* we keep calling llvm_writeout_files without deleting the writeout function list.

I personally prefer the second option.

[MaskRay]

In D82253#2107791, @marco-c wrote:

How does the patch cause a new problem? This is a pre-existing problem with the previous atexit approach.

Can you elaborate how deleting atexit(llvm_delete_writeout_function_list); can be a problem?

With the atexit approach, we were not deleting the writeout function list before exec.

With atexit:

• process calls exec;
• we write out the files;
• exec fails;
• at some point the process ends;
• we write out the files (so we don't lose coverage information collected between when exec failed and now);
• we delete the writeout function list.

With static destructor:

• process calls exec;
• we write out the files;
• we delete the writeout function list;
• exec fails;
• at some point the process ends;
• we can't write out the files because the writeout function list was deleted (and so we lose coverage information).

Please also propose a solution if the very rare use case fails. You can add a test if you want to demonstrate this is a feature we want to support.

Aren't these usually responsibility of the author of the proposed change? :)

Anyway, a couple possible solutions:

1. Don't merge llvm_delete_writeout_function_list into llvm_writeout_files, make llvm_delete_writeout_function_list a static destructor too (making it run after llvm_writeout_files);
2. Create a new function llvm_writeout_destructor (please find a better name if you do this!), which is a static destructor and calls llvm_writeout_files and then llvm_delete_writeout_function_list. So before exec* we keep calling llvm_writeout_files without deleting the writeout function list.

I personally prefer the second option.

Thanks! I did not notice that llvm_writeout_files could be called by execl, so I failed to notice the issue you reported. (I thought it was an edge case, thus asked you to clarify.)

[MaskRay]

Handle execl and improve test

[MaskRay]

[[@LINE]] -> #@LINE

The former is legacy syntax.

[MaskRay]

@marco-c Hi Marco, are you happy with the new version?

[MaskRay]

In D82253#2114893, @MaskRay wrote:

@marco-c Hi Marco, are you happy with the new version?

@marco-c :) (Sorry to bother you again in less than one week. I hope this will not take up much of your time)

[marco-c]

Sorry, I was on holiday.
Yes, this LGTM!

compiler-rt/lib/profile/GCDAProfiling.c

void llvm_writeout_files(void) {
   while (curr) {
     if (curr->id == CURRENT_ID) {
       curr->fn();
     }
     curr = curr->next;
   }
 }
 
-COMPILER_RT_VISIBILITY
-void llvm_delete_writeout_function_list(void) {
+#ifndef _WIN32
+// __attribute__((destructor)) and destructors whose priorities are greater than
+// 100 run before this function and can thus be tracked. The priority is
+// compatible with GCC 7 onwards.
+__attribute__((destructor(100)))
+#endif
+static void llvm_writeout_and_clear(void) {
+  llvm_writeout_files();
   fn_list_remove(&writeout_fn_list);
 }
 
 COMPILER_RT_VISIBILITY
 void llvm_register_flush_function(fn_ptr fn) {
   fn_list_insert(&flush_fn_list, fn);
 }
 
if (rfn)
     llvm_register_reset_function(rfn);
 
   if (atexit_ran == 0) {
     atexit_ran = 1;
 
     /* Make sure we write out the data and delete the data structures. */
     atexit(llvm_delete_reset_function_list);
     atexit(llvm_delete_flush_function_list);
-    atexit(llvm_delete_writeout_function_list);
-    atexit(llvm_writeout_files);
+#ifdef _WIN32
+    atexit(llvm_writeout_and_clear);
+#endif
   }
 }
 
 #endif

compiler-rt/test/profile/Posix/gcov-destructor.c

+/// Test that destructors and destructors whose priorities are greater than 100 are tracked.
+// RUN: mkdir -p %t.dir && cd %t.dir
+// RUN: %clang --coverage %s -o %t
+// RUN: rm -f gcov-destructor.gcda && %run %t
+// RUN: llvm-cov gcov -t gcov-destructor.gcda | FileCheck %s
+
+#include <unistd.h>
+
+void before_exec() {}                   // CHECK:      1: [[#@LINE]]:void before_exec
+void after_exec() {}                    // CHECK-NEXT: 1: [[#@LINE]]:void after_exec
+
+__attribute__((constructor))            // CHECK:      -: [[#@LINE]]:__attribute__
+void constructor() {}                   // CHECK-NEXT: 1: [[#@LINE]]:
+
+/// Runs before __llvm_gcov_writeout.
+__attribute__((destructor))             // CHECK:      -: [[#@LINE]]:__attribute__
+void destructor() {}                    // CHECK-NEXT: 1: [[#@LINE]]:
+
+__attribute__((destructor(101)))        // CHECK:      -: [[#@LINE]]:__attribute__
+void destructor_101() {}                // CHECK-NEXT: 1: [[#@LINE]]:
+
+/// Runs after __llvm_gcov_writeout.
+__attribute__((destructor(99)))         // CHECK:      -: [[#@LINE]]:__attribute__
+void destructor_99() {}                 // CHECK-NEXT: #####: [[#@LINE]]:
+
+int main() {
+  before_exec();
+  // Implicit writeout.
+  execl("/not_exist", "not_exist", (char *)0);
+  // Still tracked.
+  after_exec();
+  return 0;
+}