This is an archive of the discontinued LLVM Phabricator instance.

Make __gcov_flush flush counters for all shared libraries
ClosedPublic

Authored by marco-c on Jun 25 2018, 3:26 AM.

Details

Summary

This will make the behavior of __gcov_flush match the GCC behavior.

I would like to rename gcov_flush to llvm_gcov_flush (in case of programs linking to libraries built with different compilers), but I guess we can't for compatibility reasons.

Diff Detail

Repository
rL LLVM

Event Timeline

marco-c created this revision.Jun 25 2018, 3:26 AM
Herald added subscribers: Restricted Project, llvm-commits. · View Herald TranscriptJun 25 2018, 3:26 AM
chh added a subscriber: chh.Jun 25 2018, 9:57 AM

This change will cause each dynamic library to dump modules (gcda files) from other dynamic objects as well -- leading to wrong profile count merged.

lib/profile/GCDAProfiling.c
524 ↗(On Diff #152648)

This code looks wrong. I don't see how this can delete all write nodes from this dynamic object.

test/profile/Inputs/instrprof-dlopen-func.c.gcov
6 ↗(On Diff #152648)

this is not correct. func is executed only once.

test/profile/Inputs/instrprof-dlopen-func2.c.gcov
6 ↗(On Diff #152648)

this is not correct.

marco-c added inline comments.Jun 26 2018, 2:15 PM
lib/profile/GCDAProfiling.c
524 ↗(On Diff #152648)

Aargh, I don't know what I was thinking here, I'll fix it.

test/profile/Inputs/instrprof-dlopen-func.c.gcov
6 ↗(On Diff #152648)

I think this is pre-existing behavior unfortunately, I'll test again without the patch.

marco-c updated this revision to Diff 153690.Jul 2 2018, 3:21 AM

I'm using two lists, one for the current dynamic object, one shared between all. We could make the shared one a list of lists, but I feel it would make the code more complex, with no large benefit (if you disagree, I can update the patch).
At writeout, we only write out the current dynamic object counters; at __gcov_flush, we write out and reset counters for all dynamic objects.

A question, why do we need both a writeout function and a flush function? Wouldn't it be OK to just always use flush? When we call writeout, we are atexit, so it doesn't really matter if we clear or not the counters after the writeout, or does it?

davidxl added inline comments.Jul 2 2018, 12:30 PM
test/profile/Inputs/instrprof-shared-main-gcov-flush-before-shared-call.c
9 ↗(On Diff #153690)

Make test case more complete:

  1. add a non shared call after flush as well
  1. test the case when normal exit is not invoked -- in this case, the profile counts accumulated after the flush should not be recorded.
test/profile/Inputs/instrprof-shared-main-gcov-flush-no-writeout.c
9 ↗(On Diff #153690)

perhaps call 'abort' or '_exit' instead.

test/profile/Inputs/instrprof-shared-main-gcov-flush.c
9 ↗(On Diff #153690)

share the same source with the flush-no-writeout case by using a guarding macro: use using exit, and the other uses _exit.

More suggestion on the test case:

Define another function, say 'bar' in the main source file and call it before and after the __gcov_flush() call.

Add another call to 'foo' after the gcov_flush call.

The 'gcov' outputs between the normal write out at exit vs flushed output (when _exit is used) should be different.

(Just noticed that you have a flush-before-call test case -- can consider merge it into one source with macros).

marco-c updated this revision to Diff 153929.Jul 3 2018, 9:32 AM

I've merged all the shared / __gcov_flush tests into a single file and made the test a little more involved by adding a "bar" function in the main file.

davidxl added inline comments.Jul 3 2018, 10:14 AM
lib/profile/GCDAProfiling.c
116 ↗(On Diff #153929)

this loop is still wrong.

Also it is possible to simply the code by removing the if-then-else:

struct fn_node** prev = &list->head;
 while ((*prev) != element) 
      prev = &(*prev)->next;
 (*prev) = element->next;
537 ↗(On Diff #153929)

It suffices to do this outside the loop by making current list's head to null.

538 ↗(On Diff #153929)

The second one has quadratic behavior.

If the current list is remembered in the fn_node object, you can simply do a one sweep by comparing the recorded list with the current list.

test/profile/Inputs/instrprof-dlopen-func.c.gcov
6 ↗(On Diff #152648)

Have you examined why this is wrong? If it is not regression, it is worth fixing as a follow up.

marco-c updated this revision to Diff 154015.Jul 3 2018, 4:28 PM

I've taken another approach which looks simpler to me. Instead of having two lists, I only have a shared list and I use a static variable's address as an identifier for a dynamic object.

I've also added another test (which would have failed with the previous iteration of the patch) which dlopens three libraries.

I will file follow-up bugs for things that don't look right in the llvm-cov output which are not regressions from this patch:

  • instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).
  • instrprof-dlopen-func{2,3}.c.gcov: the only line in the function should be covered once and not thrice

Regarding removing writeout and only always using __gcov_flush, do you see any disadvantage if we do that?

Regarding llvm_gcov_flush that was recently added, should I remove it since we can now unhide __gcov_flush?

marco-c added subscribers: davidxl, dvyukov, eugenis and 7 others.EditedJul 3 2018, 5:25 PM

On Tue, Jul 3, 2018 at 4:29 PM Marco Castelluccio via Phabricator <reviews@reviews.llvm.org> wrote:

marco-c updated this revision to Diff 154015.
marco-c added a comment.

I've taken another approach which looks simpler to me. Instead of having two lists, I only have a shared list and I use a static variable's address as an identifier for a dynamic object.

I've also added another test (which would have failed with the previous iteration of the patch) which dlopens three libraries.

I will file follow-up bugs for things that don't look right in the llvm-cov output which are not regressions from this patch:

- instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).
- instrprof-dlopen-func{2,3}.c.gcov: the only line in the function should be covered once and not thrice

Regarding removing writeout and only always using __gcov_flush, do you see any disadvantage if we do that?

writeout function only writes the profile for the current dynamic module which is different from the behavior of __gcov_flush. The form is expected when shared lib is unloaded -- i.e., do not dump other load module's profile unless being explicitly told so (to use gcov_flush). The write out is similar to gcc's gcov_exit. In short, we should keep it as it is.

Sorry, what I meant is that we could remove llvm_gcov_writeout and only keep llvm_gcov_flush (they are defined in lib/Transforms/Instrumentation/GCOVProfiling.cpp and passed to llvm_gcov_init). We currently have writeout functions and flush functions, where the difference is just that the flush function is also resetting the counters to 0.
At exit, instead of calling llvm_gcov_writeout, we could call llvm_gcov_flush. Since we are at exit, the fact that we reset the counters to 0 for the current module doesn't matter (as the module is not usable anymore after that).
At exit we would still call llvm_gcov_flush only for the module that is exiting, at gcov_flush we would call __llvm_gcov_flush for all modules.

Regarding llvm_gcov_flush that was recently added, should I remove it since we can now unhide __gcov_flush?

I suggest we keep it -- there is no harm keeping it. llvm_gcov_flush can be documented to be always non hidden. There might also be a use case that user want to discover the dumper using dlsym, but only want to dump profile for that shared library. In that case, a symbol with protected visibility is needed which is a wrapper to the write-out method.

Should we make llvm_gcov_flush hidden and gcov_flush unhidden then? gcov_flush unhidden would be for compatibility with GCC.

On Tue, Jul 3, 2018 at 4:29 PM Marco Castelluccio via Phabricator <reviews@reviews.llvm.org> wrote:

marco-c updated this revision to Diff 154015.
marco-c added a comment.

I've taken another approach which looks simpler to me. Instead of having two lists, I only have a shared list and I use a static variable's address as an identifier for a dynamic object.

I've also added another test (which would have failed with the previous iteration of the patch) which dlopens three libraries.

I will file follow-up bugs for things that don't look right in the llvm-cov output which are not regressions from this patch:

- instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).
- instrprof-dlopen-func{2,3}.c.gcov: the only line in the function should be covered once and not thrice

Regarding removing writeout and only always using __gcov_flush, do you see any disadvantage if we do that?

writeout function only writes the profile for the current dynamic module which is different from the behavior of __gcov_flush. The form is expected when shared lib is unloaded -- i.e., do not dump other load module's profile unless being explicitly told so (to use gcov_flush). The write out is similar to gcc's gcov_exit. In short, we should keep it as it is.

Sorry, what I meant is that we could remove llvm_gcov_writeout and only keep llvm_gcov_flush (they are defined in lib/Transforms/Instrumentation/GCOVProfiling.cpp and passed to llvm_gcov_init). We currently have writeout functions and flush functions, where the difference is just that the flush function is also resetting the counters to 0.
At exit, instead of calling llvm_gcov_writeout, we could call llvm_gcov_flush. Since we are at exit, the fact that we reset the counters to 0 for the current module doesn't matter (as the module is not usable anymore after that).
At exit we would still call llvm_gcov_flush only for the module that is exiting, at gcov_flush we would call __llvm_gcov_flush for all modules.

Regarding llvm_gcov_flush that was recently added, should I remove it since we can now unhide __gcov_flush?

I suggest we keep it -- there is no harm keeping it. llvm_gcov_flush can be documented to be always non hidden. There might also be a use case that user want to discover the dumper using dlsym, but only want to dump profile for that shared library. In that case, a symbol with protected visibility is needed which is a wrapper to the write-out method.

Should we make llvm_gcov_flush hidden and gcov_flush unhidden then? gcov_flush unhidden would be for compatibility with GCC.

If you are going to hide llvm_gcov_flush(), you might as well delete it and break our implementation that way. We are only going to try to use llvm_gcov_flush() temporarily to restore some missing functionality. When __gcov_flush() becomes available again, we will switch back to it for simplicity.

davidxl accepted this revision.Jul 3 2018, 8:54 PM

LGTM ( watch out any potential breakages)

lib/profile/GCDAProfiling.c
152 ↗(On Diff #154015)

this is not needed.

This revision is now accepted and ready to land.Jul 3 2018, 8:54 PM
 Sorry, what I meant is that we could remove __llvm_gcov_writeout and only keep __llvm_gcov_flush (they are defined in lib/Transforms/Instrumentation/GCOVProfiling.cpp and passed to llvm_gcov_init). We currently have writeout functions and flush functions, where the difference is just that the flush function is also resetting the counters to 0.
 At exit, instead of calling __llvm_gcov_writeout, we could call __llvm_gcov_flush. Since we are at exit, the fact that we reset the counters to 0 for the current module doesn't matter (as the module is not usable anymore after that).
At exit we would still call __llvm_gcov_flush only for the module that is exiting, at __gcov_flush we would call __llvm_gcov_flush for all modules.

This sounds safe, but this does not belong to this patch.

Yeah, definitely a separate thing, I'll do it and submit it in another revision.

Looks like Stephane (the only user of the new interface) is ok for you to remove it completely, which is fine by me.

OK, I'll remove it then.

Before landing, I will "stress test" this patch on our CI to make sure everything is still working as before.

  • instrprof-shared-main-gcov-flush_no-writeout.c.gcov: lines 23, 26 and 30 should not be covered; lines 33 and 35 should not be covered (they are considered as uncoverable instead).

I've filed https://bugs.llvm.org/show_bug.cgi?id=38067. It's unrelated to the presence of shared libraries.

  • instrprof-dlopen-func{2,3}.c.gcov: the only line in the function should be covered once and not thrice

I've filed https://bugs.llvm.org/show_bug.cgi?id=38065, this one too is unrelated to the presence of shared libraries.

I've also filed https://bugs.llvm.org/show_bug.cgi?id=38066, about wrong line hit counts when exceptions are enabled.

I've tested the patch on our CI and it was working correctly. I'll land it shortly.

marco-c updated this revision to Diff 154249.Jul 5 2018, 8:53 AM

Patch rebased. I've removed llvm_gcov_flush and made __gcov_flush visible.

This revision was automatically updated to reflect the committed changes.

It looks like this causes build bot failures on s390x-linux. Three of the new tests fail:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/17143

/home/uweigand/sandbox/buildbot/clang-s390x-linux/llvm/projects/compiler-rt/test/profile/Inputs/instrprof-dlopen-dlclose-main.c.gcov:11:15: error: expected string not found in input
// CHECK-NEXT:        1:    6:  dlerror();
              ^
instrprof-dlopen-dlclose-main.c.gcov:11:1: note: scanning from here
4294967296:    6:  dlerror();

This looks suspiciously like an endian problem somewhere, we get 4294967296 == 0x1_0000_0000 instead of 0x0_0000_0001 ...
(s390x is a big-endian machine.)