I wonder if it makes sense to get rid of the conditional compile such as #ifdef L_gcov_xxxxx ..
#endif This has the advantage of producing slightly smaller instrumented binary, but this benefit can also be achieved via -ffunction-sections and let linker to garbage collect unused functions. (probably as a follow up if it makes sense). David On Mon, Nov 4, 2013 at 2:43 PM, Rong Xu <x...@google.com> wrote: > 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 <hubi...@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 <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. > > 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
