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

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.
Alexander may explain how that can be beneficial?

Note that I can fix the segfault being caused by inter-DSO calls.

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]