Re: [RFC] libgcov.c re-factoring and offline profile-tool

[Rong Xu <xur at google dot com>]

Honza, Thanks for the comments!
Attached is the patch.
Passed spec2006 fdo build and profiledbootstrap in gcc.
I'm doing some other tests.

-Rong

On Mon, Nov 4, 2013 at 1:55 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Thanks! Can you attach the patch file for the proposed change?
>>
>> David
>>
>> On Fri, Nov 1, 2013 at 2:08 PM, Rong Xu <xur@google.com> wrote:
>> > libgcov.c is the main file to generate libgcov.a. This single source generates
>> > 21 object files and then archives to a library. These objects are of
>> > very different purposes but they are in the same file guarded by various macros.
>> > The source file becomes quite large and its readability becomes very
>> > low. In its current state:
>> > 1) The code is very hard to understand by new developers;
>
> Agreed, libgcov has became hard to maintain since several parts are written
> in very low-level way.
>> > 2) It makes regressions more likely when making simple changes;
>> > More importantly,
>> > 3) it makes code reuse/sharing very difficult. For instance, Using
>> > libgcov to implement an offline profile tool (see below); kernel FDO
>> > support etc.
>
> Yep, it was my longer term plan to do this, too.
>> >
>> > We came to this issue when working on an offline tool to handle
>> > profile counters.
>> > Our plan is to have a profile-tool can merge, diff, collect statistics, and
>> > better dump raw profile counters. This is one of the most requested features
>> > from out internal users. This tool can also be very useful for any FDO/coverage
>> > users. In the first part of tool, we want to have the weight
>> > merge. Again, to reuse the code, we have to change libgcov.c. But we don't want
>> > to further diverge libgcov.a in our google branches from the trunk.
>> >
>> > Another issue in libgcov.c is function gcov_exit(). It is a huge function
>> > with about 467 lines of code. This function traverses gcov_list and dumps all
>> > the gcda files. The code for merging the gcda files also sits in the same
>> >  function. The structure makes the code-reuse and extension really difficult
>> > (like in kernel FDO, we have to break this function to reuse some of the code).
>> >
>> > We propose to break gcov_exit into smaller functions and split libgcov.c into
>> > several files. Our plan for profile-tool is listed in (3).
>> >
>> > 1. break gcov_exit() into the following structure:
>> >    gcov_exit()
>> >       --> gcov_exit_compute_summary()
>> >       --> allocate_filename_struct()
>> >           for gi_ptr in gcov_list
>> >             --> gcov_exit_dump_gcov()
>> >                    --> gcov_exit_open_gcda_file()
>> >                    --> gcov_exit_merge_gcda ()
>> >                    --> gcov_exit_write_gcda ()
>
> Just a short comment that summaries are also produced during the merging - i.e. they are
> done on per-file basis. But otherwise I agree.
> We should be cureful to not increase footprint of libgcov much for embedded users, but
> I believe changes like this can be done w/o additional overhead in the optimized library.

The merge of summary has two parts. One is to find the summary to
merge to. That is in gcov_exit_merge_gcda().
The other part of do the actual merging of summary is in gcov_exit_dump_gcov().

I tried to keep the current work flow. So except for a few more calls,
I don't think there is much overhead.

>> >
>> > 2. split libgcov.c into the following files:
>> >      libgcov-profiler.c
>> >      libgcov-merge.c
>> >      libgcov-interface.c
>> >      libgcov-driver.c
>> >        libgcov-driver-system.c (source included into libgcov-driver.c)
>
> Seems resonable.  Splitting i/o stuff into separate module (driver) is something I planned
> for long time.  What is difference in between libgcov-interface and libgcov-driver?

Basically libgcov-driver handles all the dumping of gcov-info.
libgcov-interface is the API used by instrumentation, like the wrapper for fork.


