Add llvm_gcov_flush to be called outside a shared library

[chh]

Summary

__gcov_flush is hidden.
For applications to dump profiling data of selected .so files,
they can use dlsym to find and call llvm_gcov_flush in each .so file.
See https://bugs.llvm.org/show_bug.cgi?id=27224#c5

[davidxl]

With this change, gcov_flush will resolve to the copy in the main executable for each shared library -- this is not the desired the behavior.

[chh]

If we use the unit test case, call __gcov_flush from the main function,
and dump static variables in GCDAProfiling.c, we can see that __gcov_flush
is resolved to the same copy for func.shared, func2.shared, and main.
However, when __gcov_flush is called from main and from f1_flush and f2_flush,
they use different copies of static variables defined in GCDAProfiling.c.
The "flush_fn_head" and its flush functions are different, so 3 calls of
__gcov_flush will flush to 3 different output files:

instrprof-dlopen-func.gcda
instrprof-dlopen-func2.gcda
instrprof-dlopen-dlclose-main.gcda

This keeps current Android use cases working as before __gcov_flush is hidden.
Is there other use case that could be broken by this change?

[davidxl]

The behavior will change if the shared libraries are not 'dlopen'ed but loaded at program startup time.

[chh]

Yes, calling __gcov_flush within .so files are different,
but it's a revert of https://reviews.llvm.org/D38124.
I think https://bugs.llvm.org/show_bug.cgi?id=27224
can be fixed by hiding only llvm_gcda_* functions,
without any change to __gcov_flush.

(1) Before https://reviews.llvm.org/D38124:

• Calling __gcov_flush within .so or main function dumps to main gcda file.
• Android's dlsym() lookup/call of __gcov_flush dumps to .so file specific gcda files.

(2) After https://reviews.llvm.org/D38124:

• Android's dlsym() cannot find/call __gcov_flush.
• Calling __gcov_flush from main works as in (1).
• Calling __gcov_flush from .so works differently; it will dump to .so specific gcda file, not the main one.

(3) With this change, we revert __gcov_flush behavior back to (1).

Is there any application that needs to call __gcov_flush within .so
and expects the output to .so specific gcda file like in (2)?
I think the behavior of (1) is more desirable.
If a main program wants to dump to .so specific gcda file, like Android,
it can use dlsym() to look up .so specific __gcov_flush.

[belleyb]

@chh I had a chance to try out your proposed changes. It's not causing us any trouble. In fact, __gcov_flush() is not even used at all (at least in LLVM 5.0.1).. I can recompile llvm, compiler_rt and clang and re-run all the tests with __gcov_flush commented out! No problem.

I would suggest adding a bit more documentation to __gcov_flush(), thus describing what those "special cases" are...

[marco-c]

In D45454#1070884, @belleyb wrote:

@chh I had a chance to try out your proposed changes. It's not causing us any trouble. In fact, __gcov_flush() is not even used at all (at least in LLVM 5.0.1).. I can recompile llvm, compiler_rt and clang and re-run all the tests with __gcov_flush commented out! No problem.

I would suggest adding a bit more documentation to __gcov_flush(), thus describing what those "special cases" are...

