On 08/29/2018 02:20 PM, Richard Biener wrote:
> On Wed, Aug 29, 2018 at 2:13 PM Martin Liška <mli...@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 <mli...@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 <mli...@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, ¤t_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 <mli...@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
>>>>>>
>>>>>>
>>>>
>>
