This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Introduce libgcov.so run-time library (PR gcov-profile/84107).


On 08/29/2018 02:20 PM, Richard Biener wrote:
> On Wed, Aug 29, 2018 at 2:13 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> On 08/29/2018 01:18 PM, Richard Biener wrote:
>>> On Wed, Aug 29, 2018 at 12:44 PM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> On 08/29/2018 11:17 AM, Richard Biener wrote:
>>>>> On Wed, Aug 29, 2018 at 10:31 AM Martin Liška <mliska@suse.cz> wrote:
>>>>>>
>>>>>> Hello.
>>>>>>
>>>>>> Moving the thread from gcc ML into gcc-patches. That's first implementation
>>>>>> of shared libgcov library. Currently inclusion of t-libgcov is added only
>>>>>> to *-linux targets. Is it fine to add to all configurations that already
>>>>>> include 't-slibgcc t-slibgcc-elf-ver'?
>>>>>>
>>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>>>
>>>>> I understand this is to make profiling work with multiple DSOs (all
>>>>> instrumented).
>>>>
>>>> Yes.
>>>>
>>>>> But does this also make the case work when those DSOs are dlopened/dlclosed
>>>>> multiple times?
>>>>
>>>> That works fine even now, when a DSO is loaded, __gcov_init is called and
>>>> it registers gcov_info into __gcov_root.
>>>>
>>>> Following sample:
>>>>
>>>> #include <dlfcn.h>
>>>> #include <assert.h>
>>>> #include <unistd.h>
>>>>
>>>> int main()
>>>> {
>>>>   for (int i = 0; i < 10; i++)
>>>>   {
>>>>     void *handle = dlopen ("libfoo.so", RTLD_NOW);
>>>>     assert (handle);
>>>>
>>>>     void (*fn) (void) = dlsym (handle, "foo");
>>>>     assert (fn);
>>>>
>>>>     fn ();
>>>>     sleep (1);
>>>>   }
>>>>
>>>>   return 0;
>>>> }
>>>
>>> That doesn't dlclose ;)
>>
>> Yep, I tested that but with compiler (static libgcov) :/
>>
>> I noticed that make check for postgres generated invalid *.gcda files.
>> If's caused by fact that:
>>
>> libgcov-driver.c:
>>
>> /* Per-dynamic-object gcov state.  */
>> struct gcov_root __gcov_root;
>>
>> is not longer per-DSO, but global. So when a shared library is unloaded
>> with dlclose, it computes histogram of all gcov_info files. As the program
>> still runs, arcs counters are being updated.
>>
>>
>>>
>>>> has:
>>>>
>>>> $ gcov-dump -l foo.gcda
>>>> foo.gcda:data:magic `gcda':version `A82*'
>>>> foo.gcda:stamp 2235622999
>>>> foo.gcda:  a3000000:  22:PROGRAM_SUMMARY checksum=0x02bfe640
>>>> foo.gcda:                counts=2, runs=1, sum_all=20, run_max=10, sum_max=10
>>>> foo.gcda:                counter histogram:
>>>> foo.gcda:                 9: num counts=2, min counter=10, cum_counter=20
>>>> foo.gcda:  01000000:   3:FUNCTION ident=1636255671, lineno_checksum=0x2172766e, cfg_checksum=0xc0bbb23e
>>>> foo.gcda:    01a10000:   4:COUNTERS arcs 2 counts
>>>> foo.gcda:                   0: 10 10
>>>> foo.gcda:    01af0000:   2:COUNTERS time_profiler 1 counts
>>>> foo.gcda:                   0: 1
>>>>
>>>> which is fine, runs == 1 and arcs counter executed 10x.
>>>>
>>>>
>>>>   I understand that with the shared libgcov implementation this
>>>>> library must be finalized last?
>>>>
>>>> If you do profiling, then you depend on libgcov, thus it's loaded before a DSO (or executable)
>>>> is loaded.
>>>>
>>>>  Wouldn't it be better to have some additional
>>>>> object that lives in an executable only?  Or do we want profiled DSOs without
>>>>> profiling the executable using it?
>>>>
>>>> That should be rare, but it will be possible as __gcov_exit will be eventually called.
>>>
>>> I'm not too familiar but I guess each DSO _fini will call __gcov_exit?
>>
>> Yes.
>>
>>>
>>>>  Does that case work with the shared libgcov
>>>>> (with dlopen/dlclose)?
>>>>
>>>> Yes, I've just tested that.
>>>>
>>>>  Does it work when DSO1 is compiled with GCC N
>>>>> and DSO2 is compiled with GCC N+1 where the libgcov ABI changed?
>>>>
>>>> For such case we have:
>>>>
>>>> /* Exactly one of these will be live in the process image.  */
>>>> struct gcov_master __gcov_master =
>>>>   {GCOV_VERSION, 0};
>>>> ...
>>>> and it's check in __gcov_init.
>>>
>>> So it doesn't work.  Too bad, in the past we shipped a profiling (-pg)
>>> glibc build
>>> but that would then only work for the same compiler.
>>>
>>> I guess using a shared libgcov makes profiling the dynamic loader DSO
>>> impossible?
>>
>> You mean ld.so?
>>
>>> So we'd likely want a -static-libgcov option as well?
>>
>> If you use -static, that works now.
>>
>>>
>>>>>
>>>>> I think we need a better understanding of the situation before jumping to
>>>>> the conclusion that a shared libgcov is the solution.  Maybe better isolating
>>>>> the individual instances is better?
>>>>
>>>> It's undesired as seen in the PR. When doing profiling of indirect jumps
>>>> we need have exactly one __gcov_indirect_call_callee. That's because one can
>>>> do indirect calls that cross DSO boundaries.
>>>
>>> But usually you don't inline across DSO boundaries and after devirtualizing
>>> you still need to go through the PLT...
>>>
>>> Btw, when you dlopen/dlcose a DSO multiple times the same function can
>>> end up at different addresses, does gcov somehow deal with this?  It seems
>>> that the profile functions get the callee address.
>>
>> That's fine, profile_id is saved:
>>
>>      if (__gcov_indirect_call_callee != NULL)
>>        __gcov_indirect_call_profiler_v2 (profile_id, &current_function_decl);
>>
>>   tree_uid = build_int_cst
>>               (gcov_type_node,
>>                cgraph_node::get (current_function_decl)->profile_id);
>>
>>>
>>> Can you shortly tell why the testcase in the PR segfaults?  Does the issue
>>> only affect indirect call profiling?
>>
>> What happens is that there will exist 2 instances of:
>> void * __gcov_indirect_call_callee;
> 
> Ah.  Isn't there a trick to commonize those even across DSOs by using weakrefs
> and/or weak definitions...?

