> Thanks! Can you attach the patch file for the proposed change? > > David > > On Fri, Nov 1, 2013 at 2:08 PM, Rong Xu <x...@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. > > > > 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? > > > > 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. Honza > > > > -Rong
