[PATCH] D48538: Make __gcov_flush flush counters for all shared libraries

[Stephen Hines via Phabricator via llvm-commits]

srhines added a comment.

In https://reviews.llvm.org/D48538#1151578, @marco-c wrote:

> > On Tue, Jul 3, 2018 at 4:29 PM Marco Castelluccio via Phabricator <reviews at 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.


https://reviews.llvm.org/D48538