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