>> >
>> > The details of the splitting:
>> > libgcov-interface.c
>> > /* This file contains API functions to other programs and the supporting
>> >    functions.  */
>> >   __gcov_dump()
>> >   __gcov_execl()
>> >   __gcov_execle()
>> >   __gcov_execlp()
>> >   __gcov_execv()
>> >   __gcov_execve()
>> >   __gcov_execvp()
>> >   __gcov_flush()
>> >   __gcov_fork()
>> >   __gcov_reset()
>> >   init_mx()
>> >   init_mx_once()
>> >
>> > libgcov-profiler.c
>> > /* This file contains runtime profile handlers.  */
>> >   variables:
>> >     __gcov_indirect_call_callee
>> >     __gcov_indirect_call_counters
>> >   functions:
>> >     __gcov_average_profiler()
>> >     __gcov_indirect_call_profiler()
>> >     __gcov_indirect_call_profiler_v2()
>> >     __gcov_interval_profiler()
>> >     __gcov_ior_profiler()
>> >     __gcov_one_value_profiler()
>> >     __gcov_one_value_profiler_body()
>> >     __gcov_pow2_profiler()
>> >
>> > libgcov-merge.c
>> > /* This file contains the merge functions for various counters.  */
>> >   functions:
>> >     __gcov_merge_add()
>> >     __gcov_merge_delta()
>> >     __gcov_merge_ior()
>> >     __gcov_merge_single()
>> >
>> > libcov-driver.c
>> > /* This file contains the gcov dumping functions. We separate the
>> >    system dependent part to libgcov-driver-system.c.  */
>> >   variables:
>> >     gcov_list
>> >     gcov_max_filename
>> >     gcov_dump_complete
>> >     ------ newly added file static variables --
>> >     this_prg
>> >     all_prg
>> >     crc32
>> >     gi_filename
>> >     fn_buffer
>> >     fn_tail
>> >     sum_buffer
>> >     next_sum_buffer
>> >     sum_tail
>> >     ----- end -----
>> >   functions:
>> >     free_fn_data()
>> >     buffer_fn_data()
>> >     crc32_unsigned()
>> >     gcov_version()
>> >     gcov_histogram_insert()
>> >     gcov_compute_histogram()
>> >     gcov_clear()
>> >     __gcov_init()
>> >     gcov_exit()
>> >     ------- newly added static functions --
>> >     gcov_exit_compute_summary ()
>> >     gcov_exit_merge_gcda()
>> >     gcov_exit_write_gcda()
>> >     gcov_exit_dump_gcov()
>> >     ----- end -----
>> >
>> > libgcov-driver-system.c
>> > /* system dependent part of ligcov-driver.c.  */
>> >   functions:
>> >     create_file_directory()
>> >     ------- newly added static functions --
>> >     gcov_error() /* This replaces all fprintf(stderr, ...) */
>> >     allocate_filename_struct()
>> >     gcov_exit_open_gcda_file()
>> >
>> > 3. add offline profile-tool support.
>> >    We will change the interface of merge functions to make it
>> >    take in-memory gcov_info list, and a weight as the arguments.
>> >
>> >    We will add libgcov-tool.c. It has two APIs:
>> >    (1) read_profile_dir(): read a profile directory and reconstruct the
>> >        gcov_info link list in-memory.
>> >    (2) merge_profiles(): merge two in-memory gcov_info link list.
>> >
>> >    We also add profile-tool.c in gcc directory. It will source-include
>> >    libgcov-tool.c and build a host binary. (We don't do library style
>> >    because that will need a hard dependence from gcc to libgcc).
>> >    profile-tool.c will mainly handle user options and the write-out of
>> >    gcov-info link list. Some changes in gcov-io.[ch] will be also needed.
>> >
>> > Thanks,
>
> Thanks, it all looks good to me.  One thing I also always wondered about is why
> libgcov ended up being partly in libgcc and partly in gcc (the headers) directory.
> It would make more sense to have libgcov directory like we have for other runtime
> libraries.
>
I agree with you -- it's better to have a seperate directoy for libgcov.

The other part should be re-refatored is the gcov-io.[ch]. I think they
are even messier than libgcov.c. I plan to clean them after libgcvo.c.

> Honza
>> >
>> > -Rong

Attachment: libgcov_patch.txt
Description: Text document