I'm aware of -Wl,--dynamic-list-data which we have documented in gcov manual.
But this trick does not work for the test-case Alexander wrote.

> 
>> one in main executable, and one another in DSO loaded via dlopen.
>> Then caller sets __gcov_indirect_call_callee to a function pointer, but when
>> the actual function is called it has it's own __gcov_indirect_call_callee instance
>> and it's zero. Then following dereferencing happens:
>>
>>   if (cur_func == __gcov_indirect_call_callee
>>       || (__LIBGCC_VTABLE_USES_DESCRIPTORS__
>>           && *(void **) cur_func == *(void **) __gcov_indirect_call_callee))
>>     __gcov_one_value_profiler_body (__gcov_indirect_call_counters, value, 0);
>>
>> Well, yes, it's only related to indirect profiling. And only benefit is that we maybe
>> miss a target of an indirect call. Profile for indirect call is beneficial only
>> for speculative inlining, which will not be possible because the callee lives in
>> a different DSO. Thus the code of such function will not be probably available
>> in compilation unit of a caller.
>>
>> That said, it looks the shared library libgcov.so is overkill.
> 
> It feels like that somehow.
> 
>> Alexander may explain how that can be beneficial?
>>
>> Note that I can fix the segfault being caused by inter-DSO calls.
> 
> I think that would be good and also amenable for backports.  Just
> check for NULL before
> dereferencing.

Sure. Let's wait for Alexander. Maybe he's got something we missed.

Martin

> 
> Richard.
> 
>> Martin
>>
>>>
>>> Richard.
>>>
>>>> Martin
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Martin
>>>>>>
>>>>>> libgcc/ChangeLog:
>>>>>>
>>>>>> 2018-08-28  Martin Liska  <mliska@suse.cz>
>>>>>>
>>>>>>         PR gcov-profile/84107
>>>>>>         * Makefile.in: Build libgcov.so objects separately,
>>>>>>         add rule for libgcov.map and how the library
>>>>>>         is linked.
>>>>>>         * config.host: Add t-libgcov and t-libgcov-elf-ver
>>>>>>         to *-linux targets.
>>>>>>         * config/t-libgcov: New file.
>>>>>>         * config/t-libgcov-elf-ver: New file.
>>>>>>         * libgcov-driver.c: Declare function if new L_gcov_shared
>>>>>>         is defined.
>>>>>>         * libgcov-interface.c: Likewise.
>>>>>>         * libgcov-merge.c: Likewise.
>>>>>>         * libgcov-profiler.c: Likewise.
>>>>>>         * libgcov-std.ver.in: New file.
>>>>>>         * libgcov.h (__gcov_init): Remove ATTRIBUTE_HIDDEN.
>>>>>>         (__gcov_exit): Likewise.
>>>>>>         (__gcov_merge_add): Likewise.
>>>>>>         (__gcov_merge_time_profile): Likewise.
>>>>>>         (__gcov_merge_single): Likewise.
>>>>>>         (__gcov_merge_ior): Likewise.
>>>>>>         (__gcov_merge_icall_topn): Likewise.
>>>>>>         (gcov_sort_n_vals): Likewise.
>>>>>>         (__gcov_fork): Likewise.
>>>>>>         (__gcov_execl): Likewise.
>>>>>>         (__gcov_execlp): Likewise.
>>>>>>         (__gcov_execle): Likewise.
>>>>>>         (__gcov_execv): Likewise.
>>>>>>         (__gcov_execvp): Likewise.
>>>>>>         (__gcov_execve): Likewise.
>>>>>> ---
>>>>>>  libgcc/Makefile.in              | 56 ++++++++++++++++++++++++++++++---
>>>>>>  libgcc/config.host              |  2 +-
>>>>>>  libgcc/config/t-libgcov         | 46 +++++++++++++++++++++++++++
>>>>>>  libgcc/config/t-libgcov-elf-ver |  3 ++
>>>>>>  libgcc/libgcov-driver.c         |  4 +--
>>>>>>  libgcc/libgcov-interface.c      | 28 ++++++++---------
>>>>>>  libgcc/libgcov-merge.c          | 14 ++++-----
>>>>>>  libgcc/libgcov-profiler.c       | 34 ++++++++++----------
>>>>>>  libgcc/libgcov-std.ver.in       | 53 +++++++++++++++++++++++++++++++
>>>>>>  libgcc/libgcov.h                | 31 +++++++++---------
>>>>>>  10 files changed, 209 insertions(+), 62 deletions(-)
>>>>>>  create mode 100644 libgcc/config/t-libgcov
>>>>>>  create mode 100644 libgcc/config/t-libgcov-elf-ver
>>>>>>  create mode 100644 libgcc/libgcov-std.ver.in
>>>>>>
>>>>>>
>>>>
>>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]