__gcov_flush is only used if you actually call it (it's needed for example if you want to profile only part of your program).

In GCC, gcov_flush is not hidden, so perhaps we should do the same to keep the same behavior? I've also submitted https://reviews.llvm.org/D48538, which is making gcov_flush flush counters for all shared libraries (like GCC does, with the same caveat: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83879).

[chh]

In D45454#1142197, @marco-c wrote:

In D45454#1070884, @belleyb wrote:

@chh I had a chance to try out your proposed changes. It's not causing us any trouble. In fact, __gcov_flush() is not even used at all (at least in LLVM 5.0.1).. I can recompile llvm, compiler_rt and clang and re-run all the tests with __gcov_flush commented out! No problem.

I would suggest adding a bit more documentation to __gcov_flush(), thus describing what those "special cases" are...

__gcov_flush is only used if you actually call it (it's needed for example if you want to profile only part of your program).

In GCC, gcov_flush is not hidden, so perhaps we should do the same to keep the same behavior? I've also submitted https://reviews.llvm.org/D48538, which is making gcov_flush flush counters for all shared libraries (like GCC does, with the same caveat: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83879).

I have no problem keeping these functions compatible with GCC.
My earlier proposal and David's comment in the mailing list seemed to be lost and not showing here.
So, let me summarize the case here. This change should make __gcov_flush not hidden as before in GCC,
but earlier change made it hidden as well as other llvm_gov_* functions.
Could we have both __gov_flush and llvm_gov_flush functions, one unhidden and one hidden?

[marco-c]

Wouldn't it be better to keep compatibility with GCC and make
__gcov_flush have default visibility?

• Marco.

Il 26/06/2018 00:21, Xinliang David Li ha scritto:

I don't have an objection having another interface which is just a
simple wrapper to __gcov_flush but with default visibility. Also
clearly document its usage and behavior.

David

[davidxl]

GCC's behavior is not documented and it also has changed over the years.

Unless there is a bug, changing LLVM's gcov_flush visibility can potentially break clients that depend on this behavior.

[srhines]

In D45454#1144099, @davidxl wrote:

GCC's behavior is not documented and it also has changed over the years.

Unless there is a bug, changing LLVM's gcov_flush visibility can potentially break clients that depend on this behavior.

I think that's the issue though. LLVM changed visibility of this function recently too. We had Android code depending on this function being visible, and that broke.

[marco-c]

Yes, the behavior changed very recently, I would be surprised if
somebody came to depend on it. It's more likely that some clients are
depending on the old behavior.

• Marco.

Il 26/06/2018 22:43, Stephen Hines via Phabricator ha scritto:

srhines added a comment.

In https://reviews.llvm.org/D45454#1144099, @davidxl wrote:

GCC's behavior is not documented and it also has changed over the years.

Unless there is a bug, changing LLVM's gcov_flush visibility can potentially break clients that depend on this behavior.

I think that's the issue though. LLVM changed visibility of this function recently too. We had Android code depending on this function being visible, and that broke.

https://reviews.llvm.org/D45454

[chh]

Now keep __gcov_flush hidden as libgcov; add llvm_gcov_flush to call from outside of .so files.

[srhines]

Thanks for picking this up again and updating the change to add llvm_gcov_flush().

test/profile/Inputs/instrprof-dlopen-dlclose-main.c

21

Should also clear dlerror() before this call and check that dlerror() returned a non-NULL pointer indicating that this search failed.

[marco-c]

Why keeping a gcov_flush which is incompatible with GCC and previous versions of LLVM and adding a llvm_gcov_flush which is compatible? I feel the other way around (or even just having an unhidden "gcov_flush" and not introducing "llvm_gcov_flush") would be more sensible.

[davidxl]

With the current gcov_flush implementation in LLVM, making gcov_flush's visibility to be default will simply lead to wrong behavior. GCC libgcov's implementation is more elaborate -- it allows gcov_flush to dump gcda data for all dynamic objects while making sure gcov_exit only dumps the gcda files from the same dynamic module (otherwise, there will be wrong profile merging). This is done via two levels of linked list.

The patch https://reviews.llvm.org/D48538 is in the right direction, but not complete yet.

[chh]

Marco, latest patch does not change gcov_flush, which is also hidden in libgcov.
Android coverage test programs have depended on an earlier compiler-rt that did not hide gcov_flush.
If that's the only use case broken by recent change of compiler-rt, which hide gcov_flush,
I think it is okay to keep compiler-rt the same as it is now.
Adding a visible llvm_gcov_flush will allow Android coverage test program to use it and dump system .so profiling data,
from the main test program.
Keeping an hidden gcov_flush has the purpose that David mentioned earlier, to let each .so file dump its own profiling data.

[marco-c]

OK! Sounds good to me to keep it hidden until
https://reviews.llvm.org/D48538 is done (I'm going to try to finish it
soon).

Il 29/06/2018 19:34, David Li via Phabricator ha scritto:

davidxl added a comment.

With the current gcov_flush implementation in LLVM, making gcov_flush's visibility to be default will simply lead to wrong behavior. GCC libgcov's implementation is more elaborate -- it allows gcov_flush to dump gcda data for all dynamic objects while making sure gcov_exit only dumps the gcda files from the same dynamic module (otherwise, there will be wrong profile merging). This is done via two levels of linked list.

The patch https://reviews.llvm.org/D48538 is in the right direction, but not complete yet.

https://reviews.llvm.org/D45454

[chh]

Added calls to dlerror() before other dl* functions.

[davidxl]

lib/profile/GCDAProfiling.c

546

Just document this interface has non-hidden visibility and will resolve to a unified copy. The right underlying behavior is for it to dump profiles from all dynamic modules, but it is not there until Marco's patch is in.

In other words, do not need to document the current behavior for now.

[chh]

check dlerror() where it shouldn't be NULL

test/profile/Inputs/instrprof-dlopen-dlclose-main.c

21

Look up of __gcov_flush is expected to fail because it's hidden.
Other places checks for returned value and report dlerror() messages.

[srhines]

LGTM. Thanks for making the checking more clear. This should help bridge the gap for now, so that we can resume getting per-library profile data in Android.

[davidxl]

lgtm

lib/profile/GCDAProfiling.c

void llvm_register_flush_function(flush_fn fn) {
   if (!flush_fn_head) {
     flush_fn_head = flush_fn_tail = new_node;
   } else {
     flush_fn_tail->next = new_node;
     flush_fn_tail = new_node;
   }
 }
 
+// __gcov_flush is hidden. When called in a .so file,
+// it dumps profile data of the calling .so file.
+// If a main program needs to dump profile data of each linked
+// .so files, it should use dlsym to find and call llvm_gcov_flush.
 COMPILER_RT_VISIBILITY
 void __gcov_flush() {
   struct flush_fn_node *curr = flush_fn_head;
 
   while (curr) {
     curr->fn();
     curr = curr->next;
   }
 }
 
+// llvm_gcov_flush is not hidden for a program to use dlsym to
+// find and call. In that case, it dumps profile data of a .so file.
+// If it is called directly inside a .so file, the unified copy of
+// llvm_gcov_flush might dump data of other .so file or the main module.
+void llvm_gcov_flush() {
+  __gcov_flush();
+}
+
 COMPILER_RT_VISIBILITY
 void llvm_delete_flush_function_list(void) {
   while (flush_fn_head) {
     struct flush_fn_node *node = flush_fn_head;
     flush_fn_head = flush_fn_head->next;
     free(node);
   }
 

test/profile/Inputs/instrprof-dlopen-dlclose-main.c

 #include <dlfcn.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 int main(int argc, char *argv[]) {
   dlerror();
   void *f1_handle = dlopen("func.shared", RTLD_LAZY | RTLD_GLOBAL);
   if (f1_handle == NULL) {
     fprintf(stderr, "unable to open 'func.shared': %s\n", dlerror());
     return EXIT_FAILURE;
   }
 
+  dlerror();
   void *f2_handle = dlopen("func2.shared", RTLD_LAZY | RTLD_GLOBAL);
   if (f2_handle == NULL) {
     fprintf(stderr, "unable to open 'func2.shared': %s\n", dlerror());
     return EXIT_FAILURE;
   }
 
+  dlerror();
+  void (*gcov_flush)() = (void (*)())dlsym(f1_handle, "__gcov_flush");
+  if (gcov_flush != NULL) {
+    fprintf(stderr, "__gcov_flush should not be visible in func.shared'\n");
+    return EXIT_FAILURE;
+  }
+
+  dlerror();
+  void (*f1_flush)() = (void (*)())dlsym(f1_handle, "llvm_gcov_flush");
+  if (f1_flush == NULL) {
+    fprintf(stderr, "unable to find llvm_gcov_flush in func.shared': %s\n", dlerror());
+    return EXIT_FAILURE;
+  }
+  f1_flush();
+
+  dlerror();
+  void (*f2_flush)() = (void (*)())dlsym(f2_handle, "llvm_gcov_flush");
+  if (f2_flush == NULL) {
+    fprintf(stderr, "unable to find llvm_gcov_flush in func2.shared': %s\n", dlerror());
+    return EXIT_FAILURE;
+  }
+  f2_flush();
+
+  if (f1_flush == f2_flush) {
+    fprintf(stderr, "Same llvm_gcov_flush found in func.shared and func2.shared\n");
+    return EXIT_FAILURE;
+  }
+
+  dlerror();
   if (dlclose(f2_handle) != 0) {
     fprintf(stderr, "unable to close 'func2.shared': %s\n", dlerror());
     return EXIT_FAILURE;
   }
 
   return EXIT_SUCCESS;
 }