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

[Xinliang David Li via llvm-commits]

On Tue, Jul 3, 2018 at 5:24 PM Marco Castelluccio <mcastelluccio at mozilla.com>
wrote:

> Il 04/07/2018 00:52, Xinliang David Li ha scritto:
>
>
>
> 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.
>


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



>
>
>
>
>> 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.
>

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

David

>
> - Marco.
>
>
>
>
>
>
> David
>
>
>
>
>>
>> https://reviews.llvm.org/D48538
>>
>> Files:
>>   lib/profile/GCDAProfiling.c
>>   test/profile/Inputs/instrprof-dlopen-dlclose-main.c
>>   test/profile/Inputs/instrprof-dlopen-dlclose-main.c.gcov
>>   test/profile/Inputs/instrprof-dlopen-dlclose-main_three-libs.c.gcov
>>   test/profile/Inputs/instrprof-dlopen-func.c.gcov
>>   test/profile/Inputs/instrprof-dlopen-func2.c.gcov
>>   test/profile/Inputs/instrprof-dlopen-func3.c
>>   test/profile/Inputs/instrprof-dlopen-func3.c.gcov
>>   test/profile/Inputs/instrprof-shared-lib.c.gcov
>>   test/profile/Inputs/instrprof-shared-lib_called-twice.c.gcov
>>   test/profile/Inputs/instrprof-shared-lib_in-loop.c.gcov
>>   test/profile/Inputs/instrprof-shared-main-gcov-flush.c
>>   test/profile/Inputs/instrprof-shared-main-gcov-flush_no-writeout.c.gcov
>>
>> test/profile/Inputs/instrprof-shared-main-gcov-flush_shared-call-after.c.gcov
>>
>> test/profile/Inputs/instrprof-shared-main-gcov-flush_shared-call-before-after.c.gcov
>>
>> test/profile/Inputs/instrprof-shared-main-gcov-flush_shared-call-before.c.gcov
>>   test/profile/Inputs/instrprof-shared-main.c.gcov
>>   test/profile/instrprof-dlopen-dlclose-gcov.test
>>   test/profile/instrprof-gcov-two-objects.test
>>   test/profile/instrprof-shared-gcov-flush.test
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180703/d6af025c/attachment.html>